Navigation Menu

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

Remove dead code and apply one fix in DQM/L1TMonitor #34794

Merged

Conversation

perrotta
Copy link
Contributor

@perrotta perrotta commented Aug 5, 2021

PR description:

The static analyzer is pointing to some unused code and dead initializations

While fixing them, I hit an obvious bug in previous L452 of DQM/L1TMonitor/src/L1TBPTX.cc where the numerator was filled twice, and none the numerator.

This makes me wonder whether this code was ever used... Anyhow, here is the fix.

PR validation:

It builds

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 5, 2021

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34794/24500

  • This PR adds an extra 20KB to repository

if (algoFired) {
m_missFireNumerator[triggerName]++;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the bug that is fixed

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 5, 2021

A new Pull Request was created by @perrotta (Andrea Perrotta) for master.

It involves the following packages:

  • DQM/L1TMonitor (dqm)

@andrius-k, @kmaeshima, @ErnestaP, @ahmad3213, @cmsbuild, @jfernan2, @rvenditti can you please review it and eventually sign? Thanks.
@silviodonato, @dpiparo, @qliphy, @perrotta you are the release manager for this.

cms-bot commands are listed here

@perrotta
Copy link
Contributor Author

perrotta commented Aug 5, 2021

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 5, 2021

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7ba697/17569/summary.html
COMMIT: 0d8acbd
CMSSW: CMSSW_12_1_X_2021-08-04-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/34794/17569/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 39
  • DQMHistoTests: Total histograms compared: 2999410
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2999387
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
  • Checked 165 log files, 37 edm output root files, 39 DQM output files
  • TriggerResults: no differences found

@qliphy
Copy link
Contributor

qliphy commented Aug 10, 2021

@cms-sw/dqm-l2 any comment?

@jfernan2
Copy link
Contributor

+1

@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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@perrotta
Copy link
Contributor Author

+1

@cmsbuild cmsbuild merged commit 9ac280b into cms-sw:master Aug 10, 2021
@perrotta
Copy link
Contributor Author

Thank you @jfernan2 : did you get any idea about whether DQM/L1TMonitor/src/L1TBPTX.cc is ever used? Why the bug went unnoticed?

@perrotta perrotta deleted the fixesAndDeadCodeRemovalInDqmL1tmonitor branch August 10, 2021 17:21
@jfernan2
Copy link
Contributor

It seems to me it is used in:


for Online DQM but then removed in the clients.
process.l1tMonitorOnline.remove(process.l1tBPTX)

process.l1tMonitorStage1Online.remove(process.l1tBPTX)

So, I am afraid it is only used in local tests. Hence, most likely not used any longer.

Let me include here L1T DQM contacts for confirmation:
@abrinke1 @apana @chadfreer @pgianneios @gkaratha @panoskatsoulis @benkrikler @aloeliger @vukasinmilosevic @dinyar @thomreis @rekovic @stahlleiton @hftsoi @mzarucki

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.

None yet

4 participants