Use an indexed sort for expensive comparisons in MuonShowerInformationFiller

[dan131riley]

PR description:

MuonShowerInformationFiller has several sorts over the muon hits that use comparison functions involving complex calculations. This has two problems, (1) the comparison function is executed O(n*log(n)), so the same complex calculations are performed multiple times, and (2) when vector instructions are enabled, the values calculated may not be completely stable, which may make the sort ill-defined. See issue #33663 for details.

This PR replaces the use of stable_sort with a custom sort function that pre-calculates and caches the values to be sorted, performs an index sort on the cached values, and constructs the output container. This reduces the number of complex calculations to O(n), at some cost in complexity and memory usage. In practice this is measured to reduce the CPU time for a typical sort by 2/3. It also fixes the segmentation faults.

In addition, this PR also corrects two places where I believe the in-place sorting of stable_sort was incorrectly sorting the input containers to two functions.

It would arguably be better for the MuonShowerInformationFiller to define its own hit type that caches the global coordinates and derived quantities; however, that would be a much more intrusive change, where this PR is fairly minimal.

PR validation:

Workflow 136.844 runs successfully in a Skylake IB, spot checks of the sort results found the results to be identical. Mostly technical fix, but may cause small physics changes due to the correction of the inadvertent sorts.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40675/34023

• This PR adds an extra 20KB to repository

[cmsbuild]

A new Pull Request was created by @dan131riley (Dan Riley) for master.

It involves the following packages:

• RecoMuon/MuonIdentification (reconstruction)

@cmsbuild, @mandrenguyen, @clacaputo can you please review it and eventually sign? Thanks.
@bellan, @andrea21z, @abbiendi, @JanFSchulte, @Fedespring, @missirol, @HuguesBrun, @jhgoh, @CeliaFernandez, @trocino, @cericeci, @rociovilar this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

[clacaputo]

enable profiling

[clacaputo]

please test

[cmsbuild]

-1

Failed Tests: RelVals-INPUT
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-be7814/30381/summary.html
COMMIT: 324d91d
CMSSW: CMSSW_13_0_X_2023-02-02-2300/el8_amd64_gcc11
Additional Tests: PROFILING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/40675/30381/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals-INPUT

The relvals timed out after 4 hours.

Comparison Summary

Summary:

• You potentially added 5 lines to the logs
• Reco comparison results: 13 differences found in the comparisons
• DQMHistoTests: Total files compared: 49
• DQMHistoTests: Total histograms compared: 3555495
• DQMHistoTests: Total failures: 11
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3555462
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
• Checked 213 log files, 164 edm output root files, 49 DQM output files
• TriggerResults: no differences found

[clacaputo]

please test

[clacaputo]

type muon

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-be7814/30451/summary.html
COMMIT: 324d91d
CMSSW: CMSSW_13_0_X_2023-02-06-2300/el8_amd64_gcc11
Additional Tests: PROFILING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/40675/30451/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

• You potentially added 3 lines to the logs
• Reco comparison results: 13 differences found in the comparisons
• DQMHistoTests: Total files compared: 49
• DQMHistoTests: Total histograms compared: 3555495
• DQMHistoTests: Total failures: 14
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3555459
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
• Checked 213 log files, 164 edm output root files, 49 DQM output files
• TriggerResults: no differences found

[clacaputo]

+reconstruction

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

[perrotta]

+1