Thread safe changes for HcalDcsMap, HCalElecrtonicsMap classes #909

vkuznet · 2013-09-24T15:36:22Z

No description provided.

cmsbuild · 2013-09-24T15:37:49Z

A new Pull Request was created by @vkuznet (Valentin Kuznetsov) for CMSSW_7_0_X.

Thread safe changes for HcalDcsMap, HCalElecrtonicsMap classes

It involves the following packages:

CondFormats/HcalObjects

@smuzaffar, @apfeiffer1, @nclopezo, @demattia, @rcastello, @ggovi 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.
@ktf you are the release manager for this.

vkuznet · 2013-09-24T15:38:37Z

@Dr15Jones please review

Dr15Jones · 2013-09-24T16:29:33Z

CondFormats/HcalObjects/src/HcalDcsMap.cc

 }

 HcalDcsMap::~HcalDcsMap(){
+    if (mItemsById) {


In C++ it is fine to call delete on a null pointer so there is no need for the check.

Dr15Jones · 2013-09-24T16:46:16Z

In reviewing information about std::atomic<> I see that for the case of a cache, we really should use the acquire/release memory ordering in order to avoid synchronization overheads.

cmsbuild · 2013-09-25T14:57:33Z

Pull request #909 was updated. @smuzaffar, @apfeiffer1, @nclopezo, @demattia, @rcastello, @ggovi can you please check and sign again.

nclopezo · 2013-09-26T08:34:33Z

-1
Hi,
I ran the jenkins tests, but I got segmentation violation error in all the workflows of the short matrix tests, the tests were run on top of CMSSW_7_0_X_2013-09-25-0200. You can see the outputs here:

https://cmssdt.cern.ch/jenkins/job/Pull-Request-Integration/ARCHITECTURE=slc5_amd64_gcc481/649/consoleFull
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/649

Dr15Jones · 2013-09-26T14:44:26Z

Could someone run valgrind on one of these crashing jobs so we can see what memory is being corrupted?

nclopezo · 2013-09-26T15:01:07Z

Hi Chris,

I started a Jenkins job to test workflow 4.22 here:
https://cmssdt.cern.ch/jenkins/job/Pull-Request-Integration/ARCHITECTURE=slc5_amd64_gcc481/662/console

When It finishes you should find here a valgrind.xml file with the results:

https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/662/runTheMatrix-results/4.22_RunCosmics2011A+RunCosmics2011A+RECOCOSD+ALCACOSD+SKIMCOSD+HARVESTDC/

And also the jenkins valgrind plugin should parse the results and show them in jenkins.

nclopezo · 2013-09-27T07:42:33Z

Sorry I gave you a bad url for the valgrind.xml file, it is here:

https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/662/valgrindResults-4.22/4.22_RunCosmics2011A+RunCosmics2011A+RECOCOSD+ALCACOSD+SKIMCOSD+HARVESTDC/

an the parsed results are here:

https://cmssdt.cern.ch/jenkins/job/Pull-Request-Integration/ARCHITECTURE=slc5_amd64_gcc481/662/valgrindResult/?

But also I noticed that this time 4.22 executed without errors.

cmsbuild · 2013-09-27T17:57:36Z

Pull request #909 was updated. @smuzaffar, @apfeiffer1, @nclopezo, @demattia, @rcastello, @ggovi can you please check and sign again.

apfeiffer1 · 2013-09-30T06:21:43Z

+1
provided it passes by Jenkins ...

cmsbuild · 2013-09-30T14:42:37Z

Pull request #909 was updated. @smuzaffar, @apfeiffer1, @nclopezo, @demattia, @rcastello, @ggovi can you please check and sign again.

apfeiffer1 · 2013-10-01T14:17:04Z

+1
then ! :)

nclopezo · 2013-10-02T12:21:13Z

Hi,
I ran again the usual tests and I got again segmentation violation error in the RelVals, is there still a commit pending for this pull request?

vkuznet · 2013-10-02T15:25:23Z

David, I've added initialization of mutable member data in default ctor, which may cause this. Please re-run the tests. Sorry I didn't spot this earlier.

On Oct 2, 2013, at ,Oct 2, 8:21 AM, David Mendez wrote:

Hi,
I ran again the usual tests and I got again segmentation violation error in the RelVals, is there still a commit pending for this pull request?

—
Reply to this email directly or view it on GitHub.

cmsbuild · 2013-10-02T15:27:38Z

Pull request #909 was updated. @smuzaffar, @apfeiffer1, @nclopezo, @demattia, @rcastello, @ggovi can you please check and sign again.

nclopezo · 2013-10-03T13:41:35Z

+1
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/762/summary.html

demattia · 2013-10-03T16:30:13Z

+1

apfeiffer1 · 2013-10-04T13:17:30Z

+1

cmsbuild · 2013-10-04T13:17:41Z

This pull request is fully signed and it will be integrated in one of the next IBs unless changes or unless it breaks tests. @ktf can you please take care of it?

Thread safety -- Changes for HcalDcsMap, HCalElecrtonicsMap classes

slava77 · 2016-08-17T03:56:24Z

CondFormats/HcalObjects/src/HcalElectronicsMap.cc


-  item = std::lower_bound (mPItemsById.begin(), mPItemsById.end(), &target, hcal_impl::LessById());
-  if (item == mPItemsById.end() || (*item)->mId != fId) 
+  auto ptr = (*mPItemsById.load(std::memory_order_acquire));


as a part of testing #15321 which somewhat excessively uses the HcalElectronicsMap,
I noticed that the CPU use is dominated by memmove
https://slava77sk.web.cern.ch/slava77sk/reco/cgi-bin/igprof-navigator/CMSSW_8_1_0_pre10-sign746-ccb128f.tt35pre8.mt8.pp/947
After changing to auto const& ptr the time in hcalRawData went down from ~2s/evt to ~40 ms/evt.

@lihux25 or @igv4321 please follow up

Thread safe changes for HcalDcsMap, HCalElecrtonicsMap classes

6cff0f3

Dr15Jones reviewed Sep 24, 2013
View reviewed changes

Add std::memory_order_acq_rel to exhcage methods; fix memory leak

c7bf9a4

Add MemoryOrder hooks

6aebb94

Add memory order to exchange calls

9361510

Add initialization of mutable member data in default ctor

fd77c46

ktf added a commit that referenced this pull request Oct 7, 2013

Merge pull request #909 from vkuznet/ThreadSafe_HcalObjects_v3

afa9d84

Thread safety -- Changes for HcalDcsMap, HCalElecrtonicsMap classes

ktf merged commit afa9d84 into cms-sw:CMSSW_7_0_X Oct 7, 2013

slava77 reviewed Aug 17, 2016
View reviewed changes

slava77 mentioned this pull request Aug 22, 2016

HCal uTCA packer #15520

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Thread safe changes for HcalDcsMap, HCalElecrtonicsMap classes #909

Thread safe changes for HcalDcsMap, HCalElecrtonicsMap classes #909

vkuznet commented Sep 24, 2013

cmsbuild commented Sep 24, 2013

vkuznet commented Sep 24, 2013

Dr15Jones Sep 24, 2013

Dr15Jones commented Sep 24, 2013

cmsbuild commented Sep 25, 2013

nclopezo commented Sep 26, 2013

Dr15Jones commented Sep 26, 2013

nclopezo commented Sep 26, 2013

nclopezo commented Sep 27, 2013

cmsbuild commented Sep 27, 2013

apfeiffer1 commented Sep 30, 2013

cmsbuild commented Sep 30, 2013

apfeiffer1 commented Oct 1, 2013

nclopezo commented Oct 2, 2013

vkuznet commented Oct 2, 2013

cmsbuild commented Oct 2, 2013

nclopezo commented Oct 3, 2013

demattia commented Oct 3, 2013

apfeiffer1 commented Oct 4, 2013

cmsbuild commented Oct 4, 2013

slava77 Aug 17, 2016

Thread safe changes for HcalDcsMap, HCalElecrtonicsMap classes #909

Thread safe changes for HcalDcsMap, HCalElecrtonicsMap classes #909

Conversation

vkuznet commented Sep 24, 2013

cmsbuild commented Sep 24, 2013

vkuznet commented Sep 24, 2013

Dr15Jones Sep 24, 2013

Choose a reason for hiding this comment

Dr15Jones commented Sep 24, 2013

cmsbuild commented Sep 25, 2013

nclopezo commented Sep 26, 2013

Dr15Jones commented Sep 26, 2013

nclopezo commented Sep 26, 2013

nclopezo commented Sep 27, 2013

cmsbuild commented Sep 27, 2013

apfeiffer1 commented Sep 30, 2013

cmsbuild commented Sep 30, 2013

apfeiffer1 commented Oct 1, 2013

nclopezo commented Oct 2, 2013

vkuznet commented Oct 2, 2013

cmsbuild commented Oct 2, 2013

nclopezo commented Oct 3, 2013

demattia commented Oct 3, 2013

apfeiffer1 commented Oct 4, 2013

cmsbuild commented Oct 4, 2013

slava77 Aug 17, 2016

Choose a reason for hiding this comment