Improve L1MuGMTMatrix #27538
Improve L1MuGMTMatrix #27538
Conversation
- Use just a single contiguous memory location instead of an array of arrays. - Use std::unique_ptr<T[]> to manage the memory - Added a unit test to prove no functional changes happened
UBSAN was complaining about using the copy constructor to fill elements of a std::vector<L1MuGMTMatrix<*>> when the internal array of L1MuGMTMatrix had not been initialized.
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27538/10915
|
|
please test |
|
The tests are being triggered in jenkins. |
|
A new Pull Request was created by @Dr15Jones (Chris Jones) for master. It involves the following packages: L1Trigger/GlobalMuonTrigger @cmsbuild, @rekovic, @benkrikler can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
ping @rekovic @benkrikler |
|
@rekovic I would like to move forward with this PR. The code update looks ok to me, and the tests do not show any change. Please have a look asap, I plan to merge this PR in the coming IBs unless you have explicit objections. |
|
+1 @rekovic please review this PR for possible future comments |
|
merge |
|
hi @Dr15Jones - unfortunately L1Trigger/GlobalMuonTrigger/src defines a plugin so you can't use it in the tests here (causing a warning in release builds now) https://github.com/cms-sw/cmssw/pull/27538/files#diff-ad9585c8e3b32fba9922df3e129141f8R17 |
|
It would seem to me that the correct solution is to move the plugin out of /src and into a new /plugin directory (I'm on vacation today and for the next few days). |
|
@davidlange6 see #27828 |
PR description:
Not initializing the matrix elements in the constructor was causing a complaint by UBSAN.
PR validation:
Code compiles and the new unit test passes.
This is a technical change and should have no impact on the results. It should take less memory and be faster.