[14_0_X] Removed variable-length arrays #44936
Conversation
|
A new Pull Request was created by @aehart for CMSSW_14_0_X. It involves the following packages:
@epalencia, @cmsbuild, @aloeliger can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
cms-bot internal usage |
|
@cmsbuild please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-06d76c/39314/summary.html Comparison SummarySummary:
|
| } | ||
| } | ||
| // Initialize all-false 2D vector of tracks being duplicates to other tracks | ||
| vector<vector<bool>> dupMap(numStublists, vector<bool>(numStublists, false)); |
There was a problem hiding this comment.
If symmetric and storage is an issue why not using triangular storage?
btw interesting enough vector<bool> uses one bit per entry while bool v[N] will use a full byte...
| } | ||
| } | ||
| } | ||
|
|
||
| // Check to see if the track is a duplicate | ||
| for (unsigned int itrk = 0; itrk < numStublists; itrk++) { | ||
| for (unsigned int jtrk = 0; jtrk < numStublists; jtrk++) { | ||
| if (dupMap[itrk][jtrk]) { | ||
| noMerge[itrk] = true; | ||
| if (dupMap.at(itrk).at(jtrk)) { |
There was a problem hiding this comment.
why at?
please use at only for debugging, never in production code.
| dupMap[itrk][jtrk] = true; | ||
| dupMap[jtrk][itrk] = true; | ||
| dupMap.at(itrk).at(jtrk) = true; | ||
| dupMap.at(jtrk).at(itrk) = true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Check to see if the track is a duplicate | ||
| for (unsigned int itrk = 0; itrk < numStublists; itrk++) { |
There was a problem hiding this comment.
why this loop at all?
just set noMerge[itrk] = true; where one set the dupMap.
| @@ -316,7 +308,7 @@ void PurgeDuplicate::execute(std::vector<Track>& outputtracks, unsigned int iSec | |||
| for (unsigned int itrk = 0; itrk < numStublists - 1; itrk++) { | |||
| for (unsigned int jtrk = itrk + 1; jtrk < numStublists; jtrk++) { | |||
| // Merge a track with its first duplicate found. | |||
There was a problem hiding this comment.
here is said "merge with the first duplicate found" but I no not see any break, so what prevents to merge with all other duplicates?
If really one merges with just the first found than there is no need of the dupMap: it is enough for each track to keep record of the first duplicate...
I think the whole algorithm need a review as most probably all those large maps and vectors are not needed...
cmsbuild
added
l1-approved
fully-signed
and removed
l1-pending
pending-signatures
labels
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_14_0_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_14_1_X is complete. This pull request will now be reviewed by the release team before it's merged. @rappoccio, @antoniovilela, @sextonkennedy (and backports should be raised in the release meeting by the corresponding L2) |
I think I ultimately share responsibility for this code with @tomalin and @zdemirag, and I'm happy to discuss @VinInn's comments in whatever forum makes sense. |
|
This PR looks OK to me. I've asked one of the authors of this class to check the comments made above about improving the code further. |
|
+1
|
PR description:
This PR removes all variable-length arrays in the L1Trigger/TrackFinding* packages, which turns out to just be in L1Trigger/TrackFindingTracklet/src/PurgeDuplicate.cc. This led to stack overflows, and ultimately segfaults, in CMSSW_14_1_0_pre3 on certain events, as reported in #44306 (comment).
PR validation:
With the fixes in this PR, the specific job that was reported to segfault is now able to run to completion.
If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:
Although the crash reported in #44306 (comment) is only seen in 14_1, we include this backport for safety.
Original PR: #44935