Revert "speedup SiStripClusterizer(FromRaw) using ThreeThresholdAlgorithm"

[slava77]

Reverts #47061

#47061 (comment)

Following the TSG timing checks of the full unpacking/clustering showing possible slow down (initially reported late Jan/early Feb), I rechecked the impact and (unfortunately) confirm on data inputs (full menu based on /dev/CMSSW_14_2_0/GRun/V12 data from run 386593, 10K events, single thread test, in a configuration with mkFit and full strip unpacking and max cluster size set to 8):

• valgrind callgrind still shows a speed up of around 25%
• FastTimerService shows around 10% slowdown in hltSiStripRawToClustersFacility

I can speculate that callgrind is so slow that the underlying hardware memory access costs are mis-represented

I tried to measure the impact of individual contributions

• changes in template/inline of ThreeThresholdAlgorithm have almost negligible impact on timing
• precomputed qualityBits and rawNoises lead to most of the slowdown based on FastTimerService

The timing tests were done in CMSSW_14_2_1

@mmasciov
@elusian

[cmsbuild]

cms-bot internal usage

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47803/44398

[cmsbuild]

A new Pull Request was created by @slava77 for master.

It involves the following packages:

• CalibFormats/SiStripObjects (alca)
• RecoLocalTracker/SiStripClusterizer (reconstruction)

@atpathak, @cmsbuild, @consuegs, @jfernan2, @mandrenguyen, @perrotta can you please review it and eventually sign? Thanks.
@GiacomoSguazzoni, @VinInn, @VourMa, @alesaggio, @echabert, @felicepantaleo, @gbenelli, @gpetruc, @jlidrych, @missirol, @mmusich, @mtosi, @robervalwalsh, @rovere, @rsreds, @threus, @tocheng, @yduhm, @yuanchao this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

[slava77]

@cmsbuild please test

[cmsbuild]

+1

Size: This PR adds an extra 20KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cd5b0f/45430/summary.html
COMMIT: e6165e9
CMSSW: CMSSW_15_1_X_2025-04-07-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/47803/45430/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

• @Ma128-bit Adding GEM MEs to per-LS scope in Offline DQM #47690
• @mmusich Miscellaneous updates for modules targeting CDC reconstruction at HLT #47796
• @etzovara Developing offline JetMET DQM for Scouting jets: applying JetID criteria - complementary to PR #47328 #47721
• @patinkaew Add filter to skip events without scouting objects in ScoutingNano for ScoutingPFMonitor dataset #47739
• @Dr15Jones Only use CPUTimer in XrdRequestManager debugging #47789
• @mmusich fix TrackToTrackComparisonHists logic for cosmics #47782

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cd5b0f/45430/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cd5b0f/45430/git-merge-result

Comparison Summary

Summary:

• No significant changes to the logs found
• Reco comparison results: 4 differences found in the comparisons
• DQMHistoTests: Total files compared: 50
• DQMHistoTests: Total histograms compared: 3913985
• DQMHistoTests: Total failures: 41
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3913924
• DQMHistoTests: Total skipped: 20
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
• Checked 215 log files, 184 edm output root files, 50 DQM output files
• TriggerResults: no differences found

[perrotta]

+alca

• Reversion of speedup SiStripClusterizer(FromRaw) using ThreeThresholdAlgorithm #47061 makes sense, given the observation posted in the descritpion of this PR, and all the discussions that started in the github thread of the original PR; memory footprint was increased, and now also apparently no real "speedup" can be observed

[mmusich]

@slava77, I gather a backport PR to CMSSW_15_0_X is in order.. Please prepare that one too (with CMSSW_15_0_4 coming up later today, this was timed somewhat awkwardly).

[jfernan2]

+1

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

[elusian]

For reference, from current measurements the change in timing is of about 1 ms
Test on 15_0_4 with menu /dev/CMSSW_15_0_0/GRun/V23

• onDemand unpacker, before reverting: total=443.3 ms
• onDemand unpacker, after reverting: total=442.6 ms
• full unpacker, before reverting: total=481.3 ms, just the unpacker=32.3 ms
• full unpacker, after reverting: total=478.0 ms, just the unpacker=31.4 ms

[slava77]

For reference, from current measurements the change in timing is of about 1 ms Test on 15_0_4 with menu /dev/CMSSW_15_0_0/GRun/V23

* onDemand unpacker, before reverting: [total=443.3 ms](https://cmshlttiming.app.cern.ch/display/elusiani/CMSSW_15_0_4_CA.20250408_151729)

* onDemand unpacker, after reverting: [total=442.6 ms](https://cmshlttiming.app.cern.ch/display/elusiani/CMSSW_15_0_4_CA_january.20250408_151717)

* full unpacker, before reverting: [total=481.3 ms, just the unpacker=32.3 ms](https://cmshlttiming.app.cern.ch/display/elusiani/CMSSW_15_0_4_CA_MkFit.20250408_151140)

* full unpacker, after reverting: [total=478.0 ms, just the unpacker=31.4 ms](https://cmshlttiming.app.cern.ch/display/elusiani/CMSSW_15_0_4_CA_january.20250408_151717)

Thanks for the tests.

It seems like the full scale tests are even closer to the middle of almost no impact (still faster after reverting).
I can guess that in my single-thread tests the performance is more optimistic and fractional loss was even more significant.

[mandrenguyen]

+1