CorrectedJetProducer: change into a global::EDProducer (75x) #11300

fwyzard · 2015-09-16T20:44:36Z

change all specialisations of CorrectedJetProducer into
global::EDProducer modules:

CorrectedCaloJetProducer
CorrectedPFJetProducer
CorrectedJPTJetProducer
CorrectedTrackJetProducer
CorrectedGenJetProducer

change all specialisations of `CorrectedJetProducer` into `global::EDProducer` modules: - `CorrectedCaloJetProducer` - `CorrectedPFJetProducer` - `CorrectedJPTJetProducer` - `CorrectedTrackJetProducer` - `CorrectedGenJetProducer`

fwyzard · 2015-09-16T20:44:50Z

please test

cmsbuild · 2015-09-16T20:46:09Z

The tests are being triggered in jenkins.

cmsbuild · 2015-09-16T20:46:11Z

A new Pull Request was created by @fwyzard (Andrea Bocci) for CMSSW_7_5_X.

change CorrectedJetProducer into global::EDProducer

It involves the following packages:

JetMETCorrections/Modules

@cmsbuild, @vadler, @monttj can you please review it and eventually sign? Thanks.
@rappoccio, @ahinzmann, @mmarionncern, @jdolen, @nhanvtran, @schoef, @mariadalfonso, @TaiSakuma 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.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.
@Degano you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

fwyzard · 2015-09-16T20:47:26Z

this was a rather simple change, so I suspect that there is some underlying issue ?
@Dr15Jones , @davidlange6 , @slava77 , @cvuosalo , do you know of any issues with jet corrections and multithreading ?

slava77 · 2015-09-16T20:59:42Z

jet correctors had thread safety issues up until this summer or so.

on the other hand, somewhat centralized migration was done mainly based on a reco application and these modules are not in reco

fwyzard · 2015-09-16T22:11:33Z

tracked at #10965

cmsbuild · 2015-09-16T23:50:25Z

+1
Tested at: 15d40b4
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-11300/8142/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
6cb5caa
a55deef
696d886
291f176
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-11300/8142/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-11300/8142/git-merge-result

cmsbuild · 2015-09-17T00:54:03Z

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

Dr15Jones · 2015-09-17T14:28:03Z

JetMETCorrections/Modules/interface/CorrectedJetProducer.h

  {
+    // cache - note that variable length arrays are a GCC extension
+    reco::JetCorrector const * correctors[mCorrectorTokens.size()];


Please don't use this construct since it is not in the C++ standard. Using a std::vector<> here is not going to even show up on the performance I bet.

I'm not against avoiding the extensions, but if we do want to enforce it, can we add -Werror=vla to compiler options to prevent it ?

By the way, I wouldn't mind using a boost::small_vector<> .
Can we has boost 1.58 ?

Dr15Jones · 2015-09-17T14:32:01Z

This code does seem to be thread-safe to me. My only comment was the use of the VLA which is not C++ compliant.

fwyzard · 2015-09-18T11:00:17Z

please test

cmsbuild · 2015-09-18T11:01:52Z

The tests are being triggered in jenkins.

cmsbuild · 2015-09-18T11:01:54Z

Pull request #11300 was updated. @cmsbuild, @vadler, @monttj can you please check and sign again.

cmsbuild · 2015-09-18T12:51:40Z

+1
Tested at: 4935c97
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-11300/8190/summary.html

Dr15Jones · 2015-09-18T13:25:08Z

@fwyzard Changing the line of code lost your comment about upgrading to boost 1.58 . @davidlange6 Is our ability to move to a new boost constrained because of the DB code's forward compatibility requirement on boost serialization?

cmsbuild · 2015-09-18T14:06:48Z

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

monttj · 2015-09-22T08:48:13Z

+1

cmsbuild · 2015-09-22T08:51:47Z

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_5_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_7_6_X is complete. This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @Degano, @smuzaffar

davidlange6 · 2015-09-22T16:00:33Z

@Dr15Jones - yes, i understood that boost 1.58 indeed is sufficient to break the db compatibility.

davidlange6 · 2015-09-23T16:16:58Z

+1

CorrectedJetProducer: change into a global::EDProducer (75x)

CorrectedJetProducer: change into a global::EDProducer

15d40b4

change all specialisations of `CorrectedJetProducer` into `global::EDProducer` modules: - `CorrectedCaloJetProducer` - `CorrectedPFJetProducer` - `CorrectedJPTJetProducer` - `CorrectedTrackJetProducer` - `CorrectedGenJetProducer`

cmsbuild added this to the Next CMSSW_7_5_X milestone Sep 16, 2015

cmsbuild added analysis-pending pending-signatures tests-pending orp-pending comparison-pending labels Sep 16, 2015

cmsbuild added tests-started and removed tests-pending labels Sep 16, 2015

fwyzard changed the title ~~change CorrectedJetProducer into global::EDProducer~~ CorrectedJetProducer: change into a global::EDProducer (75x) Sep 16, 2015

fwyzard mentioned this pull request Sep 16, 2015

migrate modules used by the HLT menu to multithreading (75x) #10965

Closed

cmsbuild added tests-approved and removed tests-started labels Sep 16, 2015

cmsbuild added comparison-available and removed comparison-pending labels Sep 17, 2015

Dr15Jones reviewed Sep 17, 2015
View reviewed changes

replace GNU/C99 VLA with std::vector

4935c97

cmsbuild removed comparison-available tests-approved labels Sep 18, 2015

cmsbuild added tests-pending comparison-pending labels Sep 18, 2015

cmsbuild added tests-started and removed tests-pending labels Sep 18, 2015

cmsbuild added tests-approved and removed tests-started labels Sep 18, 2015

cmsbuild added comparison-available and removed comparison-pending labels Sep 18, 2015

fwyzard mentioned this pull request Sep 21, 2015

multithreading fix for 74x: CorrectedJetProducer #11343

Merged

cmsbuild added analysis-approved fully-signed and removed analysis-pending pending-signatures labels Sep 22, 2015

cmsbuild added orp-approved and removed orp-pending labels Sep 23, 2015

cmsbuild added a commit that referenced this pull request Sep 23, 2015

Merge pull request #11300 from fwyzard/global_CorrectedJetProducer_75x

4fd9033

CorrectedJetProducer: change into a global::EDProducer (75x)

cmsbuild merged commit 4fd9033 into cms-sw:CMSSW_7_5_X Sep 23, 2015

fwyzard deleted the global_CorrectedJetProducer_75x branch September 23, 2015 20:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CorrectedJetProducer: change into a global::EDProducer (75x) #11300

CorrectedJetProducer: change into a global::EDProducer (75x) #11300

fwyzard commented Sep 16, 2015

fwyzard commented Sep 16, 2015

cmsbuild commented Sep 16, 2015

cmsbuild commented Sep 16, 2015

fwyzard commented Sep 16, 2015

slava77 commented Sep 16, 2015

fwyzard commented Sep 16, 2015

cmsbuild commented Sep 16, 2015

cmsbuild commented Sep 17, 2015

Dr15Jones Sep 17, 2015

fwyzard Sep 18, 2015

fwyzard Sep 18, 2015

Dr15Jones commented Sep 17, 2015

fwyzard commented Sep 18, 2015

cmsbuild commented Sep 18, 2015

cmsbuild commented Sep 18, 2015

cmsbuild commented Sep 18, 2015

Dr15Jones commented Sep 18, 2015

cmsbuild commented Sep 18, 2015

monttj commented Sep 22, 2015

cmsbuild commented Sep 22, 2015

davidlange6 commented Sep 22, 2015

davidlange6 commented Sep 23, 2015

CorrectedJetProducer: change into a global::EDProducer (75x) #11300

CorrectedJetProducer: change into a global::EDProducer (75x) #11300

Conversation

fwyzard commented Sep 16, 2015

fwyzard commented Sep 16, 2015

cmsbuild commented Sep 16, 2015

cmsbuild commented Sep 16, 2015

fwyzard commented Sep 16, 2015

slava77 commented Sep 16, 2015

fwyzard commented Sep 16, 2015

cmsbuild commented Sep 16, 2015

cmsbuild commented Sep 17, 2015

Dr15Jones Sep 17, 2015

Choose a reason for hiding this comment

fwyzard Sep 18, 2015

Choose a reason for hiding this comment

fwyzard Sep 18, 2015

Choose a reason for hiding this comment

Dr15Jones commented Sep 17, 2015

fwyzard commented Sep 18, 2015

cmsbuild commented Sep 18, 2015

cmsbuild commented Sep 18, 2015

cmsbuild commented Sep 18, 2015

Dr15Jones commented Sep 18, 2015

cmsbuild commented Sep 18, 2015

monttj commented Sep 22, 2015

cmsbuild commented Sep 22, 2015

davidlange6 commented Sep 22, 2015

davidlange6 commented Sep 23, 2015