acts
acts copied to clipboard
acts-project
refactor!: Surface returns transform by value
This PR tests how much performance loss we would experience when changing the return object of Surface objects from
const Transform& transform(const GeometryContext& gctx) const;
to
Transform transform(const GeometryContext& gctx) const;
This change would allow us to create surfaces on the fly, (or at least their transforms) for e.g. Drift straws, etc.
Initial tests - running the propagation test only, which is relatively highly effected by this - shows.
Propagation test
without this change
09:57:30 Sequencer INFO Average time per event: 19.184849 ms/event
with this change
09:56:55 Sequencer INFO Average time per event: 20.279401 ms/event
It indicates a 5% penalty in this workflow.
Truth Tracking
14:07:33 Sequencer INFO Average time per event: 44.465655 ms/event
vs.
14:07:42 Sequencer INFO Average time per event: 45.216276 ms/event
So, closer to 2 % effect there, already interesting, I will do a full chain run as well.
We probably want to run some workflows a number of times to characterize the performance. 5% seems non-negligible.
We probably want to run some workflows a number of times to characterize the performance. 5% seems non-negligible.
Yes, completely agree. I will run a full chain workflow later to see where we stand on that.
Codecov Report
Attention: Patch coverage is 5.00000% with 19 lines in your changes are missing coverage. Please review.
Project coverage is 49.74%. Comparing base (
4f33d42) to head (a5264dd). Report is 163 commits behind head on main.
:exclamation: Current head a5264dd differs from pull request most recent head a4063e0
Please upload reports for the commit a4063e0 to get more accurate results.
Additional details and impacted files
@@ Coverage Diff @@
## main #2527 +/- ##
==========================================
+ Coverage 48.82% 49.74% +0.91%
==========================================
Files 491 466 -25
Lines 28886 26310 -2576
Branches 13705 12104 -1601
==========================================
- Hits 14104 13087 -1017
+ Misses 4954 4616 -338
+ Partials 9828 8607 -1221
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
alternatively we could cache the transform in a unique_ptr or in a pool to avoid recomputation. but that would also be up for measurements if that actually happens a lot and if it makes a difference
📊: Physics performance monitoring for a5264ddf90bd1d1029a5c9ab797fead546ccf620
physmon summary
- ✅ CKF truth_smeared
- ✅ IVF truth_smeared
- ✅ AMVF truth_smeared
- ✅ Track Summary CKF truth_smeared
- ✅ Seeding truth_estimated
- ✅ CKF truth_estimated
- ✅ IVF truth_estimated
- ✅ AMVF truth_estimated
- ✅ Track Summary CKF truth_estimated
- ✅ Seeding seeded
- ✅ CKF seeded
- ✅ IVF seeded
- ✅ AMVF seeded
- ✅ AMVF (+grid seeder) seeded
- ✅ Track Summary CKF seeded
- ✅ Seeding orthogonal
- ✅ CKF orthogonal
- ✅ IVF orthogonal
- ✅ AMVF orthogonal
- ✅ Track Summary CKF orthogonal
- ✅ Ambisolver seeded
- ✅ Ambisolver orthogonal
- ✅ Seeding ttbar
- ✅ CKF ttbar
- ✅ Ambisolver
- ✅ Track Summary CKF ttbar
- ✅ AMVF ttbar
- ✅ AMVF (+grid seeder) ttbar
- ✅ Truth tracking (GSF)
- ✅ Truth tracking
- ✅ Particles inital fatras
- ✅ Particles final fatras
- ✅ Particles inital geant4
- ✅ Particles final geant4
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
alternatively we could cache the transform in a
unique_ptror in a pool to avoid recomputation. but that would also be up for measurements if that actually happens a lot and if it makes a difference
I am not sure we should go for some lazy initialisation - I hope we can stick to a clean design: either object and copy or pre-construct and return by const ref.
We could consider using an atomic pointer to model lazy initialization and caching of the transform.
Anyway, should we move forward with this, or close it for now?