refactor: Do not allocate memory if the surface is connected with a detector el…

[junggjo9]

If the surface has an associated detector element its internal transform is never accessed consuming memory for no-reason. If the transform is a unique_ptr which is released in such cases, the footprint should be reduced

[codecov]

Codecov Report

Attention: Patch coverage is 23.52941% with 13 lines in your changes missing coverage. Please review.

Project coverage is 47.64%. Comparing base (5885ee8) to head (3939692).

Files	Patch %	Lines
Core/src/Surfaces/Surface.cpp	30.76%	2 Missing and 7 partials ⚠️
Core/src/Surfaces/PlaneSurface.cpp	0.00%	0 Missing and 2 partials ⚠️
Core/src/Geometry/CylinderLayer.cpp	0.00%	0 Missing and 1 partial ⚠️
Core/src/Geometry/DiscLayer.cpp	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3069      +/-   ##
==========================================
- Coverage   47.65%   47.64%   -0.01%     
==========================================
  Files         507      507              
  Lines       29205    29211       +6     
  Branches    14010    14012       +2     
==========================================
+ Hits        13917    13919       +2     
- Misses       5264     5265       +1     
- Partials    10024    10027       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[github-actions]

📊: Physics performance monitoring for d49e825

Full contents

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
• ✅ Truth tracking (GX2F)
• ✅ Particles fatras
• ✅ Particles geant4

[paulgessinger]

This is what we used to have before. I think @asalzburger at some point measured that having the transform locally improved performance. But I forget the details.

[junggjo9]

Hmm, the transform is never used if the detector element is present.... Confused, I am

[paulgessinger]

The transform is only used for surfaces which don't have a detector element. Otherwise the transform is retrieved from the detector element, and that's where for example the alignment is handled.

[junggjo9]

Yep, that what I'm saying too ;)

[paulgessinger]

So for all other surfaces, boundaries etc, the transform is accessed locally.

[asalzburger]

This is what we used to have before. I think @asalzburger at some point measured that having the transform locally improved performance. But I forget the details.

Hi, yes I remember that there was a visible (but small performance change), however, we can just see with this PR if this is still a measurable effect?

[andiwand]

I could imagine that this is an additional cache miss because of the additional indirection. After measuring this we would have to weight the CPU increase against the MEM decrease

[junggjo9]

Results from the propagation test on 3 threads

patched:

10:03:28    Sequencer      INFO      Processed 100000 events in 1223.381458 s (wall clock)
10:03:28    Sequencer      INFO      Average time per event: 48.693846 ms/event

main:

22:24:31    Sequencer      INFO      Processed 100000 events in 1237.661429 s (wall clock)
22:24:31    Sequencer      INFO      Average time per event: 49.130005 ms/event

[AJPfleger]

@junggjo9 @paulgessinger @asalzburger @andiwand
In the tests, it makes a difference of around 1% (not sure if this is significant). How should we proceed with this?

[andiwand]

no significant change in performance - good from my side

[andiwand]

There are a couple of CI failure

• Can you check the formatting @junggjo9 ?
• I guess the CI Bridge ones are false positives
• codecov complains - I guess because there are a couple more branches which are not directly tested right now? Should we add some unit tests to capture the new behavior?

[andiwand]

lgtm 👍

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
68.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[acts-project-service]

✅ Athena integration test results [bd98f56]

✅ All tests successful

status	job	report
🟢	report_pull_request
🟢	run_art_test: test_data18_13TeV_1000evt
🟢	run_art_test: test_ttbarPU40_reco
🟢	test_ActsDumpGeometryIdentifiers
🟢	test_ActsCheckObjectCountsWorkflow
🟢	test_ActsEFTrackFit
🟢	test_ActsPersistifySeeds
🟢	test_ActsBenchmarkWithSpot
🟢	test_ActsAnalogueClustering
🟢	test_ActsWorkflowHeavyIons
🟢	test_ActsWorkflowFastTracking
🟢	test_ActsWorkflowCached
🟢	test_ActsWorkflow
🟢	test_ActsValidateAmbiguityResolution
🟢	test_ActsValidateResolvedTracks
🟢	test_ActsValidateTracks
🟢	test_ActsValidateActsCoreSpacePoints
🟢	test_ActsValidateActsSpacePoints
🟢	test_ActsValidateSeeds
🟢	test_ActsValidateOrthogonalSeeds
🟢	test_ActsValidateClusters
🟢	test_ActsPersistifyEDM
🟢	test_ActsGSFRefitting
🟢	test_ActsKfRefitting
🟢	test_ActsExtrapolationAlgTest
🟢	test_ActsITkTest
🟢	run_workflow_tests_run4_mc
🟢	run_workflow_tests_run2_mc
🟢	run_workflow_tests_run2_data
🟢	run_workflow_tests_run3_mc
🟢	run_workflow_tests_run3_data
🟢	run_unit_tests