Speedup of mkFit hit converters, and introduction of mkFit HLT customization for 2025 #47106
Conversation
|
cms-bot internal usage |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47106/43294 |
|
A new Pull Request was created by @mmasciov for master. It involves the following packages:
@cmsbuild, @jfernan2, @mandrenguyen can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
do you know why |
I'm curious, what is the timing reduction measured using the framework report or the |
https://stackoverflow.com/questions/32435796/when-to-use-stdhypotx-y-over-stdsqrtxx-yy Looks like builtin overflow / underflow checks slow it down? |
When running the same HLT configuration on the same machine, I had checked the |
|
+1 Size: This PR adds an extra 112KB to repository Comparison SummarySummary:
|
What's the final customization function to be applied on top of the HLT menu for testing purposes? |
Correct. As mentioned at today's TSG meeting, I'm going to push a new commit, renaming |
…h new customization function targeting 2025
|
Pull request #47106 was updated. @cmsbuild, @jfernan2, @mandrenguyen can you please check and sign again. |
|
enable profiling |
|
Given that profiling was enabled, I'll start the tests again (on top of those started by @mmusich for cms-data/RecoTracker-MkFit#15, which are equivalent except for profiling) |
|
@cmsbuild, please test |
if the pure configuration addition at that cms-data PR is considered final, it might be convenient to get that out of the way as early as possible (no physics change from that alone). |
|
+1 Size: This PR adds an extra 20KB to repository The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
You can see more details here: Comparison SummarySummary:
|
|
Just for my understanding: although the metric is not the same (FastTimer vs callgrind), the differences in MkFitSiPixelHitConverter, MkFitSiStripHitConverter and MkFitEventOfHitsProducer do not seem to be so pronounced in the Offline profiling test |
Unless I'm reading this wrong, what is reported is the "time fraction diff percent". So, the reported reduction is relative to the total time. The MkFitProducer for detachedTriplet and pixelLess steps takes a significant amount of time with respect to MkFitSiPixelHitConverter, MkFitSiStripHitConverter and MkFitEventOfHitsProducer, the fractions being 2.4 and 4.7 for MkFitProducer in detachedTriplet and pixelLess, respectively, as opposed to 0.007, 0.09 and 0.08 for the converters. |
| @@ -39,11 +72,13 @@ def customizeHLTIter0ToMkFit(process): | |||
|
|
|||
There was a problem hiding this comment.
process.hltMkFitGeometryESProducer = mkFitGeometryESProducer_cfi.mkFitGeometryESProducer.clone()when I try to run the customization function in the HLT addOnTests, I get the following failure:
----- Begin Fatal Exception 16-Jan-2025 12:34:44 CET-----------------------
An exception of category 'EventSetupConflict' occurred while
[0] Calling beginJob
Exception Message:
two EventSetup Producers want to deliver type="MkFitGeometry" label=""
from record TrackerRecoGeometryRecord. The two providers are
1) type="MkFitGeometryESProducer" label="hltMkFitGeometryESProducer"
2) type="MkFitGeometryESProducer" label="mkFitGeometryESProducer"
Please either
remove one of these Producers
or find a way of configuring one of them so it does not deliver this data
or use an es_prefer statement in the configuration to choose one.
----- End Fatal Exception -------------------------------------------------
in the step that runs HLT+RECO in the same step. Would it be possible to substitute this line with:
process.load("RecoTracker.MkFit.mkFitGeometryESProducer_cfi")such that one ESProducer overrides the other? This change would be needed also for the final implementation in ConfDB by the way.
Otherwise, please append the data to a different label, not the default '' (and change the input label for all the EDProducers that consume this product).
There was a problem hiding this comment.
Of course: d578a59
Thanks!
cmsbuild
added
tests-pending
code-checks-pending
and removed
tests-approved
code-checks-approved
labels
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47106/43325 |
|
Pull request #47106 was updated. @cmsbuild, @jfernan2, @mandrenguyen can you please check and sign again. |
|
please test |
Isn't the printout buggy in the E.g. the topmost line |
Could you copy that comment to #43166 ? |
PR description:
This PR aims at speeding up the mkFit (input) converters and the mkFit producer, targeting HLT in 2025:
-
RecoTracker/MkFit/plugins/MkFit*HitConverter.ccandconvertHits.h: save mkFit layer index per hit to be used for a fasterMkFitEventOfHitsProducer; use more variables available for a givenDetto avoid their recomputation per hit (including hit local->global conversion); access clusters from the reference collection by index directly instead of dereferencing anOmniClusterRef-
RecoTracker/MkFit/plugins/MkFitEventOfHitsProducer.ccuses pre-computed per-hit layer index-
RecoTracker/MkFit/plugins/MkFitProducer.cc(and customization functions) : disable (by config) additional/redundant re-check of the input cluster charge (already applied during the hit creation); this should work OK for an mkFit setup where CCC is the same in all iterations-
RecoTracker/MkFitCore/interface/Hit.h: vdt, replace (slow)std::hypotwith explicitsqrt(x*x+y*y); useSMatrixSym33for direct covariance storage to avoid conversion fromSVector6-
RecoTracker/MkFitCore/src/MkFinder.cc,RecoTracker/MkFitCore/src/MkFitter.cc,RecoTracker/MkFitCore/src/PropagationMPlex*.cc,RecoTracker/MkFitCore/src/Track.ccvdt, replace (slow)std::hypotwith explicitsqrt(x*x+y*y)Timing reduction (based on callgrind) in MC ttbar with PU using mkFit at HLT configuration is overall ~30% in mkFit-related modules:
MkFitEventOfHitsProducer: -40%MkFitSiStripHitConverter: -35%MkFitSiPixelHitConverter: -40%MkFitProducer: -14% (7% is the faster math, 7% from not updating the cluster mask with CCC)For testing of the HLT configuration, PR cms-data/RecoTracker-MkFit#15 is required.
PR validation:
This PR was validated using both offline and HLT configurations.
For offline: http://uaf-10.t2.ucsd.edu/~mmasciov/MIC/HLTTracking/offlineMTV_TTbarPU_2024_PR155/
--> Only differences are at the level of fluctuations, consistently with the proposed changes.
FYI: @cms-sw/tracking-pog-l2, @slava77, @mtosi, @missirol