Validation -- changes for large object passed by value

[gartung]

found by clang static analyzer cms.ArgSizeChecker

std::map
std::vector

[cmsbuild]

A new Pull Request was created by @gartung (Patrick Gartung) for CMSSW_7_0_X.

Validation -- changes for large object passed by value

It involves the following packages:

Validation/RecoEgamma
Validation/RecoB
Validation/RecoJets
Validation/RecoMuon
Validation/HcalDigis
Validation/RecoVertex
Validation/EcalDigis
Validation/DTRecHits

@rovere, @deguio 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.

[rovere]

I would definitely avoid this kind of shortcuts, since they add basically no gain due to the copy ctr called immediately inside the method. I'd therefore suggest to drop the unnecessary copy (if it is necessary, then it's the const that has to be dropped) and fix the code inside the method accordingly. I suppose most of the problems could raise from the operator[] to access elements, since this will naturally break the const attribute. I'd use the at(key) one, which has a const flavour available. After a quick look at the code, I think this is the best solution.

[rovere]

-1

On Mon, Aug 26, 2013 at 11:58 PM, cmsbuild notifications@github.com wrote:

A new Pull Request was created by @gartung https://github.com/gartung(Patrick Gartung) for CMSSW_7_0_X.

Validation -- changes for large object passed by value

It involves the following packages:

Validation/RecoEgamma
Validation/RecoB
Validation/RecoJets
Validation/RecoMuon
Validation/HcalDigis
Validation/RecoVertex
Validation/EcalDigis
Validation/DTRecHits

@rovere https://github.com/rovere, @deguio https://github.com/deguiocan 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.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/625#issuecomment-23298473
.

[cmsbuild]

The following categories have been rejected by @rovere: DQM

@cms-git-dqm

[cmsbuild]

Pull request #625 was updated. Signatures reset, please check and sign again.

[gartung]

Backed out the changes you highlighted. The idea was to avoid the copy constructor in the function call.

[rovere]

Ciao Patrick,
sorry to be pedantic, but the new pull changes the old behavior, in the
sense that now passing a reference which is non-const will propagate back
all possible changes made within the receiving methods/functions outside of
their scope. Hence I would suggest to follow my at(key) implementation for
the case protected by the if (found(key)), namely in file
Validation/DTRecHits/plugins/DTRecHitQuality.cc, and use the kindly
provided code from Chris for the other, unprotected, case.

As a small remark, there are identical cases also in pull #602.

Thanks,
Marco.

On Tue, Aug 27, 2013 at 10:21 PM, Patrick Gartung
notifications@github.comwrote:

Backed out the changes you highlighted. The idea was to avoid the copy
constructor in the function call.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/625#issuecomment-23367479
.

[cmsbuild]

Pull request #625 was updated. Signatures reset, please check and sign again.

[gartung]

I will check pull request #602 as well. I will see how often I used the shortcut of moving the copy into the body of the function on the other branches.

[ktf]

@rovere @deguio can you please check this?

[elelias]

Hi Patrick,

I tested the code and all tests were successful. You are probably aware of the campaign to move the booking of histograms to beginRun() to prevent memory spikes, allow multithreading and multiharvesting. I have noticed that the code books the histogram inside the class constructors. Can we kindly ask you to try to move those books to the beginRun()? Thanks a lot.

You can find an example on #762

Thanks.

[gartung]

On 9/18/2013 9:06 AM, Elias Ron wrote:

Hi Patrick,

I tested the code and all tests were successful. You are probably
aware of the campaign to move the booking of histograms to beginRun()
to prevent memory spikes, allow multithreading and multiharvesting. I
have noticed that the code books the histogram inside the class
constructors. Can we kindly ask you to try to move those books to the
beginRun()? Thanks a lot.

You can find an example on #762 #762

Thanks.

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

Sorry, no. That is beyond the scope of my original changes.

[Dr15Jones]

I'm afraid Patrick has no real ownership of this code and only modified this code as a larger compaign to remove passing large objects by value from the entire CMSSW.

[ktf]

Apart from Patrick clearly not being the one who should do this changes, we should avoid piling up too much stuff in one single pull request and we should try to get them to converge as quick as possible. If the testing procedures are too cumbersome we should streamline the testing, not pile up work to test at once.

[cmsbuild]

The following categories have been signed by eron (a.k.a. @eliasron on GitHub): DQM

@cms-git-dqm