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

Tkdetmap threadfix #3862

Merged
merged 6 commits into from
Jun 5, 2014
Merged

Tkdetmap threadfix #3862

merged 6 commits into from
Jun 5, 2014

Conversation

threus
Copy link
Contributor

@threus threus commented May 14, 2014

TkDetMap made thread-safe with minimal changes: still kept as a Service, but issue reported here:
https://twiki.cern.ch/twiki/bin/view/CMSPublic/FWMultithreadedServicesReview#TkDetMap

should be now avoided by moving the cached_detid and cached_layer data members outside the class.

This is a temporary fix in order to put back asap all the DQM modules removed from sequences:
fa306b4

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @threus for CMSSW_7_1_X.

Tkdetmap threadfix

It involves the following packages:

CalibTracker/SiStripCommon
DQM/SiStripCommon
DQM/SiStripMonitorClient
DQM/SiStripMonitorTrack

@ojeda, @danduggan, @rovere, @cmsbuild, @diguida, @rcastello, @deguio, @Degano, @nclopezo can you please review it and eventually sign? Thanks.
@venturia 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.

@deguio
Copy link
Contributor

deguio commented May 14, 2014

thanks @threus . is the final plan to keep it as a service? or to move it to be a producer maybe?
I believe that @davidlange6 @Dr15Jones are interested and can comment..

cheers,
F.

@threus
Copy link
Contributor Author

threus commented May 14, 2014

@deguio, the plan is indeed to transform it into an EventSetup, but for the sake of bringing the validation back asap, we first propose a temporary fix.

@Dr15Jones
Copy link
Contributor

My suggestions for this temporary fix is to

  1. make all the member functions const
  2. change detmapType to be std::vector<const TkLayerMap*>.
    Both those changes will allow the compiler to help you be confident that no memory is being changed during the calls to the Service.

@cmsbuild
Copy link
Contributor

-1
I found an error when building:

Copying tmp/slc6_amd64_gcc481/src/DQMServices/Diagnostic/src/DQMServicesDiagnostic/libDQMServicesDiagnostic.so to productstore area:
>> Building shared library tmp/slc6_amd64_gcc481/src/DPGAnalysis/SiStripTools/src/DPGAnalysisSiStripTools/libDPGAnalysisSiStripTools.so
tmp/slc6_amd64_gcc481/src/CalibTracker/SiStripDCS/src/CalibTrackerSiStripDCS/SiStripDetVOffHandler.o: In function `boost::archive::detail::iserializer::load_object_data(boost::archive::detail::basic_iarchive&, void*, unsigned int) const':
SiStripDetVOffHandler.cc:(.text._ZNK5boost7archive6detail11iserializerIN3eos17portable_iarchiveE14SiStripDetVOffE16load_object_dataERNS1_14basic_iarchiveEPvj[_ZNK5boost7archive6detail11iserializerIN3eos17portable_iarchiveE14SiStripDetVOffE16load_object_dataERNS1_14basic_iarchiveEPvj]+0x37): undefined reference to`void SiStripDetVOff::serializeeos::portable_iarchive(eos::portable_iarchive&, unsigned int)'
Copying tmp/slc6_amd64_gcc481/src/DQMServices/Components/test/runTheWhiteRabbit/runTheWhiteRabbit to productstore area:
collect2: error: ld returned 1 exit status
Leaving library rule at DQMServices/Diagnostic
gmake: **\* [tmp/slc6_amd64_gcc481/src/CalibTracker/SiStripDCS/src/CalibTrackerSiStripDCS/libCalibTrackerSiStripDCS.so] Error 1
gmake: **\* Waiting for unfinished jobs....
Leaving library rule at DQMOffline/CalibTracker
>> Checking EDM Class Version for src/Alignment/OfflineValidation/src/classes_def.xml in libAlignmentOfflineValidationCapabilities.so


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

@threus
Copy link
Contributor Author

threus commented May 15, 2014

Thanks @Dr15Jones, I will implement your suggestions.
The crash is embarrassing, but I can't reproduce it yet. The changes were tested under CMSSW_7_1_X_2014-05-13-0200 with slc6_amd64_gcc490 and CMSSW_7_1_0_pre7 with SLC5. No issues during compilation, all libraries were built (done with checkdeps -A -a).
Now I am compiling also under CMSSW_7_1_X_2014-05-14-0200 with slc6_amd64_gcc481, but it's very slow (already more than one hour).
I am not sure what am I missing.

@ktf ktf mentioned this pull request May 19, 2014
@ktf
Copy link
Contributor

ktf commented May 19, 2014

Can you create a PR in 72X? Closing this.

@ktf ktf closed this May 19, 2014
@ktf
Copy link
Contributor

ktf commented May 19, 2014

Actually no, sorry, this one is probably a valid bug fix.

@ktf ktf reopened this May 19, 2014
@threus
Copy link
Contributor Author

threus commented May 19, 2014

Thanks @ktf, I'll update the PR as soon as I manage to get around all the constness.

@threus
Copy link
Contributor Author

threus commented May 30, 2014

Added a commit according to suggestions from @Dr15Jones and tested it on CMSSW_7_1_X_2014-05-27-1400 in slc5 and slc6. If it could be tested and added to pre9 as a bug fix, it would be great. Thanks.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 4, 2014

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 5, 2014

@deguio
Copy link
Contributor

deguio commented Jun 5, 2014

+1
is it possible to have this merged for pre9?

@diguida
Copy link
Contributor

diguida commented Jun 5, 2014

+1
Hope this can go in pre9...

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 5, 2014

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?

davidlange6 added a commit that referenced this pull request Jun 5, 2014
@davidlange6 davidlange6 merged commit 109cc4c into cms-sw:CMSSW_7_1_X Jun 5, 2014
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.

8 participants