Navigation Menu

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

Use a quick union algorithm to speed up PFBlockAlgo (factor of 2-3x) #14138

Merged
merged 1 commit into from Apr 29, 2016

Conversation

lgray
Copy link
Contributor

@lgray lgray commented Apr 19, 2016

Use a quick union algorithm + knowledge of ranges of block element types to speed up PFBlockAlgo.

Minor regressions expected due to the order-sensitivity of PFAlgo.

Regressions appear as changes in the particle flow candidate list, and so can affect PF-met and PF-jets kinematic observables. Effect is statistical in nature and should be quantifiable using a large stats sample.

@lgray
Copy link
Contributor Author

lgray commented Apr 19, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

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

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @lgray (Lindsey Gray) for CMSSW_8_1_X.

It involves the following packages:

RecoParticleFlow/PFProducer

@cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@mmarionncern, @rafaellopesdesa, @bachtis this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

@cmsbuild
Copy link
Contributor

@VinInn
Copy link
Contributor

VinInn commented Apr 19, 2016

@fwyzard , measured factor 3 speedup of PFBlockProducer for HLT at PU22 corresponding to 4% of total time.

@slava77
Copy link
Contributor

slava77 commented Apr 19, 2016

On 4/19/16 3:28 PM, Vincenzo Innocente wrote:

@fwyzard https://github.com/fwyzard , measured factor 3 speedup of
PFBlockProducer for HLT at PU22 corresponding to 4% of total time.

Hi Vincenzo,

Please post a link with HLT time instructions (or forward by email)

Thank you.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#14138 (comment)

@VinInn
Copy link
Contributor

VinInn commented Apr 19, 2016

this summary
https://twiki.cern.ch/twiki/bin/view/CMS/ViHLTPerf52016
corresponds to current IB+ #14058 + #14048

@cmsbuild
Copy link
Contributor

@lgray lgray changed the title Use a quick union algorithm to speed up PFBlockAlgo (factor of at least 3x) Use a quick union algorithm to speed up PFBlockAlgo (factor of 2-3x) Apr 26, 2016
@lgray
Copy link
Contributor Author

lgray commented Apr 26, 2016

Any movement on this?

@slava77 slava77 mentioned this pull request Apr 26, 2016
@cvuosalo
Copy link
Contributor

@lgray: Review should be done Thursday unless problems are found.
More details on the expected regressions would be helpful.

@@ -153,11 +151,14 @@ class PFBlockAlgo {
PFBlockLink::Type& linktype,
reco::PFBlock::LinkTest& linktest,
double& dist) const;


std::auto_ptr< reco::PFBlockCollection > blocksNew_;
std::auto_ptr< reco::PFBlockCollection > blocks_;
Copy link
Contributor

Choose a reason for hiding this comment

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

auto_ptr has been deprecated. When adding new code, wouldn't it be better to use shared_ptr or unique_ptr instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

blocksNew_ was from debugging and implementation anyway, I'll fix it.

@cvuosalo
Copy link
Contributor

+1

For #14138 11f9bef

Speeding up PFBlockAlgo.

The code changes are satisfactory. Jenkins tests against baseline CMSSW_8_1_X_2016-04-18-2300 show numerous, very tiny differences at the level of statistical fluctuations in PF quantities. An extended test of workflow 25202.0_TTbar_13 with 70 events against baseline CMSSW_8_1_X_2016-04-17-0000 also shows numerous, tiny, insignificant differences in PF quantities. CPU time shrinks about 50-80% for the affected modules, though the effect on total time per event is harder to measure.

For HLT (excluding the first event):

  delta/mean delta/orJob     original                   new       module name
  ---------- ------------     --------                  ----       ------------
   -1.413673      -0.40%       207.18 ms/ev ->        35.58 ms/ev hltParticleFlowBlockReg
   -1.389132      -0.42%       218.73 ms/ev ->        39.42 ms/ev hltParticleFlowBlock
   -1.383895      -0.40%       212.92 ms/ev ->        38.77 ms/ev hltParticleFlowBlockForTaus
  ---------- ------------     --------                  ----       ------------

For RECO:

        particleFlowBlock        119.404 ms/ev -> 53.8104 ms/ev
        pfNoPileUpIso    19.7848 ms/ev -> 7.09613 ms/ev
        pfNoPileUp       19.7443 ms/ev -> 7.02833 ms/ev
        pfNoPileUpJME    24.8046 ms/ev -> 14.0714 ms/ev
         Total in detailed printout:     191.904 ms/ev -> 87.8453 ms/ev          delta: -104.058

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_8_1_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @Degano, @smuzaffar

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

6 participants