Remove static library targets from Atom_RPI #18315
Conversation
mprettner-huawei
added
the
sig/graphics-audio
There was a problem hiding this comment.
Doesn't work for me with clang and mold on linux, I get many errors in the form of:
[1/665 1.3/sec] Linking CXX shared library bin/profile/libAtom_RPI.Edit.so
FAILED: bin/profile/libAtom_RPI.Edit.so
: && /usr/bin/clang++ -fPIC -fno-exceptions -fvisibility=hidden -fvisibility-inlines-hidden -Wall -Werror -Wno-inconsistent-missing-override -Wrange-loop-analysis -Wno-unknown-warning-option -Wno-parentheses -Wno-reorder -Wno-switch -Wno-undefined-var-template -msse4.1 -O2 -g -fstack-protector-all -fstack-check -fuse-ld=mold -Wl,--no-undefined -fpie -Wl,-z,relro,-z,now -Wl,-z,noexecstack -shared -Wl,-soname,libAtom_RPI.Edit.so -o bin/profile/libAtom_RPI.Edit.so External/RPI-623b9f2e/Code/CMakeFiles/Atom_RPI.Edit.dir/profile/Unity/unity_3_cxx.cxx.o External/RPI-623b9f2e/Code/CMakeFiles/Atom_RPI.Edit.dir/profile/Unity/unity_2_cxx.cxx.o External/RPI-623b9f2e/Code/CMakeFiles/Atom_RPI.Edit.dir/profile/Unity/unity_1_cxx.cxx.o External/RPI-623b9f2e/Code/CMakeFiles/Atom_RPI.Edit.dir/profile/Unity/unity_0_cxx.cxx.o -Wl,-rpath,"\$ORIGIN" lib/profile/libAtomCore.a lib/profile/libAzToolsFramework.a lib/profile/libAtom_RHI.Edit.a bin/profile/libAtom_RPI.Public.so lib/profile/libAzToolsFramework.a /fast/o3de/packages/packages/SQLite-3.37.2-rev1-linux/SQLite/lib/libsqlite3.a lib/profile/libAzFramework.NativeUI.a /usr/lib/libxcb-xkb.so /usr/lib/libxcb-xfixes.so /usr/lib/libxcb.so /usr/lib/libxkbcommon-x11.so /usr/lib/libxkbcommon.so /usr/lib/libX11.so -lxcb-xinput bin/profile/libAzQtComponents.so /fast/o3de/packages/packages/qt-5.15.2-rev9-linux/qt/lib/libQt5Svg.so.5.15.1 /fast/o3de/packages/packages/qt-5.15.2-rev9-linux/qt/lib/libQt5Widgets.so.5.15.1 /fast/o3de/packages/packages/qt-5.15.2-rev9-linux/qt/lib/libQt5Gui.so.5.15.1 /fast/o3de/packages/packages/qt-5.15.2-rev9-linux/qt/lib/libQt5Xml.so.5.15.1 /fast/o3de/packages/packages/qt-5.15.2-rev9-linux/qt/lib/libQt5Core.so.5.15.1 lib/profile/libAtom_RHI.Public.a lib/profile/libAtom_RHI.Reflect.a lib/profile/libAtomCore.a lib/profile/libAzFramework.a lib/profile/libAzNetworking.a lib/profile/libAzCore.a /usr/lib/libunwind.so -lpthread -latomic -lpthread -latomic /fast/o3de/packages/packages/Lua-5.4.4-rev1-linux/Lua/lib/release/liblualib.a /fast/o3de/packages/packages/zlib-1.2.11-rev5-linux/zlib/lib/libz.a /fast/o3de/packages/packages/cityhash-1.1-multiplatform/cityhash/build/linux/clang-3.8/release/libcityhash.a bin/profile/libO3DEKernel.so -ldl /usr/lib/libzstd.so /fast/o3de/packages/packages/OpenSSL-1.1.1t-rev1-linux/OpenSSL/lib/libssl.a /fast/o3de/packages/packages/OpenSSL-1.1.1t-rev1-linux/OpenSSL/lib/libcrypto.a /fast/o3de/packages/packages/lz4-1.9.4-rev2-linux/lz4/lib/liblz4.a && cd /fast/o3de/insource/build/linux/External/RPI-623b9f2e/Code && /usr/bin/cmake -P /data/projects/o3de/code/cmake/Platform/Linux/ProcessDebugSymbols.cmake /usr/bin/strip /usr/bin/objcopy /fast/o3de/insource/build/linux/bin/profile/libAtom_RPI.Edit.so dbg SHARED_LIBRARY DETACH
mold: error: undefined symbol: AZ::RPI::MaterialVersionUpdate::AddAction(AZ::RPI::MaterialVersionUpdate::Action const&, AZStd::function<AZ::RPI::MaterialPropertyValue (AZ::Name const&, AZ::RPI::MaterialPropertyValue const&)>)
>>> referenced by unity_0_cxx.cxx
>>> External/RPI-623b9f2e/Code/CMakeFiles/Atom_RPI.Edit.dir/profile/Unity/unity_0_cxx.cxx.o:(AZ::ScriptDataContext::InspectTable(int, AZ::ScriptDataContext&))
mold: error: undefined symbol: AZ::RPI::ShaderAsset::GetDefaultShaderOptions() const
>>> referenced by unity_0_cxx.cxx
>>> External/RPI-623b9f2e/Code/CMakeFiles/Atom_RPI.Edit.dir/profile/Unity/unity_0_cxx.cxx.o:(AZ::RPI::MaterialTypeSourceData::AddShaders(AZ::RPI::MaterialTypeAssetCreator&, AZ::Name const&, AZStd::vector<AZ::RPI::MaterialTypeSourceData::ShaderVariantReferenceData, AZStd::allocator> const&, AZStd::basic_string_view<char, AZStd::char_traits<char> >) const)
mold: error: undefined symbol: AZ::RPI::MaterialTypeAssetCreator::ClaimShaderOptionOwnership(AZ::Name const&)
>>> referenced by unity_0_cxx.cxx
>>> External/RPI-623b9f2e/Code/CMakeFiles/Atom_RPI.Edit.dir/profile/Unity/unity_0_cxx.cxx.o:(AZ::RPI::MaterialTypeSourceData::BuildPropertyList(AZStd::basic_string<char, AZStd::char_traits<char>, AZStd::allocator> const&, AZ::RPI::MaterialTypeAssetCreator&, AZ::RPI::MaterialNameContext, AZ::RPI::MaterialTypeSourceData::PropertyGroup const*) const)
>>> referenced by unity_0_cxx.cxx
>>> External/RPI-623b9f2e/Code/CMakeFiles/Atom_RPI.Edit.dir/profile/Unity/unity_0_cxx.cxx.o:(AZ::RPI::MaterialTypeSourceData::AddFunctors(AZ::RPI::MaterialTypeAssetCreator&, AZ::Name const&, AZStd::vector<AZStd::intrusive_ptr<AZ::RPI::MaterialFunctorSourceDataHolder>, AZStd::allocator> const&, AZStd::basic_string_view<char, AZStd::char_traits<char> >) const)
mold: error: undefined symbol: AZ::RPI::MaterialTypeAssetCreator::ConnectMaterialPropertyToShaderEnabled(AZ::Name const&)
>>> referenced by unity_0_cxx.cxx
>>> External/RPI-623b9f2e/Code/CMakeFiles/Atom_RPI.Edit.dir/profile/Unity/unity_0_cxx.cxx.o:(AZ::RPI::MaterialTypeSourceData::BuildProperty(AZStd::basic_string<char, AZStd::char_traits<char>, AZStd::allocator> const&, AZ::RPI::MaterialTypeAssetCreator&, AZ::Name const&, AZ::RPI::MaterialNameContext, AZ::Name const&, AZ::RPI::MaterialPropertySourceData const&) const)
|
Awesome work, we need more of these kind of improvements! |
Now it works; the problem was that O3DE adds the compile flag |
mprettner-huawei
added
sig/build
mprettner-huawei
force-pushed
the
Atom_RPI-cmake-sharedlib
branch
from
ba041ed to
a05d297
Compare
| @@ -90,7 +90,7 @@ define_property(TARGET PROPERTY RUNTIME_DEPENDENCIES_DEPENDS | |||
| # \arg:AUTOGEN_RULES a set of AutoGeneration rules to be passed to the AzAutoGen expansion system | |||
| function(ly_add_target) | |||
|
|
|||
| set(options STATIC SHARED MODULE GEM_STATIC GEM_MODULE OBJECT HEADERONLY EXECUTABLE APPLICATION IMPORTED AUTOMOC AUTOUIC AUTORCC NO_UNITY) | |||
| set(options STATIC SHARED MODULE GEM_STATIC GEM_MODULE OBJECT HEADERONLY EXECUTABLE APPLICATION IMPORTED AUTOMOC AUTOUIC AUTORCC NO_UNITY EXPORT_ALL_SYMBOLS) | |||
There was a problem hiding this comment.
Isn't this a potentially global change that will massively increase the size of all binaries to include all symbols?
Isn't there maybe a more local change that could be made, like, some sort of preprocessor directive or export line that can be added to just the functions in question? The entire engine was working without this change...
There was a problem hiding this comment.
@mprettner-huawei , I'd suggest to share a before and after DLL sizes to compare.
There was a problem hiding this comment.
This change will only increase the size of shared libraries which use the EXPORT_ALL_SYMBOLS option, which as of this PR are Atom_RPI.Public, Atom_RPI.Edit and Atom_RPI.TestUtils; but will in turn decrease the size of all other shared libraries and executables which link to these (potential future use of this option will of course also increase the size of more shared libraries, but also decrease the size of other libraries which depend on them).
The size of these shared libraries increases somewhat when exporting all symbols (The size of the static libraries from current development is way bigger than the dlls (eg. Atom_RPI.Public.lib is 124MB))
| Library | Plain DLL | Export macros | EXPORT_ALL_SYMBOLS |
|---|---|---|---|
| Atom_RPI.Public.dll | 42.9MB | 44.2MB | 47.3MB |
| Atom_RPI.Edit.dll | 19.7MB | 19.8MB | 22.3MB |
| Atom_RPI.TestUtils.dll | 36.5MB | x | 36.6MB |
The previous table does not include the size of the import libraries:
| Library | Export macros | EXPORT_ALL_SYMBOLS |
|---|---|---|
| Atom_RPI.Public.lib | 2.37MB | 13.8MB |
| Atom_RPI.Edit.lib | 0.3MB | 7.6MB |
On the other hand, the size of some other libraries decreases (random selection of shared libraries which link to Atom_RPI.Public):
| Library | Before change to RPI dll | After change to RPI dll |
|---|---|---|
| EditorLib.dll | 91.5MB | 83.4MB |
| Atom_Feature_Common.Editor.dll | 50.3MB | 42.1MB |
| Multiplayer.Editor.dll | 77.2MB | 66.1MB |
For a complete picture of the build artifacts I looked at the size of each .dll and .lib file in the bin/profile and lib/profile folders with commit form before this PR and with this PR when building the "Editor" target with a clean build directory (Details: sharedlib-comparison.xlsx). The summary of the excel sheet is that the build artifacts (.dll and .lib files) are ~596MB smaller (-4.9%) than before.
The reason for using the EXPORT_ALL_SYMBOLS version instead of export macros is mostly for convenience, since I also tried adding an export macro to all public classes, but there were some problems:
- If a base class of an exported class is itself not exported, this triggers a warning, and this warning needs to be silenced with the
AZ_PUSH_DISABLE_DLL_EXPORT_BASECLASS_WARNINGand ocasionallyAZ_PUSH_DISABLE_DLL_EXPORT_MEMBER_WARNINGmacros, or more globally for the entire library; or all baseclasses need to be converted to have a dll interface themselves, which is another big undertaking - While I could successfully build and link the
Atom_RPI.PublicandAtom_RPI.Editlibraries with export macros, I was not able to fix the linker errors that showed up when dependent targets try to link to these libraries in a reasonable time (errors likelld-link: error: public: void __cdecl AZ::Internal::NonIdHandler<class AZ::SystemTickEvents, class AZ::SystemTickEvents, struct AZ::Internal::EBusContainer<class AZ::SystemTickEvents, class AZ::SystemTickEvents, 0, 1>>::BusConnect(void) was replaced, which point to files in AzCore, so not sure how to go about this). This can probably be fixed with more effort, but I didnt want to invest even more time in the "perfect" solution if the "good enough" solution worked immediately. - The sizes of the dlls with export macros were
Atom_RPI.Public44.2MB andAtom_RPI.Edit19.8MB, so only a bit larger than the dlls without exporting any symbold; but since I did not manage to link to these libraries successfully, these numbers should be taken with a grain of salt.
|
There are currently four failing test sets in the Linux CI:
All test will run successfully after PR18362 , as shown in the CI run of PR18364 (minus the flaky |
Signed-off-by: Markus Prettner <[email protected]>
Signed-off-by: Markus Prettner <[email protected]>
…ing test execution Signed-off-by: Markus Prettner <[email protected]>
mprettner-huawei
force-pushed
the
Atom_RPI-cmake-sharedlib
branch
from
a05d297 to
61eb258
Compare
|
@jhmueller-huawei , this commit must be reverted because We can't build Atom RPI in Debug configuration : #18409 |
What does this PR do?
This PR removes static libraries from the Atom_RPI part of the Atom gem and uses object and shared libraries instead.
This has the following reasons:
Downsides of this change:
SCENE_CORE_APImacro), but this would need to be added to all classes in all 225 include files, so I chose to do the following:EXPORT_ALL_SYMBOLStoly_add_targetWINDOWS_EXPORT_ALL_SYMBOLSCMake option-fvisibility=defaultcompile optionWINDOWS_EXPORT_ALL_SYMBOLSoption does not export static variables which are initialized in the cpp file. This would only be possible with an export/import macro, but for almost all cases this was circumvented by moving the initialization to the header file (mostly asset name/group/extension declarations, bot some other declarations as well). The only case where this was not possible is accessingRPI::Validation::s_isEnabledin a test fixture, there I had to add a new function.If this PR is accepted I will also look at the other Atom libraries and potentially also other Gems and do the same refactoring there.
How was this PR tested?
Compile the engine with unity and nounity option, run the editor with a small test level, test some ASV samples