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!: Reduce heap allocations in navigation #1124

Merged
merged 8 commits into from
Jan 25, 2022

Conversation

paulgessinger
Copy link
Member

@paulgessinger paulgessinger commented Dec 16, 2021

This PR changes the storage values in Navigator to use boost::container::small_vector for layers, sensitive and boundary surfaces. At the same time Layer::compatibleSurface, TrackingVolume::combatibleBoundaries and TrackingVolume::compatibleLayers are modified to return this container.

This PR contains the changes from #1119, so should be merged afterwards.

I benchmarked this with the EigenStepper without covariance transport, as this shouldn't be affected by this change.

On main I get:

  Time (mean ± σ):     14.518 s ±  0.222 s    [User: 882.433 s, System: 0.663 s]
  Range (min … max):   14.249 s … 14.836 s    5 runs

while on this branch I get

  Time (mean ± σ):     13.981 s ±  0.141 s    [User: 849.680 s, System: 0.482 s]
  Range (min … max):   13.879 s … 14.228 s    5 runs

for 1000 tracks per event and 20000 events each.

The measurements seem to be just outside about 2sigma, if we take the mean at face-value, it goes down by about 3%.

@paulgessinger paulgessinger added this to the next milestone Dec 16, 2021
@paulgessinger
Copy link
Member Author

@asalzburger what do you think?

@paulgessinger paulgessinger changed the title Refactor/nav alloc refactor!: Reduce heap allocations in navigation Dec 16, 2021
@codecov
Copy link

codecov bot commented Dec 16, 2021

Codecov Report

Merging #1124 (25b70ed) into main (b6695fd) will decrease coverage by 0.03%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1124      +/-   ##
==========================================
- Coverage   48.16%   48.13%   -0.04%     
==========================================
  Files         341      341              
  Lines       17646    17660      +14     
  Branches     8327     8342      +15     
==========================================
+ Hits         8499     8500       +1     
  Misses       3372     3372              
- Partials     5775     5788      +13     
Impacted Files Coverage Δ
Core/include/Acts/Geometry/Layer.hpp 100.00% <ø> (ø)
Core/include/Acts/Geometry/TrackingVolume.hpp 69.23% <ø> (ø)
Core/include/Acts/Propagator/Navigator.hpp 55.11% <50.00%> (-1.65%) ⬇️
Core/src/Surfaces/TrapezoidBounds.cpp 75.75% <56.25%> (-13.14%) ⬇️
Core/src/Geometry/Layer.cpp 57.57% <100.00%> (+0.75%) ⬆️
Core/src/Geometry/TrackingVolume.cpp 42.99% <100.00%> (-0.37%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a0e4a18...25b70ed. Read the comment docs.

@HadrienG2
Copy link
Contributor

@paulgessinger Try re-running the benchmark with more runs. "just outside 2 sigmas" on 5 runs is not enough to claim a significant difference on benchmark timings, whose probability law is not normal.

@paulgessinger
Copy link
Member Author

Sure. This is what I get with 30 runs on main

  Time (mean ± σ):     14.444 s ±  0.183 s    [User: 878.311 s, System: 0.630 s]
  Range (min … max):   14.141 s … 14.867 s    30 runs

and on this PR

  Time (mean ± σ):     13.977 s ±  0.235 s    [User: 848.936 s, System: 0.520 s]
  Range (min … max):   13.653 s … 14.603 s    30 runs

The means seem roughly consistent with before. I can run again with even more samples.

Overall, this isn't a huge performance improvement, I agree. But looking at the propagation profile, at least I can't see any great singular optimization opportunities, where we could get a large increase in performance in one go. It rather seems like there's a number of smaller optimization opportunities that in total could be a net positive.

@robertlangenberg
Copy link
Contributor

The changes look good, can you resolve the conflicts in Layer.hpp and Layer.cpp?

@robertlangenberg robertlangenberg removed the request for review from asalzburger January 25, 2022 16:05
@robertlangenberg robertlangenberg enabled auto-merge (squash) January 25, 2022 16:05
@robertlangenberg robertlangenberg merged commit a200144 into acts-project:main Jan 25, 2022
@paulgessinger paulgessinger modified the milestones: next, v17.0.0 Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants