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
Speedup strip digitization #40618
Speedup strip digitization #40618
Conversation
adcPrev2 = in[i - 2].adc(); | ||
thePrev2FEDlowThresh = static_cast<int16_t>( | ||
threshold.getData(strip - 2, detThRange).getLth() * noise.getNoiseFast(strip - 2, detNoiseRange) + 0.5); | ||
ldata.statPrev2 = stat[i - 2]; | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the old logic below was wrong in case say adcNext <= adcPrev
but prev is below and next is above
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40618/33891
|
A new Pull Request was created by @VinInn (Vincenzo Innocente) for master. It involves the following packages:
@cmsbuild, @civanch, @mandrenguyen, @clacaputo, @mdhildreth can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
please test |
@civanch Should we enable the profiling? |
@mandrenguyen , we establish CPU/memory/profiling of top of some IB, we are doing this for pre-releases. Vincenzo made CPU check in unit test with the factor 2 for this method, what will be summary effect we do may try to measure privately or if we ask Natascha for her standard analysis of performance. |
-1 Failed Tests: RelVals-INPUT AddOn RelVals-INPUT
AddOn Tests
Expand to see more addon errors ...Comparison SummarySummary:
|
Is there anything blocking integration (and/or further review)? |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40618/34179
|
Pull request #40618 was updated. @civanch, @mandrenguyen, @clacaputo, @mdhildreth can you please check and sign again. |
-1 Failed Tests: RelVals-INPUT RelVals-INPUTThe relvals timed out after 4 hours. Comparison SummarySummary:
|
"The relvals timed out after 4 hours." |
I've seen it plenty of times in other PRs, so I think at this stage it can be ignored. |
please test |
Such timeouts happens and I do not know other solution than resubmit the test. |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-267794/30643/summary.html Comparison SummarySummary:
|
+1 some differences in comparisons are expected. |
+1 |
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) |
release-notes: expected minor regressions from fix of ZeroSuppression algorithm |
+1 |
Speed up of Strip digitization
a)
ZeroSuppressor done in two loops: one compares adc with thresholds, the second performs the algo logic.
(it fixes also a defect in the previous implementation (fix done ONLY for the digitizer version))
speed up x2
b) remove maps and various copies in PreMixingSiStripWorker
c) optimize and make consistent math in single precision
igprof
original
http://innocent.web.cern.ch/innocent/perfResults/igprof-navigator/digiDebug1T_ori_CMSSW_13_0_X_2023-01-24-1100_slc7_amd64_gcc11/24
this PR
http://innocent.web.cern.ch/innocent/perfResults/igprof-navigator/digiDebug1T_speedStrip5_CMSSW_13_0_X_2023-01-24-1100_slc7_amd64_gcc11/26
purely technical, regression expected due to the bug fix.
I'm under the impression that using a single-precision random number generator may speed up all those gaussian noise generators by a large amount.
I think that this would require a dedicated "project".