promoting as a member of DQMStore and remove static #4756

deguio · 2014-07-23T12:21:02Z

No description provided.

cmsbuild · 2014-07-23T12:22:48Z

A new Pull Request was created by @deguio for CMSSW_7_2_X.

promoting as a member of DQMStore and remove static

It involves the following packages:

DQMServices/Core

@ojeda, @danduggan, @rovere, @cmsbuild, @nclopezo, @deguio, @Degano can you please review it and eventually sign? Thanks.
@barvic this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.

cmsbuild · 2014-07-23T12:53:27Z

Pull request #4756 was updated. @ojeda, @danduggan, @rovere, @cmsbuild, @nclopezo, @deguio, @Degano can you please check and sign again.

Dr15Jones · 2014-07-23T13:24:32Z

-1 I'm afraid this isn't thread safe and functions in Services do need to be thread safe.
The problem is two threads could be trying to set the pointer at the same time. I'll put the needed changes as comments to the appropriate lines of code.

Dr15Jones · 2014-07-23T13:24:58Z

DQMServices/Core/interface/DQMStore.h

@@ -19,6 +19,7 @@
 # include <stdio.h>
 # include <stdlib.h>
 # include <cxxabi.h>
+# include <iosfwd>


#include <atomic>

rovere · 2014-07-23T13:31:30Z

Ciao Chris,
of course I agree w/ your changes, yet my understanding is that:

there will be 1 and only 1 instance of services (hence also of DQMStore), even if we run with several streams on several threads ==> no multiple constructors could ever be called at the same time: is this assumption correct?
access to the print_trace function that uses the stream_ is protected by the lock in the iBooker interface.

Yet, as I said, it's better to be explicitly thread safe, rather than implicitely

thanks,
M.

Dr15Jones · 2014-07-23T13:36:26Z

We only create one instance of a Service and therefore you only have one constructor. When dealing with SubProcesses, however, it is possible to get multiple instances of a Service on different Processes, however I think the Service must at compile time explicitly tell the Service system it wants to be duplicated.
If it is protected by iBooker then there is no need for the std::atomic. However, not all modules are using iBooker yet, isn't that correct? The reason is although setting stream_ is safe with the changes I made, calling methods of stream_ are not safe since I don't think std::ofstream itself is thread safe and therefore it requires a serialization mechanism to protect it.

rovere · 2014-07-23T13:41:37Z

Ciao Chris,
regarding point 2, the lock in iBooker will be hold until after the booking is finished and all calls to print_trace have returned, so I do not see concurrency problems here, right?

Dr15Jones · 2014-07-23T13:58:03Z

Ciao Marco,
The problem is with legacy DQM modules which book during the Event processing. Searching for 'print_trace' in today's static analyzer output:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/ib-static-analysis/CMSSW_7_2_X_2014-07-23-0200/slc6_amd64_gcc481/reports/modules2statics.txt

I found
In call stack ' BeamMonitor::analyze() calls function DQMStore::book1D() calls function DQMStore::book1D() calls function DQMStore::book() calls function DQMStore::print_trace() static variable DQMStore::print_trace()::stream' is accessed , 'BeamMonitor::analyze()' overrides 'edm::EDAnalyzer::analyze() virtual'

In call stack ' CastorMonitorModule::analyze() calls function CastorLEDMonitor::processEvent() calls function CastorLEDMonitor::perChanHists() calls function DQMStore::book1D() calls function DQMStore::book1D() calls function DQMStore::book() calls function DQMStore::print_trace() static variable DQMStore::print_trace()::stream' is accessed , 'CastorMonitorModule::analyze()' overrides 'edm::EDAnalyzer::analyze() virtual'

In call stack ' DTDigiForNoiseTask::analyze() calls function DTDigiForNoiseTask::bookHistos() calls function DQMStore::book2D() calls function DQMStore::book2D() calls function DQMStore::book() calls function DQMStore::print_trace() static variable DQMStore::print_trace()::stream' is accessed , 'DTDigiForNoiseTask::analyze()' overrides 'edm::EDAnalyzer::analyze() virtual'

In call stack ' DTDigiTask::analyze() calls function DTDigiTask::bookHistos() calls function DQMStore::book2D() calls function DQMStore::book2D() calls function DQMStore::book() calls function DQMStore::print_trace() static variable DQMStore::print_trace()::stream' is accessed , 'DTDigiTask::analyze()' overrides 'edm::EDAnalyzer::analyze() virtual'

In call stack ' DTEfficiencyTask::analyze() calls function DTEfficiencyTask::fillHistos() calls function DTEfficiencyTask::bookHistos() calls function DQMStore::book1D() calls function DQMStore::book1D() calls function DQMStore::book() calls function DQMStore::print_trace() static variable DQMStore::print_trace()::stream' is accessed , 'DTEfficiencyTask::analyze()' overrides 'edm::EDAnalyzer::analyze() virtual'

In call stack ' DTLocalTriggerTask::analyze() calls function DTLocalTriggerTask::runDCCAnalysis() calls function DTLocalTriggerTask::bookHistos() calls function DQMStore::book2D() calls function DQMStore::book2D() calls function DQMStore::book() calls function DQMStore::print_trace() static variable DQMStore::print_trace()::stream' is accessed , 'DTLocalTriggerTask::analyze()' overrides 'edm::EDAnalyzer::analyze() virtual'

In call stack ' DTTestPulsesTask::analyze() calls function DTTestPulsesTask::bookHistos() calls function DQMStore::bookProfile() calls function DQMStore::bookProfile() calls function DQMStore::book() calls function DQMStore::print_trace() static variable DQMStore::print_trace()::stream' is accessed , 'DTTestPulsesTask::analyze()' overrides 'edm::EDAnalyzer::analyze() virtual'

There are more but I stopped after the 'D's. Now because these are legacy modules, the framework will only execute one at a time so there is no danger of simulateous calls to DQMStore::print_trace from them. However, in the highly unlikely event that someone books DQMStore histograms using the old interface and changed their modules to be a stream module then we'd have a threading problem.

rovere · 2014-07-23T14:16:06Z

Ciao Chris,
indeed you are right and the situation is fishy at the moment. Yet our final goal is to shutdown completely the old interface of DQMStore/booking once all modules will be converted. So in this intermediate situation we could have problems, I agree, yet maybe we should code for the future and not too much for the present? I'd avoid complexity if in the end it will not be needed. Or we can add complexity now and remove it after the transition is over.

Especially in this case, since this piece of code is used for "private" debugging from the DQM Team, never in production.

I'll leave the choice to Fede (and you, of course, since you already gave -1 ;( )

Ciao e grazie,
M.

Dr15Jones · 2014-07-23T14:18:37Z

My -1 doesn't mean anything to our integration system.
Given the discussion, I don't think it is worth changing the code to use std::atomic<>. However, it would be good to comment the code where stream_ is set to say why it is thread safe (i.e. iBooker protects it).

deguio · 2014-07-23T16:30:15Z

+1
thanks Marco and Chris. I have added a comment to the code.

cmsbuild · 2014-07-23T16:32:49Z

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_2_X IBs unless changes or unless it breaks tests.

cmsbuild · 2014-07-23T22:14:36Z

-1

Tested at: a1ba043
I ran the usual tests and I found errors in the following unit tests:

---> test DQMQualityTestsExample had ERRORS
---> test TestDQMServicesFwkIOScripts had ERRORS

you can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4756/440/summary.html

cmsbuild · 2014-07-23T22:17:50Z

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_2_X IBs unless changes (but tests are reportedly failing).

cmsbuild · 2014-07-24T10:43:16Z

-1

Tested at: a1ba043
I ran the usual tests and I found errors in the following unit tests:

---> test DQMQualityTestsExample had ERRORS
---> test TestDQMServicesFwkIOScripts had ERRORS

you can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4756/454/summary.html

cmsbuild · 2014-07-28T11:40:44Z

Pull request #4756 was updated. @ojeda, @danduggan, @rovere, @cmsbuild, @nclopezo, @deguio, @Degano can you please check and sign again.

cmsbuild · 2014-07-28T13:03:15Z

+1
Tested at: 7a59c31
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4756/494/summary.html

cmsbuild · 2014-07-28T13:31:20Z

+1
Tested at: 7a59c31
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4756/493/summary.html

cmsbuild · 2014-07-28T15:02:04Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4756/494/summary.html

cmsbuild · 2014-07-28T15:58:08Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4756/493/summary.html

deguio · 2014-07-29T14:29:12Z

+1

cmsbuild · 2014-07-29T14:32:50Z

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_2_X IBs unless changes (tests are also fine).

DQMServices -- promoting as a member of DQMStore and remove static

promoting as a member of DQMStore and remove static

d0a84b7

deguio added comparison-pending labels Jul 23, 2014

nullptr+close+delete

2efd3e7

Dr15Jones reviewed Jul 23, 2014
View reviewed changes

added comments to the code

a1ba043

deguio added comparison-pending labels Jul 23, 2014

deguio added the tests-rejected label Jul 23, 2014

fix crash if stream_ is not set

7a59c31

deguio added comparison-pending labels Jul 28, 2014

deguio added comparison-available labels Jul 28, 2014

deguio added comparison-available labels Jul 29, 2014

nclopezo added a commit that referenced this pull request Jul 29, 2014

Merge pull request #4756 from deguio/fixStreamDQMStore

a7c6ae4

DQMServices -- promoting as a member of DQMStore and remove static

nclopezo merged commit a7c6ae4 into cms-sw:CMSSW_7_2_X Jul 29, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

promoting as a member of DQMStore and remove static #4756

promoting as a member of DQMStore and remove static #4756

deguio commented Jul 23, 2014

cmsbuild commented Jul 23, 2014

cmsbuild commented Jul 23, 2014

Dr15Jones commented Jul 23, 2014

Dr15Jones Jul 23, 2014

rovere commented Jul 23, 2014

Dr15Jones commented Jul 23, 2014

rovere commented Jul 23, 2014

Dr15Jones commented Jul 23, 2014

rovere commented Jul 23, 2014

Dr15Jones commented Jul 23, 2014

deguio commented Jul 23, 2014

cmsbuild commented Jul 23, 2014

cmsbuild commented Jul 23, 2014

cmsbuild commented Jul 23, 2014

cmsbuild commented Jul 24, 2014

cmsbuild commented Jul 28, 2014

cmsbuild commented Jul 28, 2014

cmsbuild commented Jul 28, 2014

cmsbuild commented Jul 28, 2014

cmsbuild commented Jul 28, 2014

deguio commented Jul 29, 2014

cmsbuild commented Jul 29, 2014

promoting as a member of DQMStore and remove static #4756

promoting as a member of DQMStore and remove static #4756

Conversation

deguio commented Jul 23, 2014

cmsbuild commented Jul 23, 2014

cmsbuild commented Jul 23, 2014

Dr15Jones commented Jul 23, 2014

Dr15Jones Jul 23, 2014

Choose a reason for hiding this comment

rovere commented Jul 23, 2014

Dr15Jones commented Jul 23, 2014

rovere commented Jul 23, 2014

Dr15Jones commented Jul 23, 2014

rovere commented Jul 23, 2014

Dr15Jones commented Jul 23, 2014

deguio commented Jul 23, 2014

cmsbuild commented Jul 23, 2014

cmsbuild commented Jul 23, 2014

cmsbuild commented Jul 23, 2014

cmsbuild commented Jul 24, 2014

cmsbuild commented Jul 28, 2014

cmsbuild commented Jul 28, 2014

cmsbuild commented Jul 28, 2014

cmsbuild commented Jul 28, 2014

cmsbuild commented Jul 28, 2014

deguio commented Jul 29, 2014

cmsbuild commented Jul 29, 2014