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

Revert MuonDetLayerGeometry using modules to stream modules #3896

Conversation

Dr15Jones
Copy link
Contributor

The MuonDetLayerGeometry class was made const thread-safe so it is safe to make the modules which use it stream modules. In addition, performance tests of the threaded framework were showing some of these modules were causing bottlenecks when they were legacy modules.

The static analyzer was complaining that sortSegRadius was non-const.
Changed this to const and put it in an anonymous namespace.
The MuonDetLayerGeometry class was made const thread safe so it
is safe to make the modules which use it stream modules. In addition,
performance tests of the threaded framework were showing some of
these modules were causing bottlenecks when they were legacy modules.
@cmsbuild
Copy link
Contributor

A new Pull Request was created by @Dr15Jones (Chris Jones) for CMSSW_7_1_X.

Revert MuonDetLayerGeometry using modules to stream modules

It involves the following packages:

RecoMuon/CosmicMuonProducer
RecoMuon/GlobalMuonProducer
RecoMuon/GlobalTrackingTools
RecoMuon/MuonIdentification
RecoMuon/MuonSeedGenerator
RecoMuon/StandAloneMuonProducer

@nclopezo, @cmsbuild, @anton-a, @thspeer, @slava77, @Degano can you please review it and eventually sign? Thanks.
@bachtis 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.
@nclopezo, @ktf you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@cmsbuild
Copy link
Contributor

-1
I found an error when building:

class lumi::HLTInfo
class std::vectorlumi::HLTInfo
class lumi::BunchCrossingInfo
class std::vectorlumi::BunchCrossingInfo
class lumi::LumiSectionData
gmake: **\* [There are compilation/build errors. Please see the detail log above.] Error 2


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

@nclopezo
Copy link
Contributor

Sorry I accidentally used 7_0_X. I will start the tests again.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

SETMuonSeedProducer had to be downgraded to a legacy module since
it uses the thread-unsafe CLHEP matricies in SETFilter::findParabolaMinimum.
@Dr15Jones
Copy link
Contributor Author

The CLHEP matrix problem was found by helgrind. It was the only issue found related to this pull request.

@cmsbuild
Copy link
Contributor

Pull request #3896 was updated. @nclopezo, @cmsbuild, @anton-a, @thspeer, @slava77, @Degano can you please check and sign again.

@cmsbuild
Copy link
Contributor

-1
When I ran the RelVals I found an error in the following worklfows:
4.53 step2

runTheMatrix-results/4.53_RunPhoton2012B+RunPhoton2012B+HLTD+RECODreHLT+HARVESTDreHLT/step2_RunPhoton2012B+RunPhoton2012B+HLTD+RECODreHLT+HARVESTDreHLT.log

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

@Dr15Jones
Copy link
Contributor Author

The test failed because of a failed DAS query.
@nclopezo @ktf please rerun (or approve :D)

@Dr15Jones
Copy link
Contributor Author

@nclopezo @ktf ping on rerunning tests?

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@Dr15Jones
Copy link
Contributor Author

@slava77 @anton-a @davidlange6 Ping? These are definitely needed for thread performance and I think should be considered bug fixes.

@slava77
Copy link
Contributor

slava77 commented May 19, 2014

couldn't sign until there's a working IB and jenkins runs

@slava77
Copy link
Contributor

slava77 commented May 19, 2014

+1

for #3896 10e7c4a
jenkins OK

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_1_X IBs unless changes (tests are also fine). @nclopezo, @ktf can you please take care of it?

ktf added a commit that referenced this pull request May 19, 2014
…ngModulesToStream

Multithreading -- Revert MuonDetLayerGeometry using modules to stream modules
@ktf ktf merged commit 5126390 into cms-sw:CMSSW_7_1_X May 19, 2014
@Dr15Jones Dr15Jones deleted the revertMuonDetLayerGeometryUsingModulesToStream branch June 12, 2014 16:26
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

5 participants