Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace tbb with std containers #723

Conversation

ianna
Copy link
Contributor

@ianna ianna commented Oct 13, 2020

BEGINRELEASENOTES

  • SpecParRegistry: remove need for TBB

    • TBB has a slight overhead compare to std containers
    • Using std containers allows to return references to the containers in the registry
  • Reverse the plan to process XML files in parallel

ENDRELEASENOTES

@ianna
Copy link
Contributor Author

ianna commented Oct 13, 2020

@MarkusFrankATcernch - CMS XML description would need quite a bit of work to make it modular.

@andresailer
Copy link
Member

That means also dropping the TBB requirements for these source files, right?

@ianna
Copy link
Contributor Author

ianna commented Oct 13, 2020

That means also dropping the TBB requirements for these source files, right?

Please, do :-)

@ianna
Copy link
Contributor Author

ianna commented Oct 13, 2020

Hmm...

217 - t_LHeD_DDG4_LHeDACLick_as_ACLick_LONGTEST (Child aborted)

@MarkusFrankATcernch
Copy link
Contributor

217 - t_LHeD_DDG4_LHeDACLick_as_ACLick_LONGTEST (Child aborted)

This failing test is strange. It is entirely unrelated to tbb.

@ianna
Copy link
Contributor Author

ianna commented Oct 14, 2020

@MarkusFrankATcernch - sorry I missed the meeting where I was planning to bring this up for discussion. Would it be possible to integrate this asap? Thanks!

@MarkusFrankATcernch
Copy link
Contributor

@ianna For the time being your are the only customer of this feature. I do not see any point of not merging.
Have you removed the TBB requirements for these source files ?

@ianna
Copy link
Contributor Author

ianna commented Oct 14, 2020

@ianna For the time being your are the only customer of this feature. I do not see any point of not merging.
Have you removed the TBB requirements for these source files ?

no, I have not. I'm not sure I can test this change since I'm using scram and pkgtools to build it...

@andresailer
Copy link
Member

You just need

diff --git a/DDCore/CMakeLists.txt b/DDCore/CMakeLists.txt
index 6a4f9b73..a3ded85e 100644
--- a/DDCore/CMakeLists.txt
+++ b/DDCore/CMakeLists.txt
@@ -15,7 +15,7 @@ dd4hep_print("|++> Will be Building DDCore")

 file(GLOB DDCore_SOURCES src/*.cpp src/segmentations/*.cpp src/XML/*.cpp)

-if(NOT DD4HEP_USE_TBB OR ${CMAKE_CXX_STANDARD} LESS 17)
+if(${CMAKE_CXX_STANDARD} LESS 17)
   list(APPEND EXCLUDE_HEADER include/DD4hep/SpecParRegistry.h)
   list(APPEND EXCLUDE_HEADER include/DD4hep/Filter.h)
   list(FILTER DDCore_SOURCES EXCLUDE REGEX Filter\.cpp|SpecParRegistry\.cpp )

And set DD4HEP_USE_TBB=OFF in the CMS build config

@ianna
Copy link
Contributor Author

ianna commented Oct 14, 2020

@andresailer - thanks! I'll update the PR.

@MarkusFrankATcernch MarkusFrankATcernch merged commit 96094e0 into AIDASoft:master Oct 14, 2020
@petricm petricm added the Revert label Nov 3, 2020
mrodozov added a commit to mrodozov/cmsdist that referenced this pull request Jan 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants