ATLASSIM-3150: Enable use of static Geant4 in Athena using "BigLibrary" pattern to improve simulation performance

mentioned in merge request atlasexternals!921 (merged)

added 403 commits

1bc56a15...84c223e7 - 379 commits from branch atlas:master
96751a7e - Hacky demo of creating a "big library" for simulation
c8f31598 - Single list for grouped packages
bc5aaaf3 - Add ISF_Geant4Event to BigSimulation
ffe98519 - Merge LarG4Code with BigSimulation
220c2621 - Add MCTruthBaseLib to BigSimulation
524ab1f7 - Merge LArG4ShowerLib into BigSimulation
229ab67e - Merge G4AtlasAlg into BigSimulation
cbb350b2 - Merge TileG4Interfaces into BigSimulation
849c3681 - Merge G4AtlasInterfaces into BigSimulation
ec5b5e15 - Note order of object libs and how linking must be done
c4e6254c - Add "generation 5" Geant4 using packages to BigSimulation
641c5a47 - Move gen6 libraries to BigSimulation
ae1dae87 - Add Athena-only packages to BigSimulation
2da294c0 - First attempt at merging Athena components
83ca8aa6 - Merge all Geant4-using AthSimulation components
5eee0f06 - Get tools from main config manager
3f6a0c1b - Merge all Geant4-using Athena components
d35b474d - Remove InDetSimEvent's Geant4 dependency
a8b73844 - Create BigSimulation from all library and component objects
5f76d7cf - Exclude export of all static lib symbols from BigSimulation
7a6c004c - Merge ActsGeantFollower into BigSimulation
1fcd26f1 - Get tools in BigSimulation from main config manager
29260483 - Link Geant4 using tests to _obj dependencies
4d8ee4e0 - Rename BigSimulation to AtlasGeant4 to address reviewer comments

This latest update is just a rebase from last week plus changing the "BigSimulation" package/targets to "AtlasGeant4" (we can bikeshed this further if it's not clear enough!).

I've also started looking at the last issue of dictionaries, using @akraszna's comments and suggestions in ATLASIM-3150:

I've confirmed that the simplification to LArG4CodeEnums in LArCalorimeter/LArG4/LArG4Code to a simple dictionary that doesn't need linking to anything else is fine. @jchapman, @akraszna I think this can be submitted as a separate MR as it's independent of the big library stuff? Let me know and will prepare that.

On the remaining two, I think that for LArG4GenShowerLib could be decoupled entirely if the StepInfo class is made header only/inline? At least the implementation in the cxx file seems simple enough to support this. The dictionary would then become like LArG4CodeEnums.

The LArWheelSolidCheckerDict can't be simplified so easily, but leaving it as-is would mean just changing:

atlas_add_dictionary( LArWheelSolidCheckerDict
                      src/LArWheelSolidDDProxy.h
                      src/lcg_dict/selection.xml
                      LINK_LIBRARIES Geo2G4Lib )

to

atlas_add_dictionary( LArWheelSolidCheckerDict
                      src/LArWheelSolidDDProxy.h
                      src/lcg_dict/selection.xml
                      LINK_LIBRARIES AtlasGeant4Lib )

per @akraszna's last comment in ATLASSIM-3150.

My only concern here is if there are any use cases where AtlasGeant4 and LarWheelSolidCheckerDict would be loaded in the same process (whether Python or ROOT). That would likely cause issues due to the nature of AtlasGeant4 being the same set of object files as AtlasGeant4Lib, adding component objects on top of this - i.e. duplicate symbol issues.

Having said that, this setup is being run currently in this branch (because Geo2G4Lib is an ALIAS to AtlasGeant4Lib) and all tests and the Sim_tf transform functions, so these parts are fine. Doesn't mean there aren't other cases where it may not though!

Thoughts welcome on all of the above!

Hi @bmorgan,

Tagging @asoukhar to comment on how LArWheelSolidCheckerDict is used. I think it is only used in very specific test jobs though.

I think go ahead with the separate MR to simplify LArG4CodeEnums. Tagging @pavol in case he wants to comment on that part.

Cheers,

John

mentioned in merge request !50287 (merged)

Done on the MR for LArG4CodeEnums (!50287 (merged)).

I've also confirmed that inlining StepInfo.h in LArG4GenShowerLib, reducing its link requirement to CLHEP and AthContainers, compiles and passes tests without error - so can also submit a MR for this it that's an acceptable change.

mentioned in merge request !50326 (merged)

Just as a note to myself, once !50326 (merged) is merged, the removal of the POSITION_INDEPENDENT_CODE setting on the OBJECT libraries can go ahead. I'll do this in two stages to check:

Remove the target property, check
Remove the _obj target suffixes

I'm a bit late reacting...

Defining LArWheelSolidCheckerDict "on top of" AtlasGeant4Lib is not all too sinister in my mind. All symbols are still only held in a single, shared library, in AtlasGeant4Lib. So jobs that need both AtlasGeant4 for its algorithm/tool/service factories and LArWheelSolidCheckerDict for its dictionaries, should work just fine. All symbol lookups will go back to AtlasGeant4Lib in memory.

It does mean that if a job would want to use a type from LArWheelSolidCheckerDict in PyROOT, without using anything else from the simulation, it would still have to load all of AtlasGeant4Lib. But I don't think this will be a big use-case, will it?

I'm on board with making StepInfo header-only, as it should simplify things a bit.

Defining LArWheelSolidCheckerDict "on top of" AtlasGeant4Lib is not all too sinister in my mind. All symbols are still only held in a single, shared library, in AtlasGeant4Lib. So jobs that need both AtlasGeant4 for its algorithm/tool/service factories and LArWheelSolidCheckerDict for its dictionaries, should work just fine. All symbol lookups will go back to AtlasGeant4Lib in memory.

Unfortunately as implemented here, the build isn't structured as AtlasGeant4.so -> links to -> AtlasGeant4Lib.so . That's down to the "link only once to Geant4 static libs and don't export their symbols" pattern. That lead to building things as:

AtlasGeant4Lib.so = All objects from athena_add_libraryies that use Geant4 + Geant4 static objects
AtlasGeant4.so = All objects from athena_add_libraryies that use Geant4 + All objects from Athena_add_components that use Geant4 + Geant4 static objects

i.e. AtlasGeant4.so is a "superset" of AtlasGeant4Lib.so.

It's very likely there's a better way to structure the AtlasGeant4 component, what I wasn't sure about was sticking component objects into a linkable library. I.e. AtlasGeant4Lib.so could contain everything, and AtlasGeant4.so would be an empty shell linking to that?

I'm on board with making StepInfo header-only, as it should simplify things a bit.

O.k., I'll prep a separate MR with this change and we can check with the package maintainers there.

Hmm... Then we should change that...

I think the safest would be if "all the symbols" would be built into AtlasGeant4Lib, and AtlasGeant4 would pretty much just be a "component library" on top of AtlasGeant4Lib. Similar to things like:

https://gitlab.cern.ch/atlas/athena/-/blob/master/Reconstruction/Jet/JetRec/CMakeLists.txt#L25-37

(It's not often that I would use Jets as an example... )

Duplicating some symbols between AtlasGeant4Lib and AtlasGeant4 is definitely not a good idea. Since many symbols absolutely need to be in AtlasGeant4Lib so that outside users could use them, the only choice is to move every other ("non-technical", non-Gaudi-related) symbol from AtlasGeant4 into AtlasGeant4Lib as well.

Note that building the src/components/*.cxx files from all of the packages into AtlasGeant4Lib, while a bit counter-intuitive, should work fine. Though it would probably simplify things to move all of the object files from those sources just into AtlasGeant4Lib. But on first order you shouldn't even need that. Just link all object libraries into AtlasGeant4Lib (using atlas_add_library(...)), and then link AtlasGeant4 against AtlasGeant4Lib (using atlas_add_component(...)).

O.k., just to converge on this area, does the following look o.k. (not tried in anger yet...) ?

# ?? So AtlasGeant4 actually ends up linked to AtlasGeant4Lib ??
atlas_disable_as_needed()

# Single *library* comprising all library and component objects
atlas_add_library( AtlasGeant4Lib
                   # dummy file needed to make the function happy, does not seem to like
                   # $<TARGET_OBJECTS:...>. Linking seems to do sufficient job of adding the .o
                   dummy.cc
                   SHARED
                   NO_PUBLIC_HEADERS
                   # I.e. library contains both lib and component objects
                   LINK_LIBRARIES "-Wl,--exclude-libs,ALL" ${AtlasGeant4Component_TARGET_OBJECTS} ${AtlasGeant4_TARGET_OBJECTS})

# ALIAS trick retained so package-level links work
foreach(__object_target ${AtlasGeant4_TARGET_OBJECTS})
  string(REGEX REPLACE "_obj$" "" __alias_target_name "${__object_target}")
  add_library(${__alias_target_name} ALIAS AtlasGeant4Lib)
endforeach()

# Athena component as a lightweight layer that's really AtlasGeant4Lib by another name
atlas_add_component( AtlasGeant4
                     dummy.cc
                     LINK_LIBRARIES AtlasGeant4Lib)

Yes, this is what I had in mind... Though I also didn't try it for real...

Thanks! Will try it out asap...

Unfortunately, the unified library outlined above doesn't work as written... There are two issues seen. First, in the build itself, there's a slew of warnings when generating the component:

[60/65] Generating AtlasGeant4_clid.db
JobOptionsSvc        INFO Job options successfully read in from CLIDComps/minimalPrintout.opts
genCLIDDB            INFO Writing clid.db for package AtlasGeant4 to /build/bmorgan/athena-builds/master-athsim-3150/build/Simulation/AtlasGeant4/AtlasGeant4_clid.db.
[61/65] Generating ../../x86_64-centos7-gcc8-opt/python/AtlasGeant4/...gcc8-opt/lib/libAtlasGeant4.confdb, genConf/AtlasGeant4.confdb2_part
[warning] library [AtlasGeant4] exposes factory [AddPhysicsDecayTool] which is declared in [libAtlasGeant4Lib.so] !!
[warning] library [AtlasGeant4] exposes factory [BCMSensorSDTool] which is declared in [libAtlasGeant4Lib.so] !!
[warning] library [AtlasGeant4] exposes factory [BLMSensorSDTool] which is declared in [libAtlasGeant4Lib.so] !!
[warning] library [AtlasGeant4] exposes factory [BoxEnvelope] which is declared in [libAtlasGeant4Lib.so] !!
...
[warning] library [AtlasGeant4] exposes factory [UserLimitsSvc] which is declared in [libAtlasGeant4Lib.so] !!
[warning] library [AtlasGeant4] exposes factory [VoxelDensityTool] which is declared in [libAtlasGeant4Lib.so] !!
[warning] library [AtlasGeant4] exposes factory [iGeant4::G4LegacyTransportTool] which is declared in [libAtlasGeant4Lib.so] !!
[warning] library [AtlasGeant4] exposes factory [iGeant4::G4RunManagerHelper] which is declared in [libAtlasGeant4Lib.so] !!
[warning] library [AtlasGeant4] exposes factory [iGeant4::G4TransportTool] which is declared in [libAtlasGeant4Lib.so] !!
[warning] library [AtlasGeant4] exposes factory [iGeant4::Geant4SimSvc] which is declared in [libAtlasGeant4Lib.so] !!
[warning] library [AtlasGeant4] exposes factory [sTGCSensitiveDetectorTool] which is declared in [libAtlasGeant4Lib.so] !!
[65/65] Built /build/bmorgan/athena-builds/master-athsim-3150/build/x86_64-centos7-gcc8-opt/lib/AthSimulation.components

Guess that's a consequence of putting the component objects into the library. The build is successful though.

However, many tests no longer pass:

The following tests FAILED:
	200 - CxxUtils_procmaps_test_ctest (Failed)
	506 - BCM_G4_SD_BCM_G4_SDToolConfig_test_ctest (Failed)
	508 - BLM_G4_SD_BLM_G4_SDToolConfig_test_ctest (Failed)
	510 - PixelG4_SD_PixelG4_SDToolConfig_test_ctest (Failed)
	513 - SCT_G4_SD_SCT_G4_SDToolConfig_test_ctest (Failed)
	517 - TRT_G4_SD_TRT_G4_SDToolConfig_test_ctest (Failed)
	528 - LArG4SD_LArG4SDToolConfig_test_ctest (Failed)
	565 - G4AtlasApps_test_AtlasG4_tf_configuration_ctest (Failed)
	566 - G4AtlasServices_G4AtlasFieldServices_test_ctest (Failed)
	567 - G4AtlasServices_G4AtlasServicesConfig_test_ctest (Failed)
	568 - G4AtlasTools_G4GeometryToolConfig_Simtest_ctest (Failed)
	569 - G4AtlasTools_G4PhysicsRegionConfig_test_ctest (Failed)
	570 - G4AtlasTools_G4FieldConfig_test_ctest (Failed)
	571 - G4AtlasTools_G4AtlasToolsConfigNew_test_ctest (Failed)
	574 - ISF_Config_test_FullG4_Sim_tf_configuration_ctest (Failed)
	581 - ISF_Services_ISF_ServicesConfigNew_test_ctest (Failed)

Ignoring the 200 test as that fails for me on master as well, the remainder are all due to missing components, e.g.

3/17 Test #506: BCM_G4_SD_BCM_G4_SDToolConfig_test_ctest ............***Failed    2.75 sec
...
Traceback (most recent call last):
  File "/build/bmorgan/athena.git/InnerDetector/InDetG4/BCM_G4_SD/test/BCM_G4_SDToolConfig_test.py", line 31, in <module>
    cfg = BCMSensorSDCfg(ConfigFlags)
  File "/build/bmorgan/athena-builds/master-athsim-3150/build/x86_64-centos7-gcc8-opt/python/BCM_G4_SD/BCM_G4_SDToolConfig.py", line 24, in BCMSensorSDCfg
    result.setPrivateTools(CompFactory.BCMSensorSDTool(name, **kwargs))
  File "/build/bmorgan/athena-builds/master-athsim-3150/build/x86_64-centos7-gcc8-opt/python/AthenaConfiguration/ComponentFactory.py", line 64, in __getattr__
    return getattr(self._getFactory(),cfgName)
  File "/build/bmorgan/athena-builds/master-athsim-3150/build/x86_64-centos7-gcc8-opt/python/AthenaConfiguration/ComponentFactory.py", line 42, in __getattr__
    return getattr(_cfgs,cfgName)
  File "/build/bmorgan/athena-builds/master-athsim-3150/externals/install/AthSimulationExternals/22.0.55/InstallArea/x86_64-centos7-gcc8-opt/python/GaudiConfig2/_db.py", line 200, in __getattr__
    raise AttributeError(
AttributeError: module 'GaudiConfig2.Configurables' has no attribute 'BCMSensorSDTool'

Given that the current branch works fine, these seem certainly due to the "transparent" AtlasGeant4 actually being rather opaque as far as look up of components etc.

I did try a dirty hack which was:

Build AtlasGeant4Lib incorporating all library and component objects as here, adding a link to Gaudi::GaudiPluginService
Rather than atlas_add_component(AtlasGeant4 ...), I copy-pasted in the steps in AtlasLibraryFunctions.cmake after library/module creation, using AtlasGeant4Lib as the target these are run over.

That makes all tests pass, so clearly all objects have to be present in the library for the component generation step to work?

So the straight shared library seems to work o.k. in a dual role of linkable library and plugin/component, but as noted, this under a very hacky setup and might not cover all cases/policies.

However, I can't see another way to have both a linkable shared library and plugin module... Or at least have AtlasGeant4 as a plugin built as a pure shared library that can, but really shouldn't be linked to unless you know what you're doing.

added 758 commits

4d8ee4e0...9bf69138 - 734 commits from branch atlas:master
83fe7cf4 - Hacky demo of creating a "big library" for simulation
f24f2220 - Single list for grouped packages
8fb3f52e - Add ISF_Geant4Event to BigSimulation
72cc229a - Merge LarG4Code with BigSimulation
14a32692 - Add MCTruthBaseLib to BigSimulation
3a009bb9 - Merge LArG4ShowerLib into BigSimulation
d9596819 - Merge G4AtlasAlg into BigSimulation
e6b4fa45 - Merge TileG4Interfaces into BigSimulation
563d1033 - Merge G4AtlasInterfaces into BigSimulation
7ca71058 - Note order of object libs and how linking must be done
56396e39 - Add "generation 5" Geant4 using packages to BigSimulation
868cecbe - Move gen6 libraries to BigSimulation
0d6d7f72 - Add Athena-only packages to BigSimulation
8cd9a7f4 - First attempt at merging Athena components
180007f0 - Merge all Geant4-using AthSimulation components
94f5bd23 - Get tools from main config manager
e16b3822 - Merge all Geant4-using Athena components
6ff657c0 - Remove InDetSimEvent's Geant4 dependency
1e9b5486 - Create BigSimulation from all library and component objects
fd52491c - Exclude export of all static lib symbols from BigSimulation
62899644 - Merge ActsGeantFollower into BigSimulation
4246f123 - Get tools in BigSimulation from main config manager
17ccabf1 - Link Geant4 using tests to _obj dependencies
a7900bd8 - Rename BigSimulation to AtlasGeant4 to address reviewer comments

Compare with previous version

added 1 commit

1403c3fa - Remove explicit PIC target property from AtlasGeant4 OBJECTs

Compare with previous version

mentioned in merge request !50470 (merged)

With the update to OBJECT library support in atlasexternals, the explicit PIC settings have now been removed and all's working well. I've therefore gone back to looking at the last comment by @akraszna on removing the _obj suffix from the OBJECT libs and the aliasing of these to AtlasGeant4Lib.

The first step was simply to remove the ALIAS targets as a test of where these are used. There are in fact only a handful of places:

Packages that end up in AtlasGeant4Lib and that have atlas_add_test calls that build executable tests:
- ForwardDetectors/ALFA/ALFA_G4_SD/CMakeLists.txt
- ForwardDetectors/LUCID/LUCID_G4_SD/CMakeLists.txt
- ForwardDetectors/ZDC/ZDC_SD/CMakeLists.txt
- InnerDetector/InDetG4/TRT_G4Utilities/CMakeLists.txt
- Simulation/ISF/ISF_Core/ISF_Services/CMakeLists.txt
The already known case of Geo2G4 where the LArWheelSolidCheckerDict links to Geo2G4 and thus AtlasGeant4
Three corner cases of packages that link to a package in AtlasGeant4 but don't end up linked to it (by not using it at all, or it getting dropped by --as-needed):
- A couple in Acts that are cases of overlinking, addressed in !50470 (merged).
- ForwardTransportFast links to ForwardTransportSvcLib
- TileFastCaloSim links to TileG4InterfacesLib

If we want to get rid of the _obj suffix and ALIAS targets then I think these can be addressed in a couple of ways - so input/advice welcome:

I can see two ways to address the atlas_add_test cases:
- Link directly to the OBJECT target being tested, e.g.
```
atlas_add_library( ALFA_G4_SDLib OBJECT ...)
              
atlas_add_test(  ALFA_SensitiveDetector_gtest ... LINK_LIBRARIES ALFA_G4_SDLib )
```
  This largely works except in the case that the OBJECT library links to another OBJECT library. CMake correctly passes down the usage requirements but not the objects from the used library. This leads to missing symbol errors, so an explicit link to the needed library is required. In the case above this means:
```
atlas_add_test( ALFA_SensitiveDetector_gtest ... LINK_LIBRARIES ALFA_G4_SDLib G4AtlasToolsLib )
```
- Link explicitly to the AtlasGeant4Lib end target. "Just works", but means a package has to know that it ends up in AtlasGeant4Lib. Maybe that's not critical though, and certainly the simpler option.
The case of Geo2G4/LArWheelSolidCheckerDict is fairly special - probably only clean way is to link directly to AtlasGeant4Lib?
These cases of "using by link is dropped by --as-needed" I'm not sure about. In both cases, the packages being linked to are INTERFACE targets that declare a public usage requirement on Geant4. Whilst ForwardTransportFast and TileFastCaloSim don't end up linked to Geant4, it feels like they should go into AtlasGeant4/Lib. They don't get a NEEDED entry for any Geant4 library or for AtlasGeant4Lib AFAICT by the use of --as-needed and that they don't use any Geant4/AtlasGeant4Lib symbol directly.

My gut feeling would be to put these in AtlasGeant4 as this would make this library fully self contained with no other packages linking to it, but arguments to the contrary very welcome!

Bottom line - we can remove the _obj suffix and the ALIAS targets, but we need to decide how to tackle the above points. Thankfully, these appear more policy than technical...

mentioned in merge request !50676 (merged)

Sorry for the bump @akraszna, but could you take a look at my comments above as soon as you have some time please? I think these are the last issues to be resolved with the "big library" work, but need input from the build experts!

Sorry for the delay...

Indeed, object libraries don't seem to behave quite as conveniently as "regular" libraries, even with the latest versions of CMake. As you found, when you have a setup like:

add_library( obj1Lib OBJECT source1.cxx )

add_library( obj2Lib OBJECT source2.cxx )
target_link_libraries( obj2Lib PUBLIC obj1Lib )

add_executable( myExe source3.cxx )
target_link_libraries( myExe PRIVATE obj2Lib )

, the myExe executable would only include the object file(s) from obj2Lib. (https://cmake.org/cmake/help/latest/command/target_link_libraries.html#linking-object-libraries)

Luckily the same page also has this description:

https://cmake.org/cmake/help/latest/command/target_link_libraries.html#linking-object-libraries-via-target-objects

I.e. as long as we swallow the pill that object libraries would have to know which of the libraries that they need are also object libraries, we should be able to write our code like:

atlas_add_library( DownstreamLib
   ...
   LINK_LIBRARIES $<TARGET_OBJECTS:UpstreamLib> ...
   ...
   )

atlas_add_test( TestForDownstreamLib
   SOURCES ...
   LINK_LIBRARIES <TARGET_OBJECTS:DownstreamLib> ... )

According to that description the object files will be "transitive" in such a setup. And as long as that is the case, I think linking the tests to the object libraries would indeed be better than to link them to AtlasGeant4Lib. (As a "failing build" would still be able to produce some useful tests in such a setup.)

For the dictionary issue I hope that !50676 (merged) works, as that should indeed be the simplest solution.

Finally, for the --as-needed flags... I'm not sure I understand the question there. We might want to just talk in some way to clarify what issue you're running into exactly.

For TileFastCaloSim I think the G4 dependency is only there due to the use of G4Types.hh in ITileCalculator.h
https://gitlab.cern.ch/atlas/athena/-/blob/master/TileCalorimeter/TileG4/TileG4Interfaces/TileG4Interfaces/ITileCalculator.h
Only one method of the ITileCalculator interface is used in the TileFCSmStepToTileHitVec algorithm:
https://gitlab.cern.ch/atlas/athena/-/blob/master/TileCalorimeter/TileFastCaloSim/src/TileFCSmStepToTileHitVec.cxx#L254
https://gitlab.cern.ch/atlas/athena/-/blob/master/TileCalorimeter/TileG4/TileG4Interfaces/TileG4Interfaces/ITileCalculator.h#L89
and that has no explicit G4 dependence. So we could get around this by splitting this method out into a separate G4-free interface class maybe??

From what I can tell the ForwardTransportFast Algorithm is intended to be used as an alternative to Geant4 simulation, so it once again looks like a case of loosely written interfaces pulling in more dependencies than necessary. In this case the use of G4ThreeVector is the culprit.

Both the ForwardTransportFast and ForwardTransportSvc packages (and the TileFastCaloSim package) are in need of updating.

Thanks both for the feedback! @akraszna, I'll try out the TARGET_OBJECTS link - assume we're o.k. with the CMake 3.21 requirement.

Finally, for the --as-needed flags... I'm not sure I understand the question there. We might want to just talk in some way to clarify what issue you're running into exactly.

Sorry, I didn't phrase it particularly clearly! It's not an issue per se, rather, the ForwardTransportFast and TileFastCaloSim declare a link to a package that ends up in AtlasGeant4Lib, e.g. in the case of the latter

atlas_add_component( TileFastCaloSim
                     src/*.cxx
                     src/components/*.cxx
                     INCLUDE_DIRS ${CLHEP_INCLUDE_DIRS}
                     LINK_LIBRARIES ${CLHEP_LIBRARIES} ... TileG4InterfacesLib ... )

Under the ALIAS scheme, TileG4InterfacesLib is AtlasGeant4Lib, so TileFastCaloSim would nominally end up linked to that. However, readelf -d libTileFastCaloSim.so shows that it has no NEEDED entry for libAtlasGeant4Lib.so (or any Geant4 library).

With @jchapman's comments above, I think the dependence is limited to typedefs or similar that mean there's no use of symbols from AtlasGeant4Lib or Geant4, and thus the --as-needed linker flag removes these from the actual link.

So in terms of this MR I don't think it's a big issue - the packages clearly need a tidy up, but as they don't actually end up linked to AtlasGeant4Lib at present, it would not cause duplicate symbol issues or similar.

I think we're all on the same page then. Let's just let --as-needed remove those dependencies from the produced libraries.

The 3.21 dependency should indeed be okay, we've been using CMake 3.21.3 in the master build since quite a while. But to make things as nice as possible, you could add

cmake_minimum_required( VERSION 3.21 )

into the packages that will end up using this $<TARGET_OBJECTS:...> formalism. (Similar to Generators/Epos_i/CMakeLists.txt.) Just to make error messages very clear when somebody would try to use an older version of CMake. (We had that happen in private builds in the past.)

I've had a go with the $<TARGET_OBJECTS:..> linking suggested above by @akraszna, but I've not been able to get it working without introducing nasty side effects that aren't easily detectable. It may be easiest to have a quick meeting to go through this, as the following is a bit long winded...

The following is the simplest demo of the existing problem. Read A, B and C as Athena packages with D begin AtlasGeant4Lib. main and testCase are examples of consumers of this library or individual package, e.g. tests.

cmake_minimum_required(VERSION 3.21)
project(test-objects)
set(CMAKE_VERBOSE_MAKEFILE ON)
foreach(_f main a b c d)
  file(TOUCH ${PROJECT_SOURCE_DIR}/${_f}.cc)
endforeach()

# Base lib
add_library(A OBJECT a.cc)
target_compile_definitions(A PUBLIC A)

# First implementation lib
add_library(B OBJECT b.cc)
target_compile_definitions(B PUBLIC B)
target_link_libraries(B PUBLIC A)

# Second implementation lib (so we have a diamond dependency to A)
add_library(C OBJECT c.cc)
target_compile_definitions(C PUBLIC C)
target_link_libraries(C PUBLIC A)

# Use case: library composed of OBJECT libs (AtlasGeant4Lib)
add_library(D SHARED d.cc)
target_link_libraries(D PUBLIC A B C)

# Use case: another target that consumes the shared lib (could also be executable)
add_library(main SHARED main.cc)
target_link_libraries(main PUBLIC D)

# Use case: A test that consumes its local OBJECT lib
# PROBLEM: needed a.cc.o object not linked in
add_library(testCase SHARED main.cc)
target_link_libraries(testCase PUBLIC C)

The problem here is that testCase does not get a link to the a.cc.o object from A needed by C. We can use TARGET_OBJECTS here to change how C is linked:

target_link_libraries(C PUBLIC A INTERFACE $<TARGET_OBJECTS:A>)

This now correctly links testCase with a.cc.o, but... The D target is correctly composed of the objects from the other three libraries, but if we inspect how the main target is linked with D we see:

[ 77%] Linking CXX shared library libmain.dylib
c++ ... -o libmain.dylib ... CMakeFiles/A.dir/a.cc.o libD.dylib

i.e. main is linked to both D and a.cc.o, but that object is already in D... In other words, we get double linking/duplicate symbol issues, or worse, the same object can end up in more than one library without any build or link error being raised. This is essentially exercising the warning given in the CMake docs

I've not been able to find any combination of PUBLIC/PRIVATE/INTERFACE usage requirements that satisfy the requirements:

Linking an OBJECT library X into target Y should propagate its usage requirements, objects, and any upstream objects into compilation/linking of Y
Any target linking to Y should forward Xs usage requirements, but not add Xs objects to the link line

Not to say this isn't possible, but as even a PRIVATE link of $<TARGET_OBJECTS:...> will propagate this through to dependents (like for STATIC libs). It would almost certainly imply fiddling about with LINK_LIBRARIES properties and possibly double targets - one to propagate usage requirements, which is essentially the existing OBJECT target, and one to be "consumed". However, in the latter case, you still need to know which targets you are linking to are OBJECT libraries, so the $<TARGET_OBJECTS:> genex can be added for that target.

I'm not sure this would end up any less complicated than having executable tests that link to an OBJECT library from the same package needing to specify any further required OBJECT libs as LINK_LIBRARIES. It's a potential dependency hell issue so would be great if a solution is possible, but the advantage at the moment is that errors are raised immediately at build/link time for that package only, with zero side effects beyond that. There are also only a handful of packages that expose this issue:

ForwardDetectors/ALFA/ALFA_G4_SD
ForwardDetectors/LUCID/LUCID_G4_SD
ForwardDetectors/ZDC/ZDC_SD

and two that are marginal (have a test with a dependency on a OBJECT target, but that does not depend on any other).

InnerDetector/InDetG4/TRT_G4Utilities
Simulation/ISF/ISF_Core/ISF_Services

So whilst a full transparent solution should be investigated and implemented if possible, I'm not sure it's critical to have it in place for this MR.

Whew, that was long, sorry ... Happy to chat on Zoom at some point if that would be easier to go through any questions/further discussion.

added 492 commits

1403c3fa...1fecbfe2 - 465 commits from branch atlas:master
27ecdbc1 - Hacky demo of creating a "big library" for simulation
50a05f10 - Single list for grouped packages
51d9b9f1 - Add ISF_Geant4Event to BigSimulation
dfd1e06f - Merge LarG4Code with BigSimulation
24bca08a - Add MCTruthBaseLib to BigSimulation
887ca689 - Merge LArG4ShowerLib into BigSimulation
846b5e9c - Merge G4AtlasAlg into BigSimulation
224f6bf8 - Merge TileG4Interfaces into BigSimulation
d4834997 - Merge G4AtlasInterfaces into BigSimulation
44573061 - Note order of object libs and how linking must be done
7894e0c5 - Add "generation 5" Geant4 using packages to BigSimulation
032f2410 - Move gen6 libraries to BigSimulation
ee632d84 - Add Athena-only packages to BigSimulation
cd062a0f - First attempt at merging Athena components
9c847cd3 - Merge all Geant4-using AthSimulation components
90c4a836 - Get tools from main config manager
45905e7b - Merge all Geant4-using Athena components
debc6ce3 - Remove InDetSimEvent's Geant4 dependency
d7f82c31 - Create BigSimulation from all library and component objects
0107b2c7 - Exclude export of all static lib symbols from BigSimulation
5cbb44d2 - Merge ActsGeantFollower into BigSimulation
9759ec6b - Get tools in BigSimulation from main config manager
6518a595 - Link Geant4 using tests to _obj dependencies
f2661ae8 - Rename BigSimulation to AtlasGeant4 to address reviewer comments
096f0720 - Remove explicit PIC target property from AtlasGeant4 OBJECTs
e05bab27 - CMake cleanup
8e48ee25 - Remove all bar three ALIAS targets to AtlasGeant4Lib

Compare with previous version

added 159 commits

8e48ee25...70e257a9 - 131 commits from branch atlas:master
efb960ad - Hacky demo of creating a "big library" for simulation
b678df44 - Single list for grouped packages
fd206d63 - Add ISF_Geant4Event to BigSimulation
4c9a2ee6 - Merge LarG4Code with BigSimulation
86409324 - Add MCTruthBaseLib to BigSimulation
ff3f69a2 - Merge LArG4ShowerLib into BigSimulation
98a748f5 - Merge G4AtlasAlg into BigSimulation
8b3ddf6b - Merge TileG4Interfaces into BigSimulation
4c036a3f - Merge G4AtlasInterfaces into BigSimulation
d53085fd - Note order of object libs and how linking must be done
0dd0ff9a - Add "generation 5" Geant4 using packages to BigSimulation
3acfa8e7 - Move gen6 libraries to BigSimulation
973e3463 - Add Athena-only packages to BigSimulation
994e2b09 - First attempt at merging Athena components
8d775809 - Merge all Geant4-using AthSimulation components
0785e312 - Get tools from main config manager
8170a2e5 - Merge all Geant4-using Athena components
88aa6c0f - Remove InDetSimEvent's Geant4 dependency
e6a77174 - Create BigSimulation from all library and component objects
0f531bdb - Exclude export of all static lib symbols from BigSimulation
d046bc88 - Merge ActsGeantFollower into BigSimulation
c6ffb21c - Get tools in BigSimulation from main config manager
63a0ec02 - Link Geant4 using tests to _obj dependencies
c938131d - Rename BigSimulation to AtlasGeant4 to address reviewer comments
82aa2680 - Remove explicit PIC target property from AtlasGeant4 OBJECTs
0d6efa97 - CMake cleanup
2461f80f - Remove all bar three ALIAS targets to AtlasGeant4Lib
27c62c4b - Remove Geo2G4 alias to AtlasGeant4Lib

Compare with previous version

added 225 commits

27c62c4b...ff447c46 - 197 commits from branch atlas:master
dc5f37c4 - Hacky demo of creating a "big library" for simulation
d4cbdd72 - Single list for grouped packages
c318d566 - Add ISF_Geant4Event to BigSimulation
78371f7d - Merge LarG4Code with BigSimulation
55081962 - Add MCTruthBaseLib to BigSimulation
2dd096ff - Merge LArG4ShowerLib into BigSimulation
5970d4f2 - Merge G4AtlasAlg into BigSimulation
4f6491ab - Merge TileG4Interfaces into BigSimulation
53c6fa8c - Merge G4AtlasInterfaces into BigSimulation
ff0c1b1b - Note order of object libs and how linking must be done
0c978866 - Add "generation 5" Geant4 using packages to BigSimulation
0ae43fd3 - Move gen6 libraries to BigSimulation
adf964d6 - Add Athena-only packages to BigSimulation
aa827eb4 - First attempt at merging Athena components
77bd43b9 - Merge all Geant4-using AthSimulation components
611f5cc6 - Get tools from main config manager
1adbdb1c - Merge all Geant4-using Athena components
625c4a0f - Remove InDetSimEvent's Geant4 dependency
aeb685bc - Create BigSimulation from all library and component objects
1425e32a - Exclude export of all static lib symbols from BigSimulation
f9fb4de2 - Merge ActsGeantFollower into BigSimulation
94bab776 - Get tools in BigSimulation from main config manager
952140d3 - Link Geant4 using tests to _obj dependencies
d08c25f2 - Rename BigSimulation to AtlasGeant4 to address reviewer comments
51c41cda - Remove explicit PIC target property from AtlasGeant4 OBJECTs
89876ca2 - CMake cleanup
0ce05d42 - Remove all bar three ALIAS targets to AtlasGeant4Lib
4ad60bb4 - Remove Geo2G4 alias to AtlasGeant4Lib

Compare with previous version

Just another rebase update to check all is fine. With the dictionary removal, I think the last step is to decide what to do with the _obj naming and linking issues mentioned above. My feeling is to:

Remove the _obj naming suffix
- Would require the remaining "link but not use" ForwardTransportSvcLib and TileG4InterfacesLib to explicitly link to AtlasGeant4Lib (to avoid pulling in objects), but that may be a good marker in to remind to fix.
I don't think there's a clean fix for the binary executable tests that need to link to OBJECT libs right now
- Requiring these to link to all needed OBJECT libs is awkward, but at least for now limited to 2 OBJECT libs and a handful of packages.
- My feeling is to accept this for now and document the issue/requirement in the CMake scripts for those packages. If we come up with a full fix, it can then easily be applied.
Will need to be careful going forward about packages that aren't composed into AtlasGeant4 linking to the OBJECT libraries to avoid duplicate symbols.
- I don't have a way to check this automatically off the top of my header, but perhaps the DOT/graphviz output of dependencies could be used.

What do you think @akraszna, @jchapman ?

added 313 commits

4ad60bb4...c66f1482 - 284 commits from branch atlas:master
42e04e36 - Hacky demo of creating a "big library" for simulation
f060be29 - Single list for grouped packages
f37d3eb8 - Add ISF_Geant4Event to BigSimulation
358af212 - Merge LarG4Code with BigSimulation
8eef881a - Add MCTruthBaseLib to BigSimulation
0d0a0fbe - Merge LArG4ShowerLib into BigSimulation
55d754a2 - Merge G4AtlasAlg into BigSimulation
3fde66d0 - Merge TileG4Interfaces into BigSimulation
9cb19a68 - Merge G4AtlasInterfaces into BigSimulation
63a47dd0 - Note order of object libs and how linking must be done
fe6d37c7 - Add "generation 5" Geant4 using packages to BigSimulation
913bc373 - Move gen6 libraries to BigSimulation
63f160c6 - Add Athena-only packages to BigSimulation
0bd6cb55 - First attempt at merging Athena components
0f6bf7b5 - Merge all Geant4-using AthSimulation components
c3e98d22 - Get tools from main config manager
1f4977b8 - Merge all Geant4-using Athena components
418cc8ee - Remove InDetSimEvent's Geant4 dependency
aa808d3e - Create BigSimulation from all library and component objects
4d5403fc - Exclude export of all static lib symbols from BigSimulation
ac5bc0b9 - Merge ActsGeantFollower into BigSimulation
01d7877e - Get tools in BigSimulation from main config manager
4c5a1476 - Link Geant4 using tests to _obj dependencies
5f50206e - Rename BigSimulation to AtlasGeant4 to address reviewer comments
5b1faa26 - Remove explicit PIC target property from AtlasGeant4 OBJECTs
56875c2e - CMake cleanup
f681be29 - Remove all bar three ALIAS targets to AtlasGeant4Lib
5c3320e1 - Remove Geo2G4 alias to AtlasGeant4Lib
8fd5dc5e - Remove AtlasGeant4 aliases and _obj target suffixes

Compare with previous version

Hi @akraszna, I've now pushed/tested the removal of the ALIAS targets and _obj suffixes of targets. I think therefore unless you have other fixes you or @jchapman and @mbandier want me to make, this is ready to move out of Draft status...

Hi @akraszna (cc @jchapman, @mbandier), just a gentle bump on this for your feedback on the previous points. I think we're on the last steps, so would be good to update.

FYI @bmorgan, this MR may pick up a conflict after !51725 (merged) is merged, but it should just be a matter of dropping the changes for Simulation/ISF/ISF_Geant4/ISF_Geant4UserActions/CMakeLists.txt to resolve it.

added 1460 commits

8fd5dc5e...b597b0f8 - 1431 commits from branch atlas:master
45150bee - Hacky demo of creating a "big library" for simulation
f024c0ec - Single list for grouped packages
27a51171 - Add ISF_Geant4Event to BigSimulation
06a15634 - Merge LarG4Code with BigSimulation
b3cdafdd - Add MCTruthBaseLib to BigSimulation
d0dd4e15 - Merge LArG4ShowerLib into BigSimulation
127effa8 - Merge G4AtlasAlg into BigSimulation
e443eb42 - Merge TileG4Interfaces into BigSimulation
72f9ad6f - Merge G4AtlasInterfaces into BigSimulation
79eb6207 - Note order of object libs and how linking must be done
342608db - Add "generation 5" Geant4 using packages to BigSimulation
3b048430 - Move gen6 libraries to BigSimulation
41e13fca - Add Athena-only packages to BigSimulation
92989d03 - First attempt at merging Athena components
0dc02fde - Merge all Geant4-using AthSimulation components
b857e57a - Get tools from main config manager
178e8bd8 - Merge all Geant4-using Athena components
8c33f5cf - Remove InDetSimEvent's Geant4 dependency
e7409810 - Create BigSimulation from all library and component objects
c946e775 - Exclude export of all static lib symbols from BigSimulation
722543b5 - Merge ActsGeantFollower into BigSimulation
f2e8d456 - Get tools in BigSimulation from main config manager
58e08e1e - Link Geant4 using tests to _obj dependencies
2ff3018a - Rename BigSimulation to AtlasGeant4 to address reviewer comments
ab86f930 - Remove explicit PIC target property from AtlasGeant4 OBJECTs
ea61e3eb - CMake cleanup
4bd6bfa7 - Remove all bar three ALIAS targets to AtlasGeant4Lib
56ed8610 - Remove Geo2G4 alias to AtlasGeant4Lib
3f47090f - Remove AtlasGeant4 aliases and _obj target suffixes

Compare with previous version

Thanks for the heads up @jchapman - I've rebased to current master and will redo this after !51725 (merged) is merged. I'll then remove the Draft status!

added 352 commits

3f47090f...56edc7e4 - 323 commits from branch atlas:master
d094f262 - Hacky demo of creating a "big library" for simulation
1bbee74e - Single list for grouped packages
c0dd9792 - Add ISF_Geant4Event to BigSimulation
77f7edcf - Merge LarG4Code with BigSimulation
fbcba883 - Add MCTruthBaseLib to BigSimulation
06afcbb8 - Merge LArG4ShowerLib into BigSimulation
f1cc569d - Merge G4AtlasAlg into BigSimulation
0585213d - Merge TileG4Interfaces into BigSimulation
ea69d742 - Merge G4AtlasInterfaces into BigSimulation
85cbe22f - Note order of object libs and how linking must be done
3bd1d530 - Add "generation 5" Geant4 using packages to BigSimulation
ecdf99d8 - Move gen6 libraries to BigSimulation
55353474 - Add Athena-only packages to BigSimulation
742d9316 - First attempt at merging Athena components
51b85718 - Merge all Geant4-using AthSimulation components
bd846231 - Get tools from main config manager
b39f7e2b - Merge all Geant4-using Athena components
9954a17e - Remove InDetSimEvent's Geant4 dependency
f63dc8c8 - Create BigSimulation from all library and component objects
9946593a - Exclude export of all static lib symbols from BigSimulation
246ce9e1 - Merge ActsGeantFollower into BigSimulation
c857c30c - Get tools in BigSimulation from main config manager
2488c3a2 - Link Geant4 using tests to _obj dependencies
f1f2bd3e - Rename BigSimulation to AtlasGeant4 to address reviewer comments
dcfbb6ed - Remove explicit PIC target property from AtlasGeant4 OBJECTs
28917173 - CMake cleanup
350e675e - Remove all bar three ALIAS targets to AtlasGeant4Lib
c216650e - Remove Geo2G4 alias to AtlasGeant4Lib
c4c147e4 - Remove AtlasGeant4 aliases and _obj target suffixes

Compare with previous version

Rebased following merge of !51725 (merged), so think this is ready for a first round of non-draft review.

marked this merge request as ready

This merge request affects 88 packages. Since this is a long list, it will not be printed.

Adding @jchapman ,@cgrefe ,@rosati ,@dshope ,@mfauccig ,@cohm ,@stavrop ,@battagl ,@xiaozhon ,@solodkov ,@jojungge ,@akraszna ,@stsuno ,@egodden ,@cvarni ,@schaarsc ,@ggach ,@goetz ,@amorley ,@pjacka ,@wleight ,@pavol ,@zmarshal ,@sroe ,@harkusha ,@pagessin ,@calfayan ,@ahasib ,@tcuhadar ,@lshan ,@tadej as watchers

added ACTS Build Calorimeter ForwardDetectors HGTD InnerDetector JetEtmiss LAr Magnets MuonSpectrometer Simulation TestBeam Tile Tracking master review-pending-level-1 labels

CI Result SUCCESS (hash c4c147e4)

	Athena	AthSimulation	AthGeneration	AnalysisBase	AthAnalysis	DetCommon
externals
cmake
make
required tests
optional tests

Full details available on this CI monitor view. Check the JIRA CI status board for known problems
Athena: number of compilation errors 0, warnings 0
AthSimulation: number of compilation errors 0, warnings 0
AthGeneration: number of compilation errors 0, warnings 0
AnalysisBase: number of compilation errors 0, warnings 0
AthAnalysis: number of compilation errors 0, warnings 0
DetCommon: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 49784]

OK, looks good to me.

Will ask @akraszna to do the expert sign-off.

Also should we make a full-build just in case?

removed review-pending-level-1 label

added review-pending-expert label

ATLASSIM-3150: Enable use of static Geant4 in Athena using "BigLibrary" pattern to improve simulation performance

Merged by Vakhtang Tsulaia 3 years ago (Apr 14, 2022 5:01pm UTC) 3 years ago

Activity

ATLASSIM-3150: Enable use of static Geant4 in Athena using "BigLibrary" pattern to improve simulation performance

Merge request reports

Merged by Vakhtang Tsulaia 3 years ago (Apr 14, 2022 5:01pm UTC) 3 years ago

Activity