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

slowdown in PFCandidatePrimaryVertexSorter in CMSSW_12_1_0_pre2 -> CMSSW_12_1_0_pre3 #35986

Closed
jpata opened this issue Nov 4, 2021 · 14 comments

Comments

@jpata
Copy link
Contributor

jpata commented Nov 4, 2021

Slava noticed that it looks like the PFCandidatePrimaryVertexSorter is called 4x in CMSSW_12_1_0_pre3 [1] instead of 2x in CMSSW_12_1_0_pre2 [2], and it's a fairly significant increase in the step4 runtime.

There are two new callers:

  • primaryVertexAssociationJME
  • primaryVertexAssociationJMEnoHF

[1] http://cms-reco-profiling.web.cern.ch/cms-reco-profiling/circles/piechart.php?local=false&dataset=CMSSW_12_1_0_pre3%2Fslc7_amd64_gcc900%2F11834.21%2Fstep4_circles&resource=time_thread&colours=default&groups=packages&threshold=0

[2] http://cms-reco-profiling.web.cern.ch/cms-reco-profiling/circles/piechart.php?local=false&dataset=CMSSW_12_1_0_pre2%2Fslc7_amd64_gcc900%2F11834.21%2Fstep4_circles&resource=time_thread&colours=default&groups=packages&threshold=0

@laurenhay @ahinzmann could this be related to #33885?

@jpata
Copy link
Contributor Author

jpata commented Nov 4, 2021

assign reconstruction

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 4, 2021

New categories assigned: reconstruction

@slava77,@jpata you have been requested to review this Pull request/Issue and eventually sign? Thanks

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 4, 2021

A new Issue was created by @jpata Joosep Pata.

@Dr15Jones, @perrotta, @dpiparo, @makortel, @smuzaffar, @qliphy can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

@slava77
Copy link
Contributor

slava77 commented Nov 4, 2021

Looking at https://cms-reco-profiling.web.cern.ch/cms-reco-profiling/cgi-bin/igprof-navigator/releases/CMSSW_12_1_0_pre4/34834.21/step4/cpu_endjob/21
or https://cms-reco-profiling.web.cern.ch/cms-reco-profiling/cgi-bin/igprof-navigator/releases/CMSSW_12_1_0_pre4/11834.21/step4/cpu_endjob/22

it's a bit puzzling to see that a call to chargedHadronVertex(vertices, iVertex, ... does not appear in the cost stack, as if it was completely inlined in the chargedHadronVertex(vertices, trackRef...
(I doubt that its cost is so small that it would never show up in the sampling).

Perhaps for debugging it's worth to compile the two functions in two different .cc files?

@ahinzmann
Copy link
Contributor

yes, this is because the new modules primaryVertexAssociationJME is taking the track-vertex-association-decision for CHS and PUPPI. This could be speed up, by not reusing the same module, but instead making a more light-weight association module for CHS and PUPPI. We plan to implement this, once the CHS/PUPPI vertex-association criteria for Run3 are fixed.

@jpata
Copy link
Contributor Author

jpata commented Nov 15, 2021

We plan to implement this, once the CHS/PUPPI vertex-association criteria for Run3 are fixed.

could you clarify a bit what's the timeline for this?

@ahinzmann
Copy link
Contributor

so first studies are done, but need to iterate order of 2 weeks, before taking a decision

@jpata
Copy link
Contributor Author

jpata commented Feb 8, 2022

I believe this was addressed in #36639.

@jpata jpata closed this as completed Feb 8, 2022
@slava77
Copy link
Contributor

slava77 commented Feb 8, 2022

Slava noticed that it looks like the PFCandidatePrimaryVertexSorter is called 4x in CMSSW_12_1_0_pre3 [1] instead of 2x in CMSSW_12_1_0_pre2 [2], and it's a fairly significant increase in the step4 runtime.

looking at http://cms-reco-profiling.web.cern.ch/cms-reco-profiling/circles/piechart.php?local=false&dataset=CMSSW_12_3_0_pre4%2Fslc7_amd64_gcc10%2F11834.21%2Fstep4_circles&resource=time_thread&colours=default&groups=packages&threshold=0

image

I still see 4 cases taking about 13% of CPU time; compared to 12% in CMSSW_12_1_0_pre3 when the issue was opened.
Am I missing something?

@jpata jpata reopened this Feb 8, 2022
@jpata
Copy link
Contributor Author

jpata commented Feb 8, 2022

cc @ahinzmann

@ahinzmann
Copy link
Contributor

Looking at https://github.com/cms-sw/cmssw/releases, I don't see #36639 listed under the PRs entering CMSSW_12_3_0_pre4. Is it integrated into release yet?

@slava77
Copy link
Contributor

slava77 commented Feb 8, 2022

@slava77
Copy link
Contributor

slava77 commented Feb 8, 2022

sorry for the noise, I guess the issue can be closed after all

@jpata
Copy link
Contributor Author

jpata commented Feb 9, 2022

thanks for the checking (and sorry for not double checking on my end).

@jpata jpata closed this as completed Feb 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants