Improve BDT in L1Trigger/L1TMuonEndCap #47599

Dr15Jones · 2025-03-14T15:22:15Z

PR description:

fixed memory corruption problems which could lead to double deletes or deletion of random memory locations
memory handling now done via std::unique_ptr
improved const correctness

This deletion of memory coming from an uninitialized pointer was found by gcc 14.

PR validation:

Code compiles and local tests pass.

resolves cms-sw/framework-team#1307

cmsbuild · 2025-03-14T15:22:42Z

cms-bot internal usage

Dr15Jones · 2025-03-14T15:23:44Z

please test

cmsbuild · 2025-03-14T15:23:52Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47599/44099

There are other open Pull requests which might conflict with changes you have proposed:
- File L1Trigger/L1TMuonEndCap/interface/bdt/Tree.h modified in PR(s): [GCC14] Fix compiler warnings #47556
- File L1Trigger/L1TMuonEndCap/src/bdt/Tree.cc modified in PR(s): [GCC14] Fix compiler warnings #47556

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47599/44099/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47599/44099/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

Dr15Jones · 2025-03-14T15:26:04Z

L1Trigger/L1TMuonEndCap/src/bdt/Node.cc

-
-Node::~Node() {
-  // Recursively delete all nodes in the tree.
-  if (leftDaughter)


NOTE: using the defaultly made Node(Node&&) with this implementation of a destructor leads to double delete of leftDaughter and rightDaughter as the default Node(Node&&) will not reset the pointers to nullptr.

Dr15Jones · 2025-03-14T15:28:32Z

L1Trigger/L1TMuonEndCap/src/bdt/Tree.cc

@@ -135,24 +121,14 @@ void Tree::findLeafs(Node* local_root, std::list<Node*>& tn) {
    findLeafs(local_root->getRightDaughter(), tn);
 }

-Tree::Tree(Tree&& tree) {
-  if (rootNode)


This is the problem spotted by gcc 14. rootNode is uninitialized at this point so the following line deletes some random address.

Dr15Jones · 2025-03-14T15:30:41Z

please test

cmsbuild · 2025-03-14T15:31:23Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47599/44100

There are other open Pull requests which might conflict with changes you have proposed:
- File L1Trigger/L1TMuonEndCap/interface/bdt/Tree.h modified in PR(s): [GCC14] Fix compiler warnings #47556
- File L1Trigger/L1TMuonEndCap/src/bdt/Tree.cc modified in PR(s): [GCC14] Fix compiler warnings #47556

cmsbuild · 2025-03-14T15:31:47Z

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

It involves the following packages:

L1Trigger/L1TMuonEndCap (l1)

@aloeliger, @epalencia can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @dinyar, @eyigitba, @missirol, @mmusich, @thomreis 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

makortel · 2025-03-14T15:34:00Z

(just to remind, the code improved in this PR is uses substantial amount of memory as reported in #42526)

cmsbuild · 2025-03-14T17:52:58Z

+1

Size: This PR adds an extra 36KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c88bb7/44981/summary.html
COMMIT: 96be2a4
CMSSW: CMSSW_15_1_X_2025-03-14-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/47599/44981/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially added 1 lines to the logs
Reco comparison results: 8 differences found in the comparisons
DQMHistoTests: Total files compared: 50
DQMHistoTests: Total histograms compared: 3927607
DQMHistoTests: Total failures: 24
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3927563
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

BenjaminRS · 2025-03-19T14:22:13Z

Hello @Dr15Jones -- @quinnanm and I are starting to take over from @aloeliger as L1T OffSW coordinators. Are you planning on rebasing this PR to resolve the conflicts? I talked to @eyigitba who said the EMTF team would test custom BDT loading with your modifications.
Thanks,
Benjamin

Dr15Jones · 2025-03-19T15:00:28Z

I can rebase

- fixed memory corruption problems which could lead to double deletes or deletion of random memory locations - memory handling now done via std::unique_ptr - improved const correctness

cmsbuild · 2025-03-19T16:05:26Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47599/44149

cmsbuild · 2025-03-19T16:05:44Z

Pull request #47599 was updated. @BenjaminRS, @cmsbuild, @quinnanm can you please check and sign again.

Dr15Jones · 2025-03-19T16:06:58Z

please test

cmsbuild · 2025-03-19T20:26:24Z

+1

Size: This PR adds an extra 20KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c88bb7/45075/summary.html
COMMIT: 759c430
CMSSW: CMSSW_15_1_X_2025-03-19-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/47599/45075/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:

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

Comparison Summary

Summary:

You potentially removed 1 lines from the logs
Reco comparison results: 2 differences found in the comparisons
DQMHistoTests: Total files compared: 50
DQMHistoTests: Total histograms compared: 3928069
DQMHistoTests: Total failures: 64
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3927985
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

BenjaminRS · 2025-03-21T16:19:41Z

+l1
EMTF team have done a test with custom BDT loading before and after this fix: everything looks ok.

cmsbuild · 2025-03-21T16:20:05Z

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

mandrenguyen · 2025-03-22T09:04:42Z

+1

cmsbuild added this to the CMSSW_15_1_X milestone Mar 14, 2025

cmsbuild added l1-pending pending-signatures tests-pending orp-pending code-checks-pending labels Mar 14, 2025

cmsbuild added tests-started code-checks-rejected and removed tests-pending code-checks-pending labels Mar 14, 2025

Dr15Jones commented Mar 14, 2025

View reviewed changes

Dr15Jones force-pushed the fixL1TMuonEndCapBDT branch from 7e93e60 to 96be2a4 Compare March 14, 2025 15:29

cmsbuild added tests-pending code-checks-pending and removed tests-started code-checks-rejected labels Mar 14, 2025

cmsbuild added tests-started and removed tests-pending labels Mar 14, 2025

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 14, 2025

cmsbuild added tests-approved and removed tests-started labels Mar 14, 2025

Improve BDT in L1Trigger/L1TMuonEndCap

759c430

- fixed memory corruption problems which could lead to double deletes or deletion of random memory locations - memory handling now done via std::unique_ptr - improved const correctness

Dr15Jones force-pushed the fixL1TMuonEndCapBDT branch from 96be2a4 to 759c430 Compare March 19, 2025 16:03

cmsbuild added tests-pending code-checks-pending and removed tests-approved code-checks-approved labels Mar 19, 2025

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 19, 2025

cmsbuild added tests-started and removed tests-pending labels Mar 19, 2025

cmsbuild added tests-approved and removed tests-started labels Mar 19, 2025

cmsbuild added l1-approved fully-signed and removed l1-pending pending-signatures labels Mar 21, 2025

cmsbuild added orp-approved and removed orp-pending labels Mar 22, 2025

cmsbuild merged commit 7bfefad into cms-sw:master Mar 22, 2025
11 checks passed

Dr15Jones deleted the fixL1TMuonEndCapBDT branch April 4, 2025 13:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve BDT in L1Trigger/L1TMuonEndCap #47599

Improve BDT in L1Trigger/L1TMuonEndCap #47599

Dr15Jones commented Mar 14, 2025 •

edited

Loading

cmsbuild commented Mar 14, 2025 •

edited

Loading

Dr15Jones commented Mar 14, 2025

cmsbuild commented Mar 14, 2025

Dr15Jones Mar 14, 2025

Dr15Jones Mar 14, 2025

Dr15Jones commented Mar 14, 2025

cmsbuild commented Mar 14, 2025

cmsbuild commented Mar 14, 2025

makortel commented Mar 14, 2025

cmsbuild commented Mar 14, 2025

BenjaminRS commented Mar 19, 2025

Dr15Jones commented Mar 19, 2025

cmsbuild commented Mar 19, 2025

cmsbuild commented Mar 19, 2025

Dr15Jones commented Mar 19, 2025

cmsbuild commented Mar 19, 2025

BenjaminRS commented Mar 21, 2025

cmsbuild commented Mar 21, 2025

mandrenguyen commented Mar 22, 2025

Improve BDT in L1Trigger/L1TMuonEndCap #47599

Improve BDT in L1Trigger/L1TMuonEndCap #47599

Conversation

Dr15Jones commented Mar 14, 2025 • edited Loading

PR description:

PR validation:

cmsbuild commented Mar 14, 2025 • edited Loading

Dr15Jones commented Mar 14, 2025

cmsbuild commented Mar 14, 2025

Dr15Jones Mar 14, 2025

Choose a reason for hiding this comment

Dr15Jones Mar 14, 2025

Choose a reason for hiding this comment

Dr15Jones commented Mar 14, 2025

cmsbuild commented Mar 14, 2025

cmsbuild commented Mar 14, 2025

makortel commented Mar 14, 2025

cmsbuild commented Mar 14, 2025

Comparison Summary

BenjaminRS commented Mar 19, 2025

Dr15Jones commented Mar 19, 2025

cmsbuild commented Mar 19, 2025

cmsbuild commented Mar 19, 2025

Dr15Jones commented Mar 19, 2025

cmsbuild commented Mar 19, 2025

Comparison Summary

BenjaminRS commented Mar 21, 2025

cmsbuild commented Mar 21, 2025

mandrenguyen commented Mar 22, 2025

Dr15Jones commented Mar 14, 2025 •

edited

Loading

cmsbuild commented Mar 14, 2025 •

edited

Loading