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

Make history registry non-singleton and also fix read calls to not use p... #681

Merged

Conversation

wmtan
Copy link
Contributor

@wmtan wmtan commented Aug 30, 2013

Two different changes to the framework to support multithreading are being submitted together because they touch some of the same lines of code:

Change 1:
Functions that read a run, lumi, or an event principal fill in the principal that is passed in by reference. These functions no longer create the principal. Therefore, there is no need for them to return a pointer to the principal.
With this pull request, these functions now return void or bool, as appropriate. Furthermore, the functions that read a run or lumi principal were passed a shared pointer for filling. As this is no longer needed, we now pass references instead. Also, the functions readAndCacheRun and readAndCacheLumi were modified to fill in the principal rather than creating it, and were renamed readRun and readLuminosityBlock respectively, because the caching of the principal has been moved outside these functions.

Change 2:
The process history registry is redone so that it is no longer a singleton instance of the ThreadSafeRegistry template. It is now a separate class, and the primary input source, each secondary input sources, and each ROOT/EDM output module now each has its own instance of this registry.

This pull request contains and supersedes #641, which is being closed.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @wmtan for CMSSW_7_0_X.

Make history registry non-singleton and also fix read calls to not use p...

It involves the following packages:

DataFormats/Provenance
FWCore/TFWLiteSelector
IOPool/Output
FWCore/Integration
IORawData/DaqSource
DQMServices/FwkIO
IOPool/Input
FWCore/Framework
FWCore/Sources
GeneratorInterface/LHEInterface
IOPool/SecondaryInput
IOPool/Common
DataFormats/FWLite
IOPool/Streamer
Fireworks/Core

@vciulli, @Dr15Jones, @emeschi, @rovere, @deguio, @mommsen, @ktf can you please review it and eventually sign? Thanks.
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.

@Dr15Jones
Copy link
Contributor

+1

I need this change merged into CMSSW_7_0_X before I can make a pull request for additional framework changes (given that we touch the same lines of code). All these changes are purely technical so I suggest bypassing signatures from the other L2s as long as a build test and the regular tests pass. I'm building the changes now, but given the large number of packages to be compiled it would probably be better to have this done centrally.

@wmtan
Copy link
Contributor Author

wmtan commented Aug 31, 2013

I successfully built this and ran all the unit tests. The relvals are currently running, and all have passed so far.

@Dr15Jones
Copy link
Contributor

I have also successfullly built this. If Bill reports that the RelVal's all succeed, then please go ahead and bypass the signatures and merge into CMSSW_7_0_X.

@wmtan
Copy link
Contributor Author

wmtan commented Aug 31, 2013

I ran the full relval matrix in CMSSW_7_0_X_2013-08-30-1400 with my changes. All tests except 143.0 passed, Even the two tests that are highlighted in red on the IB page passed for me.
The failure in 143.0 is in a python configuration file, and I touched exactly zero lines of python in my changes. I will investigate further.

@wmtan
Copy link
Contributor Author

wmtan commented Aug 31, 2013

I reran test 143.0 and it failed in the cmsDriver step. I then reran 143.0 on the unchanged IB and got the same error.
Just to check my environment, I reran a few other relvals on the unchanged IB and they passed.
So, all relvals pass except 143.0, which also fails in the IB without any code changes. Good to go, I think.

@Dr15Jones
Copy link
Contributor

I then approve having this merged into 7_0_X

@Dr15Jones
Copy link
Contributor

@davidlt @nclopezo @smuzaffar @ktf To whom ever is handling CMSSW_7_0_X while Giulio's away, at your earliest convenience please bypass the rest of the signatures for this change (since it is purely technical) and merge it into the release.

@davidlt
Copy link
Contributor

davidlt commented Sep 1, 2013

Just for future. If two features are not entangled together, they should be in two separate branches. Trusting @Dr15Jones and after testing bypassing it. If there are any issues in IBs, please report to this ticket.

davidlt added a commit that referenced this pull request Sep 1, 2013
…reading

Make history registry non-singleton and also fix read calls to not use p...
@davidlt davidlt merged commit 991df7e into cms-sw:CMSSW_7_0_X Sep 1, 2013
@Dr15Jones
Copy link
Contributor

Trusting @Dr15Jones and after testing bypassing it. If there are any issues in IBs, please report to this ticket.

Thanks David. With the time difference it can some times be hard to figure out which IB is next. I assume this is in the 0200 build for Monday?

@davidlt
Copy link
Contributor

davidlt commented Sep 1, 2013

@Dr15Jones correct, it's 0200 Monday IB.

Dr15Jones added a commit to Dr15Jones/cmssw that referenced this pull request Sep 1, 2013
…ltipleStreamsInEventProcessor

Resolved minor overlapping change in EventProcessor.cc and SubProcess.cc. In both cases the
resolution was to use the change already in this branch.
@wmtan wmtan deleted the TwoFrameworkImprovementsForMultithreading branch September 3, 2013 16:42
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.

4 participants