ECAL : corrections in the condition tools code : ThreadSafety, Dead store #36293

depasse · 2021-11-30T09:18:52Z

PR description:

ECAL : corrections in the condition tools code : dead assignment, dead initialization, non-const static variables

PR validation:

Done with success :
$ scram build code-checks
$ scram build code-format
$ scram build runtests

if this PR is a backport please specify the original PR and why you need to backport that PR:

No backport

cmsbuild · 2021-11-30T09:26:39Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36293/27002

This PR adds an extra 68KB to repository

cmsbuild · 2021-11-30T09:27:02Z

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

It involves the following packages:

CondTools/Ecal (db)

@cmsbuild, @ggovi, @tvami, @malbouis, @francescobrivio can you please review it and eventually sign? Thanks.
@rchatter, @simonepigazzini, @mmusich, @thomreis, @argiro this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

thomreis · 2021-11-30T10:20:51Z

CondTools/Ecal/src/EcalDCSHandler.cc

@@ -272,6 +272,8 @@ bool popcon::EcalDCSHandler::insertHVDataSetToOffline(const std::map<EcalLogicID
        ex_y[4] = 12;
        ex_x[5] = 6;
        ex_y[5] = 11;
+      } else {
+        std::cout << "Dee number not known !" << std::endl;


This will not fix the issue about ex_x[l] containing garbage since the array is still not defined. And indeed the same is true for ex_y[l] as far as I can see even though the SA does not flag it.

Since we have only four Dees in CMS I think you could just throw an exception since clearly something is wrong in this case.

thomreis · 2021-11-30T10:31:07Z

CondTools/Ecal/src/EcalDCSHandler.cc

+  // if (lv_off_dbstatus > 0)
+  //   lv_off_dbstatus = 1;
+  // if (lv_nomi_off_dbstatus > 0)
+  //   lv_nomi_off_dbstatus = 1;


Remove the commented out code if it is not needed.

cmsbuild · 2021-11-30T15:56:09Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36293/27025

This PR adds an extra 68KB to repository

cmsbuild · 2021-11-30T15:56:29Z

Pull request #36293 was updated. @cmsbuild, @ggovi, @tvami, @malbouis, @francescobrivio can you please check and sign again.

tvami · 2021-11-30T16:20:04Z

@cmsbuild , please test

mmusich · 2021-11-30T16:21:37Z

the title of this PR is extremely generic. For the benefit of the readers of the future release notes would it be possible to state explicitly this is about ECAL condition tools?

cmsbuild · 2021-11-30T19:25:11Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ccb5de/20873/summary.html
COMMIT: b474817
CMSSW: CMSSW_12_2_X_2021-11-29-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36293/20873/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: 41
DQMHistoTests: Total histograms compared: 3042214
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3042192
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 40 files compared)
Checked 175 log files, 37 edm output root files, 41 DQM output files
TriggerResults: no differences found

tvami · 2021-11-30T19:31:29Z

Hi @depasse yes, please change the title of the PR.
@thomreis please note that there is a legacy module used in EcalPedestalHistory.h https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ccb5de/20873/llvm-analysis/
(not for this PR, just mentioning)

thomreis · 2021-12-01T08:36:17Z

@tvami yes the legacy module is going to be fixed in a different PR to be made shortly.

ggovi · 2021-12-01T13:09:43Z

+1

cmsbuild · 2021-12-01T13:10:24Z

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 · 2021-12-01T14:02:57Z

This PR is supposed to improve Thread Safety of the code involved.
Still, I see several classes (at least EcalChannelStatusHandler, EcalErrorMask) which have a lot of cout's in them.
What are these classes intended to? If they have to be thread safe, a migration to thread safe Log messages is also in order.

thomreis · 2021-12-01T16:57:12Z

This PR is to fix Static Analyzer issues. Some of those concern thread safety in the form of non-const static variables. Those should be fixed with the current changes. The modules themselves do not need to be thread safe.

perrotta · 2021-12-01T16:59:32Z

+1

depasse · 2021-12-01T17:22:02Z

Hi, I had begun to change the std::cout by edm:LogInfo but there are tens of such structures in this PR files. So if well understood, I stop there. Best regards.

…

________________________________ From: Andrea Perrotta ***@***.*** Sent: 01 December 2021 17:59 To: cms-sw/cmssw Cc: Pierre Depasse; Mention Subject: Re: [cms-sw/cmssw] ECAL : corrections in the condition tools code : ThreadSafety, Dead store (PR #36293) +1 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#36293 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABDIEW3RSCXTAAFE4G2HBATUOZH75ANCNFSM5JBIOFYQ>. Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

tvami · 2021-12-01T17:40:07Z

@depasse if you have things done, let's not stash them, but let's open a new PR, it could be a bigger dedicated one that concentrates on the cout-s.

Corrections in the code : ThreadSafety, Dead store

d1fc873

cmsbuild added this to the CMSSW_12_2_X milestone Nov 30, 2021

cmsbuild added code-checks-pending db-pending orp-pending pending-signatures tests-pending labels Nov 30, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 30, 2021

thomreis reviewed Nov 30, 2021

View reviewed changes

Corrections in the code : ThreadSafety, Dead store

b474817

cmsbuild added code-checks-pending and removed code-checks-approved labels Nov 30, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 30, 2021

cmsbuild added tests-started and removed tests-pending labels Nov 30, 2021

cmsbuild added tests-approved and removed tests-started labels Nov 30, 2021

depasse changed the title ~~Corrections in the code : ThreadSafety, Dead store~~ ECAL : corrections in the condition tools code : ThreadSafety, Dead store Dec 1, 2021

cmsbuild added db-approved fully-signed and removed db-pending pending-signatures labels Dec 1, 2021

cmsbuild added orp-approved and removed orp-pending labels Dec 1, 2021

cmsbuild merged commit cfd502d into cms-sw:master Dec 1, 2021

This was referenced Dec 1, 2021

Update Run3 offline data GT and mcRun3 GTs with final DD4HEP #36326

Merged

Move to root 6.24 cms-sw/cmsdist#7445

Merged

depasse deleted the CodeCorrections branch November 9, 2023 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ECAL : corrections in the condition tools code : ThreadSafety, Dead store #36293

ECAL : corrections in the condition tools code : ThreadSafety, Dead store #36293

depasse commented Nov 30, 2021 •

edited

Loading

cmsbuild commented Nov 30, 2021

cmsbuild commented Nov 30, 2021

thomreis Nov 30, 2021

thomreis Nov 30, 2021

cmsbuild commented Nov 30, 2021

cmsbuild commented Nov 30, 2021

tvami commented Nov 30, 2021

mmusich commented Nov 30, 2021

cmsbuild commented Nov 30, 2021

tvami commented Nov 30, 2021

thomreis commented Dec 1, 2021

ggovi commented Dec 1, 2021

cmsbuild commented Dec 1, 2021

perrotta commented Dec 1, 2021

thomreis commented Dec 1, 2021

perrotta commented Dec 1, 2021

depasse commented Dec 1, 2021 via email

tvami commented Dec 1, 2021

ECAL : corrections in the condition tools code : ThreadSafety, Dead store #36293

ECAL : corrections in the condition tools code : ThreadSafety, Dead store #36293

Conversation

depasse commented Nov 30, 2021 • edited Loading

PR description:

PR validation:

if this PR is a backport please specify the original PR and why you need to backport that PR:

cmsbuild commented Nov 30, 2021

cmsbuild commented Nov 30, 2021

thomreis Nov 30, 2021

Choose a reason for hiding this comment

thomreis Nov 30, 2021

Choose a reason for hiding this comment

cmsbuild commented Nov 30, 2021

cmsbuild commented Nov 30, 2021

tvami commented Nov 30, 2021

mmusich commented Nov 30, 2021

cmsbuild commented Nov 30, 2021

Comparison Summary

tvami commented Nov 30, 2021

thomreis commented Dec 1, 2021

ggovi commented Dec 1, 2021

cmsbuild commented Dec 1, 2021

perrotta commented Dec 1, 2021

thomreis commented Dec 1, 2021

perrotta commented Dec 1, 2021

depasse commented Dec 1, 2021 via email

tvami commented Dec 1, 2021

depasse commented Nov 30, 2021 •

edited

Loading