Skip to content
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

Improve L1TMuonBarrelTrackProducer #46640

Merged
merged 9 commits into from
Nov 15, 2024

Conversation

Dr15Jones
Copy link
Contributor

PR description:

  • decreased substantially the number of per event allocations done
  • made code changes to allow this module to now be a stream module
  • improved memory handling

PR validation:

Ran step3 of workflow 12861.0 with improved ModuleAllocMonitor. Previously each event processed by the module needed 85k allocations. Now only the first event of each stream uses that much and all subsequent only use 19 allocations.

This should strictly be a technical change and should have no effect on the results.

- Hold member data directly not via pointers
- Changed interface to reflect data is always available
- Fixed const correctness issues
- Removed unnecessary virtual interfaces
This required moving the header file to follow CMS coding rules.
Moved producer header info into source.
This routine was found to do an excessive number of allocations.
@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 8, 2024

cms-bot internal usage

@Dr15Jones
Copy link
Contributor Author

@makortel FYI

@Dr15Jones
Copy link
Contributor Author

please test

@Dr15Jones
Copy link
Contributor Author

The reason the allocations dropped is the code now only calls L1MuBMTFConfig::setDefaultsES if the dependent record has changed.

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 8, 2024

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 8, 2024

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

It involves the following packages:

  • L1Trigger/L1TMuonBarrel (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
Copy link
Contributor

makortel commented Nov 8, 2024

type performance-improvements

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 8, 2024

-1

Failed Tests: RelVals RelVals-INPUT AddOn
Size: This PR adds an extra 216KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8e66cc/42691/summary.html
COMMIT: ee26979
CMSSW: CMSSW_14_2_X_2024-11-08-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/46640/42691/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals

----- Begin Fatal Exception 08-Nov-2024 23:12:30 CET-----------------------
An exception of category 'StdException' occurred while
   [0] Processing  Event run: 301998 lumi: 9 event: 8792128 stream: 0
   [1] Running path 'HLTAnalyzerEndpath'
   [2] Prefetching for module L1TRawToDigi/'hltGtStage2Digis'
   [3] Prefetching for module RawDataCollectorByLabel/'rawDataCollector'
   [4] Calling method for module L1TDigiToRaw/'packGmtStage2'
Exception Message:
A std::exception was thrown.
array::at: __n (which is 6) >= _Nm (which is 6)
----- End Fatal Exception -------------------------------------------------
----- Begin Fatal Exception 08-Nov-2024 23:12:31 CET-----------------------
An exception of category 'StdException' occurred while
   [0] Processing  Event run: 274199 lumi: 21 event: 39389360 stream: 0
   [1] Running path 'HLTAnalyzerEndpath'
   [2] Prefetching for module L1TRawToDigi/'hltGtStage2Digis'
   [3] Prefetching for module RawDataCollectorByLabel/'rawDataCollector'
   [4] Calling method for module L1TDigiToRaw/'packGmtStage2'
Exception Message:
A std::exception was thrown.
array::at: __n (which is 6) >= _Nm (which is 6)
----- End Fatal Exception -------------------------------------------------
----- Begin Fatal Exception 08-Nov-2024 23:12:32 CET-----------------------
An exception of category 'StdException' occurred while
   [0] Processing  Event run: 319450 lumi: 76 event: 105676750 stream: 0
   [1] Running path 'HLTAnalyzerEndpath'
   [2] Prefetching for module L1TRawToDigi/'hltGtStage2Digis'
   [3] Prefetching for module RawDataCollectorByLabel/'rawDataCollector'
   [4] Calling method for module L1TDigiToRaw/'packGmtStage2'
Exception Message:
A std::exception was thrown.
array::at: __n (which is 6) >= _Nm (which is 6)
----- End Fatal Exception -------------------------------------------------
Expand to see more relval errors ...

RelVals-INPUT

  • 136.724136.724_RunJetHT2016B/step2_RunJetHT2016B.log
  • 136.731136.731_RunSinglePh2016B/step2_RunSinglePh2016B.log
  • 136.735136.735_RunNoBPTX2016B/step2_RunNoBPTX2016B.log
Expand to see more relval errors ...

AddOn Tests

----- Begin Fatal Exception 08-Nov-2024 23:14:06 CET-----------------------
An exception of category 'StdException' occurred while
   [0] Processing  Event run: 1 lumi: 1 event: 2 stream: 3
   [1] Running path 'RAWSIMoutput_step'
   [2] Prefetching for module PoolOutputModule/'RAWSIMoutput'
   [3] Prefetching for module RawDataCollectorByLabel/'rawDataCollector'
   [4] Calling method for module L1TDigiToRaw/'gmtStage2Raw'
Exception Message:
A std::exception was thrown.
array::at: __n (which is 6) >= _Nm (which is 6)
----- End Fatal Exception -------------------------------------------------
----- Begin Fatal Exception 08-Nov-2024 23:12:44 CET-----------------------
An exception of category 'StdException' occurred while
   [0] Processing  Event run: 272762 lumi: 51 event: 41555597 stream: 0
   [1] Running path 'RAWoutput_step'
   [2] Prefetching for module PoolOutputModule/'RAWoutput'
   [3] Prefetching for module RawDataCollectorByLabel/'rawDataCollector'
   [4] Calling method for module L1TDigiToRaw/'packGmtStage2'
Exception Message:
A std::exception was thrown.
array::at: __n (which is 6) >= _Nm (which is 6)
----- End Fatal Exception -------------------------------------------------

@Dr15Jones
Copy link
Contributor Author

I was able to reproduce the problem locally and then used git bisect to determine which commit introduced a problem. It was

ee26979 is the first bad commit

Author: Christopher Jones <[email protected]>
Date:   Fri Nov 8 14:57:31 2024 -0600

    Make L1TMuonBarrelTrackProducer a stream module
    
    No more global variables are being used by code called by the
    producer.
    Switched to using edm::Event::emplace.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Pull request #46640 was updated. @aloeliger, @atpathak, @cmsbuild, @consuegs, @epalencia, @francescobrivio, @perrotta can you please check and sign again.

@Dr15Jones
Copy link
Contributor Author

I switched on Debug output from the module and saw that the building stage was not occurring.

@Dr15Jones
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

+1

Size: This PR adds an extra 20KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8e66cc/42779/summary.html
COMMIT: 5975fee
CMSSW: CMSSW_14_2_X_2024-11-12-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/46640/42779/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

There are some workflows for which there are errors in the baseline:
2024.101001 step 1
2024.202001 step 1
2024.303001 step 1
The results for the comparisons for these workflows could be incomplete
This means most likely that the IB is having errors in the relvals.The error does NOT come from this pull request

Summary:

@perrotta
Copy link
Contributor

+1

@aloeliger
Copy link
Contributor

+l1

@cmsbuild
Copy link
Contributor

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
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit bdf7b42 into cms-sw:master Nov 15, 2024
11 checks passed
@Dr15Jones Dr15Jones deleted the improveL1TMuonBarrel branch November 19, 2024 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants