Speed up Puppi

[kpedro88]

PR description:

Switches tend to be faster than else if when more than a couple of (simple) cases exist (better optimization by the compiler). This change in particular made Puppi 23% faster (tested on a ttbar UL sample in 10_6_X).

PR validation:

Code compiles and runs successfully, no changes expected or observed.

This PR will be backported to 10_6_X to speed up ultra-legacy analysis processing. Backports to other releases can be made upon request.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35799/26164

• This PR adds an extra 16KB to repository

[cmsbuild]

A new Pull Request was created by @kpedro88 (Kevin Pedro) for master.

It involves the following packages:

• CommonTools/PileupAlgos (reconstruction)

@jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@rappoccio, @missirol, @ahinzmann, @jdolen, @gkasieczka, @hatakeyamak, @clelange this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

[kpedro88]

please test

[VinInn]

not anymore in recent gcc
https://godbolt.org/z/eqYYcqhM1

[VinInn]

and in clang since ever.

[kpedro88]

Good to know that newer gcc versions have picked up the better optimization. Nevertheless, it's still worthwhile to have the optimization in 10_6_X with its older gcc version. I leave it up to release managers whether they still want the change to enter master, even if it no longer has an effect.

[VinInn]

The switch code is cleaner anyhow: strange that gcc was not able to optimize.

[slava77]

enable profiling

[slava77]

@cmsbuild please test

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ccffe7/19865/summary.html
COMMIT: 45ac7ee
CMSSW: CMSSW_12_1_X_2021-10-23-1100/slc7_amd64_gcc900
Additional Tests: PROFILING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35799/19865/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

• No significant changes to the logs found
• Reco comparison results: 0 differences found in the comparisons
• DQMHistoTests: Total files compared: 40
• DQMHistoTests: Total histograms compared: 2751113
• DQMHistoTests: Total failures: 0
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 2751091
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 39 files compared)
• Checked 170 log files, 37 edm output root files, 40 DQM output files
• TriggerResults: no differences found

[kpedro88]

@gartung the "Igprof Comparison cpu usage RECO produce methods." page in the profiling results is blank. is there a bug in the script that needs to be fixed?

[slava77]

strangely enough this still shows up as tests started

ah, I guess that in response to the last comment somebody restarted the profiling tests some 20 minutes ago.

[gartung]

Reverting to the older script revealed the real issue

+ /data/cmsbld/jenkins/workspace/ib-run-pr-profiling/profiling/Analyze_tool/compare_cpu_txt.py --old CMSSW_12_1_X_2021-10-23-1100_RES_CPU_step3.txt --new RES_CPU_step3.txt
Traceback (most recent call last):
  File "/data/cmsbld/jenkins/workspace/ib-run-pr-profiling/profiling/Analyze_tool/compare_cpu_txt.py", line 2, in <module>
    import pandas as pd
ImportError: No module named pandas

[kpedro88]

@gartung if this script relies on the CMSSW environment, maybe it needs to be changed to #!/bin/env python3 rather than just python?

[gartung]

Yes. I just pushed that fix.

[gartung]

I resubmitted the pr-profiling Jenkins job which will pick up this fix.

[jpata]

• phase2 profiling is here: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ccffe7/19865/profiling/23434.21/RES_CPU_compare_23434.21.txt
• run3 is blank (wasn't resubmitted I suppose): https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ccffe7/19865/profiling/11834.21/RES_CPU_compare_11834.21.txt

Is any measurable effect expected or observed here for phase2?

[gartung]

I see a similar difference in 23424.21 workflow for a test pull request
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-766103/19922/profiling/23434.21/RES_CPU_compare_23434.21.txt
The 11834.21 workflow for the test pull request completed
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-766103/19922/profiling/11834.21/RES_CPU_compare_11834.21.txt

[gartung]

The jenkins job log shows that a new RES_CPU_compare_11834.21.txt was created but it was not copied. Triggering the build again should create a new results directory and allow the file to be uploaded.l

[gartung]

please test

[kpedro88]

PuppiProducer isn't listed in that CPU comparison. I guess it isn't high enough on the list.

If I drill down into the full igprof reports:
Baseline: https://cmssdt.cern.ch/SDT/cgi-bin/igprof-navigator/CMSSW_12_1_X_2021-10-23-1100/slc7_amd64_gcc900/profiling/23434.21/igprofCPU_step3/1131
PR: https://cmssdt.cern.ch/SDT/cgi-bin/igprof-navigator/CMSSW_12_1_X_2021-10-23-1100/slc7_amd64_gcc900/profiling/23434.21/PR-ccffe7/19865/igprofCPU_step3/1158

The % total does decrease, though the overall time usage seems to be higher for the PR (for everything, not just this model).

My use case was rerunning Puppi on top of miniAOD, where it's a larger % of the total.

[gartung]

The increase in time is probably caused by enabling the IgprofService in the job which takes some time to write out the profile after the first, middle and next to last event.

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ccffe7/19957/summary.html
COMMIT: 45ac7ee
CMSSW: CMSSW_12_1_X_2021-10-26-1100/slc7_amd64_gcc900
Additional Tests: PROFILING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35799/19957/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

• No significant changes to the logs found
• Reco comparison results: 4 differences found in the comparisons
• DQMHistoTests: Total files compared: 41
• DQMHistoTests: Total histograms compared: 2797338
• DQMHistoTests: Total failures: 5
• DQMHistoTests: Total nulls: 1
• DQMHistoTests: Total successes: 2797310
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: -0.004 KiB( 40 files compared)
• DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
• Checked 173 log files, 37 edm output root files, 41 DQM output files
• TriggerResults: no differences found

[jpata]

+reconstruction

• technical
• no runtime change detected in the PR profiling (none expected as far as I understand)

[cmsbuild]

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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

[perrotta]

+1

• Technical