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

refactor: Split GainMatrixUpdater compilation #3486

Merged

Conversation

paulgessinger
Copy link
Member

@paulgessinger paulgessinger commented Aug 6, 2024

Once again, our compilation memory usage is out of control, among others due to GainMatrixUpdater. This currently takes about 1.2G peak memory.

In this PR, I'm moving the function doing the heavy instantiation back into a header, but I'm suppressing the instantiation of the template via extern template.

Then, I have CMake generate .cpp files where each file instantiates one dimension. The linker ultimately resolves these separate template insantiations.

Now, this is a bit ugly, but it does reduce peak memory consumption to under 600M, at the cost of now having 6 instead of one compilation unit:

[   16.73M, max:   295.39M] [    1.63s] - Core/src/TrackFitting/GainMatrixUpdater.cpp
[   16.58M, max:   431.10M] [    2.95s] - build/Core/src/TrackFitting/GainMatrixUpdaterImpl1.cpp
[   16.58M, max:   456.64M] [    3.13s] - build/Core/src/TrackFitting/GainMatrixUpdaterImpl2.cpp
[   17.06M, max:   485.18M] [    3.40s] - build/Core/src/TrackFitting/GainMatrixUpdaterImpl3.cpp
[   16.86M, max:   456.70M] [    3.24s] - build/Core/src/TrackFitting/GainMatrixUpdaterImpl4.cpp
[   16.58M, max:   568.34M] [    4.37s] - build/Core/src/TrackFitting/GainMatrixUpdaterImpl5.cpp
[   16.73M, max:   537.15M] [    4.11s] - build/Core/src/TrackFitting/GainMatrixUpdaterImpl6.cpp

Not sure this is the way to go, but I wanted to suggest it.

@paulgessinger paulgessinger added this to the next milestone Aug 6, 2024
Copy link
Contributor

@andiwand andiwand left a comment

Choose a reason for hiding this comment

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

Since this cuts the memory consumption drastically I would definitely consider putting this in. One thing I wondered is if we want to move the templated visit measurement into a detail header and also the generated source files and then only include them in the GainMatrixUpdater.cpp

@paulgessinger
Copy link
Member Author

@andiwand The generated source files are only in the build folder, not in the source folder at all. In terms of where exactly the template function is, I don't have a preference. We'd have to include it the .cpp file, but it doesn't matter where it is.

@andiwand
Copy link
Contributor

andiwand commented Aug 6, 2024

Maybe we can just put it directly into the GainMatrixUpdater.cpp

@paulgessinger
Copy link
Member Author

@andiwand And then include the .cpp file? Always feels weird to me.

@andiwand
Copy link
Contributor

andiwand commented Aug 6, 2024

Right it really should be a header file. I would put it rather in detail to box the template and only show it to the source file. But it is only a slight preference on my side

@paulgessinger
Copy link
Member Author

Done now @andiwand.

@kodiakhq kodiakhq bot merged commit ea794f2 into acts-project:main Aug 7, 2024
45 checks passed
@github-actions github-actions bot removed the automerge label Aug 7, 2024
Copy link

sonarcloud bot commented Aug 7, 2024

@paulgessinger paulgessinger deleted the refactor/gain-matrix-updater-split branch August 7, 2024 09:48
@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Aug 7, 2024
@paulgessinger paulgessinger modified the milestones: next, v36.1.0 Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Core Affects the Core module Event Data Model Fails Athena tests This PR causes a failure in the Athena tests Track Fitting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants