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

De-Template Track Finding, main branch (2024.10.01.) #722

Merged

Conversation

krasznaa
Copy link
Member

@krasznaa krasznaa commented Oct 1, 2024

So... this is how I've been imagining for a while now, how we should provide the reconstruction algorithms/tools. Not exposing any template based configurability in the APIs of the algorithms. 🤔

I wanted to demonstrate this on the track finding, because:

  • it might just be our most elaborate piece of code;
  • contrary to how it looks, we actually only ever used a single template specialization of it so far.

I moved everything from finding_algorithm.ipp into a private header under core/src/finding/. That code is still implemented as a templated function, with the same level of configurability that traccc::finding_algorithm had. But now it will be up to us to create well documented user APIs to that code, in the form of algorithms that perform one specific type of track finding each. 🤔

On the naming front I'm very open to good ideas. The name I chose now (traccc::host::default_detector_const_field_ckf_algorithm) is just meant to demonstrate that we may need to make the algorithm names quite descriptive in the end. Depending on how many different setups we may come up with.

I believe this code organization could be used for all algorithms that use detray::detector. What do you lot think?

P.S. I also tweaked the algorithmic code a little. Replacing some simple for-loops with some STL algorithms. I don't think it would change the code's performance, but I agree with @stephenswat that it's generally good practice to use STL algorithms wherever we can.

@krasznaa krasznaa added the cpu Changes related to CPU code label Oct 1, 2024
Copy link
Contributor

@beomki-yeo beomki-yeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't read the full PR - but I don't think I like this
Can't you just make a header file that defines the types that we want

// algorithms.hpp
using host_default_detector_const_field_ckf_algorithm = host::finding_algorithm<default_navigator_type, default_stepper_type>
using cuda_default_detector_const_field_ckf_algorithm =
cuda::finding_algorithm<default_navigator_type, default_stepper_type>
.. etc

@krasznaa
Copy link
Member Author

krasznaa commented Oct 3, 2024

Didn't read the full PR - but I don't think I like this Can't you just make a header file that defines the types that we want

// algorithms.hpp
using host_default_detector_const_field_ckf_algorithm = host::finding_algorithm<default_navigator_type, default_stepper_type>
using cuda_default_detector_const_field_ckf_algorithm =
cuda::finding_algorithm<default_navigator_type, default_stepper_type>
.. etc

I'm pondering about this still. 🤔 I worry about the understandability of the API. Let me tweak the proposal a bit more...

@krasznaa krasznaa marked this pull request as draft October 3, 2024 10:01
@krasznaa krasznaa force-pushed the DeTemplateTrackFinding-main-20241001 branch from d951a80 to 156bf5a Compare October 4, 2024 15:43
@krasznaa
Copy link
Member Author

krasznaa commented Oct 4, 2024

You guys might like this version even less, but here it goes... How about using some good old function overloading? 🤔

Again, I'd really like to avoid having templating "at the algorithm level". 🤔

@krasznaa krasznaa force-pushed the DeTemplateTrackFinding-main-20241001 branch from 156bf5a to 1b85d78 Compare October 5, 2024 08:16
@krasznaa krasznaa force-pushed the DeTemplateTrackFinding-main-20241001 branch from 1b85d78 to f4abcae Compare October 22, 2024 08:23
@krasznaa
Copy link
Member Author

Once we sort out #713, I'll use that to characterize these changes in a bit more detail. 🤔

The new algorithm has no template arguments, and implements
CKF track finding for just the default Detray detector geometry
with a constant magnetic field. Other concrete algorithm types
are to be added later on.
…lgorithm.

As it turns out, in every single place we were instantiating
traccc::finding_algorithm with the exact same template
parameters so far. So a single concrete algorithm could be
used to replace all those instances.
@krasznaa
Copy link
Member Author

So... Now that #713 can help with characterizing the changes of this PR.

  • In the current code, to get track finding into traccc_seq_example, the build of seq_example.cpp does everything. And it uses 1.55 GB of memory to do so. (With GCC 11.)
  • With this PR's code, bringing track finding to life in traccc_seq_example is split among the following source files:
    • ckf_algorithm.cpp: 350 MB
    • ckf_algorithm_teldet_cfield.cpp: 560 MB
    • ckf_algorithm_defdet_cfield.cpp: 810 MB
    • seq_example.cpp: 1.43 GB

So, we win a little more than 100 MB on compiling seq_example.cpp. While adding 3 more source files, with large, but semi-acceptable memory needs each.

As I discussed it with @stephenswat before lunch, I expect that after de-templating spacepoint creation and track fitting, seq_example.cpp should go down below 1 GB of memory. But I have no hard proof that it will actually happen.

As an interesting tidbit: I didn't do an exhaustive search yet for the biggest memory users, but read_detector.cpp, with 1.8 GB, seems to be a surprisingly large one. Clearly, constructing a geometry from JSON files is not an easy task for the compiler. 🤔

For @paulgessinger: Linking libActsCore.so takes a little more than 1 GB of memory. Underlining my previous statements that it's good to keep an eye on the linking steps as well. 🤔

Switched to implementing the reconstruction code for different
Detray detector and Covfie magnetic field types as overloads on
the call operator of a single "CKF algorithm".

Updated all clients to use this new name.

Split the source file, to parallelize compilation, and hopefully
reduce memory usage in individual build processes.
@krasznaa krasznaa force-pushed the DeTemplateTrackFinding-main-20241001 branch from f4abcae to e1a8b5e Compare October 22, 2024 11:25
@krasznaa krasznaa marked this pull request as ready for review October 22, 2024 11:25
@krasznaa
Copy link
Member Author

@stephenswat, @beomki-yeo, have a look again. How about organizing the code like this?

Copy link

Copy link
Member

@stephenswat stephenswat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine as far as I'm concerned. 👍

@paulgessinger
Copy link
Member

paulgessinger commented Oct 22, 2024

For @paulgessinger: Linking libActsCore.so takes a little more than 1 GB of memory. Underlining my previous statements that it's good to keep an eye on the linking steps as well. 🤔

@krasznaa I've been looking into using CMake launchers for this (COMPILER, LINKER), that avoids having to abuse CTEST_USE_LAUNCHERS for this purpose, and is non-intrusive.

@krasznaa
Copy link
Member Author

Okay, taking an executive decision here... 🤔

@krasznaa krasznaa merged commit ed05313 into acts-project:main Oct 23, 2024
23 checks passed
@krasznaa krasznaa deleted the DeTemplateTrackFinding-main-20241001 branch October 23, 2024 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cpu Changes related to CPU code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants