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

Migration to multithread friendly modules #6687

Merged
merged 8 commits into from Nov 30, 2014

Conversation

mbluj
Copy link
Contributor

@mbluj mbluj commented Nov 28, 2014

In this pull request modules used by HLT paths with taus are migrated to multithread friendly modules - most of them to to edm::global::EDProducer.
This is a forward-port of #6686 to 74X release series.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @mbluj for CMSSW_7_4_X.

Migration to multithread friendly modules

It involves the following packages:

HLTrigger/btau
RecoTauTag/HLTProducers
RecoTauTag/TauTagTools

@perrotta, @cmsbuild, @nclopezo, @fwyzard, @Martin-Grunewald, @StoyanStoynev, @slava77 can you please review it and eventually sign? Thanks.
@ferencek, @acaudron, @jpavel, @pvmulder, @imarches 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.

@slava77
Copy link
Contributor

slava77 commented Nov 28, 2014

why is this one different from #6686? (the number of diffs according to github web is +129 −147 compared to +111 −129).
If there are no overlapping conflicts since in going from 73X to 74X, it should be possible to just submit a PR with the same branch from 73X here in 74X

@slava77
Copy link
Contributor

slava77 commented Nov 28, 2014

nm, it's actually the same.
I was looking at the other pr after suppression of spaces-only change
no need to do anything here

@slava77
Copy link
Contributor

slava77 commented Nov 28, 2014

+1

for #6687 8a8b639

tested in CMSSW_7_4_X_2014-11-27-0200 ( test area sign469, in combination with PRs 6658-6659-6662-6670-6671-6687)

checked with a short matrix: no diffs in reco, no diffs in 5.1 and 25.0 short matrix DQM outputs

edm::EDGetTokenT<trigger::TriggerFilterObjectWithRefs> triggerFilterMuonsSrc;
edm::EDGetTokenT<edm::View<reco::Vertex> > vertexLabel;
edm::EDGetTokenT<reco::BeamSpot> beamSpotLabel;
const edm::ParameterSet config() const { return theConfig; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a const & to avoid making a copy.

@slava77
Copy link
Contributor

slava77 commented Nov 29, 2014

Oh, nice.
Looks like the modified files themselves are fairly small.

On 11/28/14, 5:33 PM, Chris Jones wrote:

@slava77 https://github.com/slava77 I was looking at full files :)


Reply to this email directly or view it on GitHub
#6687 (comment).


Vyacheslav (Slava) Krutelyov
TAMU: Physics Dept Texas A&M MS4242, College Station, TX 77843-4242
CERN: 42-R-027
AIM/Skype: siava16 googleTalk: slava77@gmail.com
(630) 291-5128 Cell (US) +41 76 275 7116 Cell (CERN)


@cmsbuild
Copy link
Contributor

@Martin-Grunewald
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_4_X IBs unless changes (tests are also fine). This pull request will be automatically merged.

@mbluj
Copy link
Contributor Author

mbluj commented Dec 1, 2014

Dear Chris,

thank you for your detailed cheks of the code. I will follow your suggestions and then prepare a new PR.

@mbluj mbluj mentioned this pull request Dec 1, 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.

None yet

5 participants