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

speed up DA clusterizer allowing more vectorization #4435

Merged
merged 2 commits into from Jul 4, 2014

Conversation

VinInn
Copy link
Contributor

@VinInn VinInn commented Jun 28, 2014

35% speed up in the DA clusterizer itself at low pileup.
small regression observed in primary vertex reconstruction and other products whose producer algorithms depend on primary vertices (detached tracks, conversion)

https://twiki.cern.ch/twiki/bin/viewauth/CMS/HighGranularityCMSSWOptimization

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @VinInn (Vincenzo Innocente) for CMSSW_7_2_X.

speed up DA clusterizer allowing more vectorization

It involves the following packages:

Alignment/OfflineValidation
RecoVertex/PrimaryVertexProducer

@nclopezo, @cmsbuild, @diguida, @rcastello, @StoyanStoynev, @slava77, @Degano can you please review it and eventually sign? Thanks.
@ghellwig, @frmeier, @GiacomoSguazzoni, @rovere, @cerati, @mmusich 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.

@cmsbuild
Copy link
Contributor

Pull request #4435 was updated. @nclopezo, @cmsbuild, @Degano, @StoyanStoynev, @slava77 can you please check and sign again.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@StoyanStoynev
Copy link
Contributor

Working on it (for the above and for the record - see #4438).

@StoyanStoynev
Copy link
Contributor

I'll have it finalized by tomorrow. At least I can say I have no issues with the code changes. There are a lot of (seemingly expected) differences seen in QCD and ttbar (with PU) samples that I need to sort out - all look a fluctuation ("random") type, no trend, acceptable. This is to finalize and also check timing.

@StoyanStoynev
Copy link
Contributor

Timing. Tested on wf 202 (ttbar with PU; 200 events) and wf 38 (QCD, 1000 events). wf 25202 is consistent but has somewhat lesser precision (less events and less time per event for the interesting quantities). Overall - vertex times are affected positively (upto ~ 30+ %), some secondary vertex related quantities are affected negatively.

wf 202 (note: should apply +10% for proper normalization):
pixelVertices 55.8764 ms/ev -> 39.6211 ms/ev (after normalization would be ~ 44 ms/ev)
firstStepPrimaryVertices 70.3883 ms/ev -> 54.6836 ms/ev
offlinePrimaryVertices 144.392 ms/ev -> 111.15 ms/ev

negatively affected:
combinedSecondaryVertexBJetTags 2.12806 ms/ev -> 2.67696 ms/ev

not affected within few % (for the record):
particleFlowDisplacedVertex 57.2981 ms/ev -> 52.3893 ms/ev
secondaryVertexTagInfos 4.79849 ms/ev -> 4.28263 ms/ev
ghostTrackVertexTagInfos 9.97337 ms/ev -> 8.97076 ms/ev
hpsPFTauPrimaryVertexProducer 59.2892 ms/ev -> 55.452 ms/ev

In wf 38 (no PU) gains are much less pronounced (note: should apply +15% for proper normalization):
pixelVertices 29.1288 ms/ev -> 25.1571 ms/ev
firstStepPrimaryVertices 49.6234 ms/ev -> 42.6784 ms/ev
offlinePrimaryVertices 77.8378 ms/ev -> 65.9154 ms/ev

combinedSecondaryVertexBJetTags 4.25616 ms/ev -> 7.02886 ms/ev

particleFlowDisplacedVertex 16.0216 ms/ev -> 13.4798 ms/ev
secondaryVertexTagInfos 15.4817 ms/ev -> 13.0882 ms/ev
ghostTrackVertexTagInfos 36.6605 ms/ev -> 30.2541 ms/ev

Event Size. It is affected including other objects further down the line but in a mild way
(wf 202 tested). Examples are given below to illustrate changes.

"Branch Name | Average Uncompressed Size (Bytes/Event)":
recoVertexs_offlinePrimaryVertices__RECO. 14984.7 -> 14978.7
recoVertexs_pixelVertices__RECO. 8066.63 -> 8042.84
recoSecondaryVertexTagInfos_secondaryVertexTagInfos__RECO. 4137.22 -> 4139.88
recoTracks_generalTracks__RECO. 282111 -> 282031
recoJetedmRefToBaseProdTofloatsAssociationVector_combinedSecondaryVertexBJetTags__RECO. 380.555 -> 380.775
recoPFDisplacedVertexs_particleFlowDisplacedVertex__RECO. 616.42 -> 616.42 (not affected)
recoPFTaus_hpsPFTauProducer__RECO. 9980.17 -> 9976.24

Comparison plots with summary will follow shortly.

@StoyanStoynev
Copy link
Contributor

Comparisons. The number of PV (and constituents) is affected and this propagates to affect distributions in other objects (low and high level). Most of the changes are practically invisible. Taus and parts of isolation are probably the most significant (still not large but visible) to mention.

PV:
npv
tracksinvert
pvtracks

Tau:
tau
tauiso

Muon Iso /note some shapes are affected; the only significant shape change i spotted/:
muon_sumchhad
muon_sumphoet
muoniso

Jet/MET:
pfjet
met
met_sumet

Low level:
sistrip

All differences are acceptable and most are either negligible or not affecting the actual shapes of the distributions (based on this statistics but I see no hints for the opposite).

@StoyanStoynev
Copy link
Contributor

(red above is this PR)

@StoyanStoynev
Copy link
Contributor

+1
Based on the review above. In addition - most of the code changes are editorial (indentations), the non-trivial ones are optimizations (plus some additions). A note: there are new warnings appearing (wf 202):

%MSG-w TwoTrackMinimumDistance: PFTauPrimaryVertexProducer:hpsPFTauPrimaryVertexProducer 02-Jul-2014 21:03:13 CEST Run: 1 Event: 5088
comparing track with itself!
(---and---)
Computation HelixHelix::CAIR failed.

It happened in one event only (though 77 times) out of 200 events.

Tested 37e4476 on top of CMSSW_7_2_X_2014-06-27-0200 (as #4437, #4438, #4447). It is still mergeable in CMSSW_7_2_X_2014-07-04-0200 .

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 4, 2014

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

davidlange6 added a commit that referenced this pull request Jul 4, 2014
speed up DA clusterizer allowing more vectorization
@davidlange6 davidlange6 merged commit 365680c into cms-sw:CMSSW_7_2_X Jul 4, 2014
@VinInn VinInn deleted the DAspeedup branch July 13, 2016 13:47
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

4 participants