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
make EcalRingCalibrationTools thread safe (76x) #11093
make EcalRingCalibrationTools thread safe (76x) #11093
Conversation
please test |
+1 |
The tests are being triggered in jenkins. |
A new Pull Request was created by @fwyzard (Andrea Bocci) for CMSSW_7_6_X. make EcalRingCalibrationTools thread safe (76x) It involves the following packages: Calibration/Tools @cmsbuild, @mmusich, @diguida, @cerminar can you please review it and eventually sign? Thanks. |
@bmarzocc , can you check if there is an alternative, more CMS-compliant approach available ? |
tracked at #10964 |
-1 >> Compiling /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2015-09-02-1100/src/Calibration/Tools/src/GenericHouseholder.cc >> Compiling /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2015-09-02-1100/src/Calibration/Tools/src/GenericMinL3Algorithm.cc >> Compiling /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2015-09-02-1100/src/Calibration/Tools/src/HouseholderDecomposition.cc >> Compiling /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2015-09-02-1100/src/Calibration/Tools/src/InvMatrixCommonDefs.cc In file included from /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2015-09-02-1100/src/Calibration/Tools/src/EcalRingCalibrationTools.cc:1: /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2015-09-02-1100/src/Calibration/Tools/interface/EcalRingCalibrationTools.h:51:23: error: 'thread_guard' attribute requires a string [[cms::thread_guard(isInitializedFromGeometry_)]] ^ >> Compiling /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2015-09-02-1100/src/Calibration/Tools/src/InvMatrixUtils.cc >> Compiling /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2015-09-02-1100/src/Calibration/Tools/src/MinL3AlgoUniv.cc In file included from /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2015-09-02-1100/src/Calibration/Tools/src/HouseholderDecomposition.cc:7: you can see the results of the tests here: |
@Dr15Jones , do I need to use a different syntax to make LLVM/clang happy ? |
@fwyzard Is an alternative still needed? An alternative tool in CMSSW for extracting the ringID is this one: however it is not used and maintained anymore, so don't know if it works correctly with the latest CMSSW version. |
I'd let David comment if we are happy with my minimal fix, it if something
better is needed long term.
Ciao,
.Andrea
|
Am I mistaken that computing the ringID from a DetId is more than a few lines of code?
|
Yeah, the name in the thread_guard statement must be quoted |
My thoughts are what Andrea has done is fine for an emergency patch but this code violates CMSSW coding policies and we need a fix. This design is not a good long term pattern for handling threaded code. |
The tests are being triggered in jenkins. |
Sorry I missed it the first time. |
+1 |
This pull request is fully signed and it will be integrated in one of the next CMSSW_7_6_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @Degano, @smuzaffar |
+1 |
…threadsafe make EcalRingCalibrationTools thread safe (76x)
Implement a minimal set of changes to EcalRingCalibrationTools to make its initialisation and usage thead-safe.
In particular, this should make HLTEcalPhiSymFilter thread-safe again.
Note that EcalRingCalibrationTools is still non-compliant with the CMS coding rules, and should be rewritten either as an ESProducer, or as a non-static data member of an EDProducer.