Significant reduction of memory footprint for 2D templates, etc. #23263
Conversation
… in 3D template reco.
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-23263/4749 |
|
A new Pull Request was created by @pmaksim1 (Petar Maksimovic) for master. It involves the following packages: RecoLocalTracker/SiPixelRecHits @perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
Comparison is ready @slava77 comparisons for the following workflows were not done due to missing matrix map:
Comparison Summary:
|
|
Thanks. So I propose this PR gets integrated ASAP, and we immediately start putting together another one with a new location of the templates. I presume we are going to make one small package just for the low-level pixel reco. Where should it go, so that it can be references both by RecoLocalTracker and simulation packages (both SiPixelDigitizer and FastSimulation/TrackingRecHitProducer)? |
|
Thank you for the pull request. Please have also a look at the issues pointed out by the static analyzer: |
|
This is pretty cool! (Is there a way for me to run it myself before making a PR? That may save everybody some time...) The "dead assignments" are trivial. (That would have been a part of cosmetic cleanup in the next PR anyway, but we can start now.) For the "memory leak"... I need to talk to Morris since the logic is a bit convoluted... Moreover, the job should really raise a fatal exception right there anyway, since there's no point proceeding. (So the problem is superficial, as at that point we have a much greater problem than just leaking memory.) |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
The memory performance was inspected with igprof (MEM_LIVE) The overall job memory used in the 2018 TTbar MC workflow 10824 while allowing the ClusterRepair and PixelTemplates (which are still not enabled by default in the config) increases by some 53 MB with this PR. With the same setup and before merging this PR the overall increase amounts to some 169 MB. Therefore, this pull requests reduces by a factor 3.2 the additional memory footprint of 2D templates, which is in line with what written in the PR description. There is probably room for some further improvement, but this is definitely a good start. Going into the details, the memory reduction is mostly visible in the following modules and methods:
The largest additional memory eater when 2DTemplates are enabled remains now |
|
With the same igprof, I don't see sign of memory leakes in 2D templates. Still, I believe that the procedure of I would let as a possible improvement for a future PR the move to smart pointers for those |
|
A few comparison with the current version of 2D templates (i.e. enabling those templates in CMSSW_10_2_0_pre4) show some non negligible difference in track outputs: Some change in track outputs, with some 2% increase in the number of generalTracks
Quite noticeable is the reduction of electron seeds in a given DPhi bin:
The whole set of comparisons can be seen in /afs/cern.ch/work/a/aperrott/public/TESTPR23263 @pmaksim1 : are these differences understood/acceptable due to the bug fixes and adjustments included in this PR? |
|
Do not understand at all the effect on the electronSeeds: they should NOT use templates. |
|
@VinInn @pmaksim1 (Just to make clare what I did for the comparisons posted in #23263 (comment)) I compared baseline CMSSW_10_2_X_2018-05-30-2300 and the same baseline plus this PR where IN BOTH CASES I made the following modifications (as it was suggested in the initial 2D template PR):
with |
|
@perrotta @VinInn
1) I confirm that this is the correct recipe (given that you are running on
ttbar MC)
2) there should be small changes -- 2% improvement in the number of general
is probably reasonable. (Some rec hits that were on edges and were crap
are now more reasonable. We have lots of hits like these.)
3) I have no idea about the electronSeeds; AFAICT nothing in our PR should
have affected them.
4) I was trying to access other plots in /afs/
cern.ch/work/a/aperrott/public/TESTPR23263, but I don't have permission to
access them from lxplus. Can they be viewed on the web?
Thanks!
…On Sat, Jun 2, 2018 at 5:10 AM, perrotta ***@***.***> wrote:
@VinInn <https://github.com/VinInn> @pmaksim1
<https://github.com/pmaksim1> (Just to make clare what I did)
I compared baseline CMSSW_10_2_X and the same baseline plus this PR where
IN BOTH CASES I made the following modifications (as it was suggested in
the initial 2D template PR):
- Uncomment in RecoTracker/TransientTrackingRecHit/python/
TTRHBuilderWithTemplate_cfi.py the following two lines
# from Configuration.Eras.Modifier_phase1Pixel_cff import phase1Pixel
# phase1Pixel.toModify(TTRHBuilderAngleAndTemplate, PixelCPE = cms.string('PixelCPEClusterRepair'))
- Replace in CalibTracker/SiPixelESProducers/plugins/
SiPixel2DTemplateDBObjectESProducer.cc
- // std::string label = "denominator"; // &&& Temporary: matches Barrel Layer1 fullsim MC
- std::string label = ""; // the correct default
with
+ std::string label = "denominator"; // &&& Temporary: matches Barrel Layer1 fullsim MC
+ // std::string label = ""; // the correct default
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#23263 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AEppUUQbSdJo5ZEUyCQMmZsfU88JECilks5t4lZ8gaJpZM4UF71G>
.
|
|
@pmaksim1 (et al.): you can access the plots with the differences in reco and miniAOD from |
|
Awesome, thanks! I can see them all now. I went through a bunch of plots, and I didn't see much of a difference. The only differences seem to be in the number of hits, e.g. However, it's hard to say whether the size of the effect is appropriate. One would really need to understand how the hits are picked up, and if some of the hits get artificially blow-up errors, how much more hits would be picked up. I will crowd-source the examination of these plots :) and we'll try to identify all that visibly differ. That being said, we may need to involve other experts -- esp. from tracking and people who understand all of these objects. Given that we are still parasitic (i.e. I don't think we are turning this own by default just yet, right?), what is the level of scrutiny you'd like to see? |
|
@pmaksim1 : I would like that we all get convinced that this PR does not introduce new bugs or mistakes in the code. Besides that, I understand that the 2D templates and ClusterRepair are not enabled yet in CMSSW, and if some tuning of the parameters is needed this can be applied when they will be enabled, and I don't expect they will be already integrated now. |
|
@perrotta @VinInn @slava77
The problem with using 10.2.0pre4 with ClusterRepair enabled is that we are using code which has a couple of known bugs fixed by this PR. So if we see differences, I'm not sure what we should conclude from that. So, can we run this ourselves? Or these histograms have already been run for the vanilla 10.2.0pre4, and we just need to compare them? (If you know how to do it and it's easy, we'd appreciate help with it :) Thanks! |
|
you can find the difference between this PR with cluster repair enabled and the baseline (no cluster repair) in http://perrotta.web.cern.ch/perrotta/TESTPR23263vsBASELINE Differences look healtier to me, and it goes in the direction of your explanation based on the effect of the bug fixes implemented here I plan to sign this PR in time for this afternoon ORP, but I let you have a look at the new comparison in case you notice anything worth to report or investigate further. Please let me know if you have any comment |
|
Hi @perrotta , Thanks, this was very helpful! So as we suspected, the issues arose mostly from the old 2D reco, the new one is much better. @OzAmram just summarized at the pixel offline meeting: There are a couple of small things we are still looking into, but since the feature is still parasitic, we agree with you that you can sign off on it. Thanks for all the help! |
|
+1
|
cmsbuild
added
fully-signed
reconstruction-approved
and removed
pending-signatures
reconstruction-pending
labels
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
Tests:
As discussed previously, the plan is to have a separate FastSim PR and then another SiPixelRecHits + SiPixelDigitizer + pixel FastSim PR which involves moving templates + miscellanea to another package. So I'd like to defer all cosmetics to that PR. (But you can still give me suggestions :)
@perrotta @slava77 @fabiocos @makortel @tvami @tsusa @OzAmram @cmantill @schuetzepaul