Make DetSetNew to support concurrent filler #12411

VinInn · 2015-11-13T14:53:17Z

With this PR DetSetNew support concurrent fillers.
The main application is Strip Unpacking and Clusterization on Demand at HLT.

Passed all tests and show no regression in sequential mode.

How to test the concurrency (besides the unit test)?
Waiting for the in-the-event module concurrency you can try concurrent PatternReco

git cms-addpkg RecoTracker/CkfPattern
touch RecoTracker/CkfPattern/src/CkfTrackCandidateMakerBase.cc
scram b -j8 USER_CXXFLAGS="-DVI_TBB"

take 134.91
add

process.options.numberOfThreads=cms.untracked.uint32(8)
process.options.numberOfStreams=cms.untracked.uint32(1)

to step2_L1REPACK_HLT.py
and

cmsRun step2_L1REPACK_HLT.py

you can also switch VI_DEBUG in ClustersFromRawProducer.cc and compare
the dump (better just the event by event VI VI clusters lines)

At the moment it will show some differences due to the fact that concurrent PatternReco is not reproducible due to cleaning of seeds and candidate (or a bug???)
I will see if I can had a flag to make it fully reproducible at the cost of slowness and memory-blowup...

btw the current version (contrary to 7_6) will stop with

Exception Message:
Instantiating a second DetSetVector::FastFiller only one DetSetVector::FastFiller can be active at a given time!

both

typo

The thread_local in RandomNumberGeneratorService was being used to support a backwards compatible interface. The old interface is also supported for certain transitions via another thread_local from the framework. It appears that the old interface is no longer being called by non-framework supported transitions.

We no longer support calling RandomNumberGenerate::mySeed at specific framework transitions. Test now enforces the change.

VinInn · 2015-11-13T14:53:35Z

@cmsbuild, please test

cmsbuild · 2015-11-13T14:56:14Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/9703/console

cmsbuild · 2015-11-19T13:36:06Z

+1
Tested at: ce076e8
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-12411/9846/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
ce076e8
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-12411/9846/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-12411/9846/git-merge-result

Dr15Jones · 2015-11-19T14:21:12Z

DataFormats/Common/interface/DetSetVectorNew.h

+#endif
+
+#include <atomic>
+#include <thread>


What symbol do you need from this include?

none left, indeed.

cmsbuild · 2015-11-19T14:40:11Z

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

Dr15Jones · 2015-11-19T14:56:28Z

I think we should have a later pull request which gets rid of the const_cast and changes all the related code to be const so that it is explicitly const thread safe. This will allow the static analyzer to properly check the code.

Dr15Jones · 2015-11-19T14:57:28Z

+1
I think at this point we need to see how this performs in a full IB.

cmsbuild · 2015-11-19T15:14:57Z

+1
Tested at: 65c71d5
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-12411/9851/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
f0d1af8
ce076e8
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-12411/9851/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-12411/9851/git-merge-result

Dr15Jones · 2015-11-19T15:38:55Z

DataFormats/Common/interface/DetSetVectorNew.h

+      static DetSetVector<T>::Item & dummy() {
+        assert(false);
+	static  DetSetVector<T>::Item d; return d;
+      }
      FastFiller(DetSetVector<T> & iv, id_type id, bool isaveEmpty=false) : 


Were all uses of FastFiller replaced with TSFastFiller? If so, then in a subsequent pull request we should remove FastFiller.

No, this is not foreseen at least at the moment (at least on my personal side).

cmsbuild · 2015-11-19T17:02:42Z

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

cvuosalo · 2015-11-19T22:17:55Z

RecoJets /JetAnalyzers /src /myFilter.cc and RecoJets /JetAnalyzers /src /myJetAna.cc generate a lot of static analyzer warnings. Could you please fix them?

https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-12411/9851/llvm-analysis/

VinInn · 2015-11-20T07:50:45Z

@cvuosalo , I think we should survive with those (or ask jet people to take care...
v.

VinInn · 2015-11-20T07:52:44Z

@cvuosalo , those warnings are not related to this PR...
v.

cvuosalo · 2015-11-20T19:24:22Z

+1

For #12411 65c71d5

Changing DetSetNew to support concurrent fillers.. There should be no change in monitored reco quantities.

The code changes are satisfactory. There are static analyzer warnings about RecoJets/JetAnalyzers/src/myFilter.cc and RecoJets /JetAnalyzers /src /myJetAna.cc that need to be fixed in a later PR. Jenkins tests against baseline CMSSW_8_0_X_2015-11-18-2200 show no significant differences, as expected.

cvuosalo · 2015-11-20T20:08:55Z

@VinInn: We are just noticing that the history of this PR includes a lot of irrelevant merges. Could you please clean it up?
Thanks.

VinInn · 2015-11-21T09:03:57Z

@cvusalo, please give verbatim recipe on how to cleanup.
@davidlange6 If it merges w/o problems, please merge.

davidlange6 · 2015-11-23T03:23:18Z

@VinInn - I think @cvuosalo just means there are some commits that have nothing to do with the current request (triggering an additional signature in this case that you have been waiting for..)

Make DetSetNew to support concurrent filler

BetterWang and others added 22 commits October 19, 2015 11:36

[+] init commit

52c97ed

[+] milestone version

2944b96

[+] working version

1971a8b

[!] fix label

4a770c1

[+] UCC

2dc047d

[+] UCC working code

62bb2c1

[!] clean up

15b76c0

[!]

d485dbf

[!] 10% to 5%

a5ee844

[+] update HLT path with 5% and 10%. either of them will run, but not

1c14196

both

thread safe DetSet

41ef2c2

fix copies by value...

a0ec998

hide atomic from root

4f8cd6b

print what is

32c6490

Update eventshapeDQM_cfi.py

3983f97

typo

Updated test to match new behavior

aaa76e4

We no longer support calling RandomNumberGenerate::mySeed at specific framework transitions. Test now enforces the change.

merged with HEAD

24f0ae3

Merged DetSetTS800 from repository VinInn

90cf80d

compiles

58873f6

scale test with nthreads

1577234

remove old from test

181c3d1

cmsbuild added this to the Next CMSSW_8_0_X milestone Nov 13, 2015

cmsbuild added reconstruction-pending dqm-pending core-pending pending-signatures tests-pending labels Nov 13, 2015

cmsbuild added tests-started and removed tests-pending labels Nov 19, 2015

cmsbuild added tests-approved and removed tests-started labels Nov 19, 2015

Dr15Jones reviewed Nov 19, 2015
View reviewed changes

cmsbuild added comparison-available and removed comparison-pending labels Nov 19, 2015

cmsbuild added core-approved and removed core-pending labels Nov 19, 2015

Dr15Jones reviewed Nov 19, 2015
View reviewed changes

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Nov 20, 2015

davidlange6 added a commit that referenced this pull request Nov 23, 2015

Merge pull request #12411 from VinInn/DetSetTS800

8957451

Make DetSetNew to support concurrent filler

davidlange6 merged commit 8957451 into cms-sw:CMSSW_8_0_X Nov 23, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make DetSetNew to support concurrent filler #12411

Make DetSetNew to support concurrent filler #12411

VinInn commented Nov 13, 2015

VinInn commented Nov 13, 2015

cmsbuild commented Nov 13, 2015

cmsbuild commented Nov 19, 2015

Dr15Jones Nov 19, 2015

VinInn Nov 19, 2015

cmsbuild commented Nov 19, 2015

Dr15Jones commented Nov 19, 2015

Dr15Jones commented Nov 19, 2015

cmsbuild commented Nov 19, 2015

Dr15Jones Nov 19, 2015

VinInn Nov 19, 2015

cmsbuild commented Nov 19, 2015

cvuosalo commented Nov 19, 2015

VinInn commented Nov 20, 2015

VinInn commented Nov 20, 2015

cvuosalo commented Nov 20, 2015

cvuosalo commented Nov 20, 2015

VinInn commented Nov 21, 2015

davidlange6 commented Nov 23, 2015

Make DetSetNew to support concurrent filler #12411

Make DetSetNew to support concurrent filler #12411

Conversation

VinInn commented Nov 13, 2015

VinInn commented Nov 13, 2015

cmsbuild commented Nov 13, 2015

cmsbuild commented Nov 19, 2015

Dr15Jones Nov 19, 2015

Choose a reason for hiding this comment

VinInn Nov 19, 2015

Choose a reason for hiding this comment

cmsbuild commented Nov 19, 2015

Dr15Jones commented Nov 19, 2015

Dr15Jones commented Nov 19, 2015

cmsbuild commented Nov 19, 2015

Dr15Jones Nov 19, 2015

Choose a reason for hiding this comment

VinInn Nov 19, 2015

Choose a reason for hiding this comment

cmsbuild commented Nov 19, 2015

cvuosalo commented Nov 19, 2015

VinInn commented Nov 20, 2015

VinInn commented Nov 20, 2015

cvuosalo commented Nov 20, 2015

cvuosalo commented Nov 20, 2015

VinInn commented Nov 21, 2015

davidlange6 commented Nov 23, 2015