backport of #14058: speed up avoiding eta phi computations, and #14151: Slim down PFRecHit #15294

fwyzard · 2016-07-26T23:25:04Z

backport to 8.0.x:

speed up avoiding eta phi computations #14058 speed up avoiding eta phi computations
Slim down PFRecHit #14151 Slim down PFRecHit

fwyzard · 2016-07-26T23:25:21Z

tracked at #15151

fwyzard · 2016-07-26T23:25:24Z

please test

cmsbuild · 2016-07-26T23:25:30Z

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

cmsbuild · 2016-07-26T23:25:31Z

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

It involves the following packages:

CondTools/Geometry
DataFormats/Math
DataFormats/ParticleFlowReco
Fireworks/ParticleFlow
Geometry/CaloGeometry
Geometry/HcalEventSetup
Geometry/HcalTowerAlgo
RecoParticleFlow/PFClusterProducer
RecoParticleFlow/PFClusterTools
RecoParticleFlow/PFProducer

@civanch, @Dr15Jones, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @alja, @slava77, @ggovi, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @apfeiffer1, @makortel, @mmarionncern, @rafaellopesdesa, @lgray, @alja, @cbernet, @bachtis this is something you requested to watch as well.
@slava77, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

cmsbuild · 2016-07-27T20:54:51Z

-1

Tested at: 299d487

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-15294/14259/summary.html

I found follow errors while testing this PR

Failed tests: UnitTests

Unit Tests:

I found errors in the following unit tests:

---> test runtestRecoEgammaElectronIdentification had ERRORS

cmsbuild · 2016-07-27T22:05:02Z

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

fwyzard · 2016-07-28T13:15:45Z

please test

cmsbuild · 2016-07-28T13:16:11Z

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

cmsbuild · 2016-07-28T18:35:08Z

-1

Tested at: 299d487

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-15294/14265/summary.html

I found follow errors while testing this PR

Failed tests: UnitTests

Unit Tests:

I found errors in the following unit tests:

---> test runtestRecoEgammaElectronIdentification had ERRORS

cmsbuild · 2016-07-28T19:31:28Z

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

slava77 · 2016-07-29T01:44:01Z

@smuzaffar
could you please check if this PR will break FWLite build or not in 80X.
I seem to recall we needed some adjustments in 81X for this (dependence on Geometry)

slava77 · 2016-07-30T02:43:45Z

Looking at timing, apparently the effect is only about 3%, not as large as the 7% presented in the RECO meeting (change estimated before #15240 was merged).
The test is using HLTPhysics_Run276935_LS80-220.root provided by Clint, I tested in CMSSW_8_0_X_2016-07-26-2300, which includes this PR

Backport of #13959: SpeedUp ParticleFlow for HLT; and #14070: remove memory fence #15240 Backport of SpeedUp ParticleFlow for HLT #13959: SpeedUp ParticleFlow for HLT; and remove memory fence #14070: remove memory fence

   -1.107807      -0.32%         1.40 ms/ev ->         0.40 ms/ev hltParticleFlowRecHitECALL1Seeded
   -1.099400      -1.39%         6.04 ms/ev ->         1.76 ms/ev hltParticleFlowRecHitPSUnseeded
   -1.081088      -0.43%         1.88 ms/ev ->         0.56 ms/ev hltParticleFlowRecHitPSL1Seeded
   -1.043078      -0.00%         0.00 ms/ev ->         0.00 ms/ev hltParticleFlowRecHitECALForTkMuons
   -1.010721      -0.45%         2.09 ms/ev ->         0.69 ms/ev hltParticleFlowRecHitECALUnseeded
   -1.008720      -0.00%         0.00 ms/ev ->         0.00 ms/ev hltParticleFlowRecHitPSForTkMuons
   -1.008665      -0.00%         0.01 ms/ev ->         0.00 ms/ev hltParticleFlowRecHitPSForMuons
   -0.855917      -0.14%         0.73 ms/ev ->         0.29 ms/ev hltParticleFlowRecHitHBHEForEgamma
   -0.832427      -0.23%         1.19 ms/ev ->         0.49 ms/ev hltParticleFlowRecHitHBHE
   -0.753037      -0.38%         2.14 ms/ev ->         0.97 ms/ev hltParticleFlowRecHitHF
   -0.553912      -0.35%         2.47 ms/ev ->         1.40 ms/ev hltParticleFlowRecHitHCAL
..  -11.4 ms/evt 
   +0.043551      +0.07%         4.63 ms/ev ->         4.84 ms/ev hltParticleFlowBlockReg
   +0.041118      +0.11%         7.96 ms/ev ->         8.29 ms/ev hltParticleFlowBlockForTaus
   +0.041006      +0.26%        19.40 ms/ev ->        20.21 ms/ev hltParticleFlowBlock
.. +1.3 ms/ev total
   +0.422218      +0.17%         1.01 ms/ev ->         1.55 ms/ev hltParticleFlowClusterECALL1Seeded
   +0.204638      +0.28%         3.85 ms/ev ->         4.72 ms/ev hltParticleFlowClusterECALUnseeded
   -0.093731      -0.05%         1.84 ms/ev ->         1.68 ms/ev hltParticleFlowClusterECALUncorrectedUnseeded
   -0.063899      -0.03%         1.27 ms/ev ->         1.19 ms/ev hltParticleFlowClusterECALUncorrectedL1Seeded
.. +1.2 ms/ev

Total per-job time changes from 0.379 s/ev to 0.371 s/ev, consistent with the total from the most relevant modules listed above.
#15240 (comment) shows that there is a large overlap in the set of modules with changing time per event.

slava77 · 2016-07-30T03:40:06Z

+1

for #15294 299d487

backport of a combination of speed up avoiding eta phi computations #14058 (parts of this were already in 80X, changed to a faster version of 81X) and Slim down PFRecHit #14151.
jenkins tests pass and comparisons with baseline show small differences observed also in individual tests of the 81X PRs
[edited] extended statistics tests show small changes in products in RECO and in HLT, without any specific pattern. This is expected from numerical level changes made in the code of this PR.
HLT timing tests show noticeable improvement, mainly in hltParticleFlowRecHit* modules, ups and downs consistent with what was seen also in 81X testing Slim down PFRecHit #14151 (comment). The net gain on HLTPhysics_Run276935_LS80-220 appears to be close to 3%.

@fwyzard it would be nice if you could check for the record the effect in your HLT setup using 8_0_16.

I'm signing already, since even the 3% are worth it.

The changes include backward-compatible data format changes and will need to be followed with dataset processing version change.

fwyzard · 2016-07-30T09:48:15Z

Hi Slava, yes, I plan to redo the tests with redirect to 8.0.16. Ciao, .Andrea

Dr15Jones · 2016-08-02T15:36:37Z

DataFormats/ParticleFlowReco/interface/PFRecHit.h

+    Neighbours buildNeighbours(unsigned int n) const { return  Neighbours(&neighbours_.front(),n);}
+
+    /// cell geometry
+    CaloCellGeometry const * caloCell_=nullptr;


Since PFRecHit are storable, how can this work? ROOT will attempt to write out the CaloCellGeometry object since it was not marked as transient!

Objection withdrawn. I didn't look far enough into the pull request changes.

it's a pointer. Why would root try to write the actual CaloCellGeometry object?

Because that is what ROOT I/O does when it encounters a pointer, it tries to write the object pointed to out to the file.

Interesting. I didn't know that.
(the caloCell_ is transient, so this discussion is mostly academic)

VinInn added 16 commits July 27, 2016 01:15

speed up avoiding eta phi computations

ccedb06

add rhoetaphi to calocell (for corners as well)

90ef330

first version of Sli PFRecHit

e82219b

RecoParticleFlow compiles, need to fix HF z corr

5f2fef0

Fireworks compiles, need to fix corners

bec604b

add PF geometry for HF

7f29f13

fix order of corners

588f02b

fix mistype, correct axis definition

12452df

mugrate to float, protect FireWork

1c23df4

use a single vector for neighbours

ceec029

do a proper sort

f9f339a

move to floats in position computation

cb74958

factorize conditionals and some division

783e0fb

vectorized (including icc compatibility)

7dd261a

fix bug, remove obsolete code

5946d08

fix calo writers

299d487

cmsbuild added this to the Next CMSSW_8_0_X milestone Jul 26, 2016

cmsbuild added reconstruction-pending db-pending geometry-pending visualization-pending pending-signatures tests-pending orp-pending comparison-pending labels Jul 26, 2016

cmsbuild removed the tests-pending label Jul 26, 2016

cmsbuild added tests-rejected and removed tests-started labels Jul 27, 2016

cmsbuild added comparison-available and removed comparison-pending labels Jul 27, 2016

cmsbuild added tests-pending comparison-pending and removed comparison-available tests-rejected labels Jul 28, 2016

cmsbuild added tests-started and removed tests-pending labels Jul 28, 2016

cmsbuild added tests-rejected and removed tests-started labels Jul 28, 2016

cmsbuild added comparison-available and removed comparison-pending labels Jul 28, 2016

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Jul 30, 2016

Dr15Jones reviewed Aug 2, 2016
View reviewed changes

cvuosalo mentioned this pull request Aug 24, 2016

possible 80X backports (terminated on 8_0_20) slava77/cmssw#93

Closed

davidlange6 merged commit ced6db6 into cms-sw:CMSSW_8_0_X Aug 26, 2016

fwyzard deleted the backport_14058+14151_80x branch September 1, 2016 12:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

backport of #14058: speed up avoiding eta phi computations, and #14151: Slim down PFRecHit #15294

backport of #14058: speed up avoiding eta phi computations, and #14151: Slim down PFRecHit #15294

fwyzard commented Jul 26, 2016 •

edited

fwyzard commented Jul 26, 2016

fwyzard commented Jul 26, 2016

cmsbuild commented Jul 26, 2016 •

edited

cmsbuild commented Jul 26, 2016

cmsbuild commented Jul 27, 2016

cmsbuild commented Jul 27, 2016

fwyzard commented Jul 28, 2016

cmsbuild commented Jul 28, 2016 •

edited

cmsbuild commented Jul 28, 2016

cmsbuild commented Jul 28, 2016

slava77 commented Jul 29, 2016

slava77 commented Jul 30, 2016

slava77 commented Jul 30, 2016 •

edited

fwyzard commented Jul 30, 2016 via email

Dr15Jones Aug 2, 2016

Dr15Jones Aug 2, 2016

slava77 Aug 2, 2016

Dr15Jones Aug 2, 2016

slava77 Aug 2, 2016

backport of #14058: speed up avoiding eta phi computations, and #14151: Slim down PFRecHit #15294

backport of #14058: speed up avoiding eta phi computations, and #14151: Slim down PFRecHit #15294

Conversation

fwyzard commented Jul 26, 2016 • edited

fwyzard commented Jul 26, 2016

fwyzard commented Jul 26, 2016

cmsbuild commented Jul 26, 2016 • edited

cmsbuild commented Jul 26, 2016

cmsbuild commented Jul 27, 2016

cmsbuild commented Jul 27, 2016

fwyzard commented Jul 28, 2016

cmsbuild commented Jul 28, 2016 • edited

cmsbuild commented Jul 28, 2016

cmsbuild commented Jul 28, 2016

slava77 commented Jul 29, 2016

slava77 commented Jul 30, 2016

slava77 commented Jul 30, 2016 • edited

fwyzard commented Jul 30, 2016 via email

Dr15Jones Aug 2, 2016

Choose a reason for hiding this comment

Dr15Jones Aug 2, 2016

Choose a reason for hiding this comment

slava77 Aug 2, 2016

Choose a reason for hiding this comment

Dr15Jones Aug 2, 2016

Choose a reason for hiding this comment

slava77 Aug 2, 2016

Choose a reason for hiding this comment

fwyzard commented Jul 26, 2016 •

edited

cmsbuild commented Jul 26, 2016 •

edited

cmsbuild commented Jul 28, 2016 •

edited

slava77 commented Jul 30, 2016 •

edited