Branchless MVA

[VinInn]

avoid conditional loads to allow the compiler to emit branchless code
for the tracking MVA is a 35% speedup
regression tested with the code in 8c2964a

[cmsbuild]

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

It involves the following packages:

CondFormats/EgammaObjects

@diguida, @cerminar, @cmsbuild, @franzoni, @ggovi, @mmusich, @davidlange6 can you please review it and eventually sign? Thanks.
@istaslis, @apfeiffer1, @tocheng 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

[VinInn]

@cmsbuild, please test

purely technical. no regression expected of any sort
@bendavid please have a look

[cmsbuild]

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

[bendavid]

Does the compiler really produce worse code if these three lines were replaced by
index = vector[fCutIndices[index]] > fCutVals[index] ? fRightIndices[index] : fLeftIndices[index];

[VinInn]

yes,
because no where is explicitly said that if
vector[fCutIndices[index]] > fCutVals[index] is true fLeftIndices[index] exists

obvious in
a = p ? p->k : 0
here you cannot evaluate the two options as if p il null the first will segfault

indeed a tree could be implemented in some strange way that not all right and left node exists for a given index...

[bendavid]

Alright looks good to me.

[cmsbuild]

+1
Tested at: 01cc6c3
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-13143/10882/summary.html

[mmusich]

+1

[cmsbuild]

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

[VinInn]

Jenkins seems to confirm absence of regression

[VinInn]

@slava77 , did not notice you were absent from the official subscribers
Hope we can merge in pre6: it is a not negligible speedup in reco

p.s.
It is not normal that a major reco algorithm is inside CondFormats:
Formats should contain only data, not algos
(this is pretty common, many calib algos are hidden inside calib-objects)
@Dr15Jones , @davidlange6, @slava77 : maybe is NOT to late to act?

[davidlange6]

is not waiting on @slava77 in any case.. @ggovi - any comments?

[VinInn]

the point is that it SHOULD wait on @slava77 ....

[ggovi]

+1

[cmsbuild]

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

[slava77]

On 2/2/16 7:18 AM, Vincenzo Innocente wrote:

@slava77 https://github.com/slava77 , did not notice you were absent
from the official subscribers
Hope we can merge in pre6

I've seen this and the fact that there are no regressions.
As David mentioned, it's not waiting for me.

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

[ggovi]

no change on persistent data attributes...

[slava77]

@VinInn
How did you quantify the speedup and what was the denominator of that 35% (or, alternatively, what is the absolute decrease in CPU)?
All code here is inlined, so an igprof will not be too descriptive.

[VinInn]

@slava77 , actually I used perf (I am writing a note on that)
in any case the 35% is from igprof on TrackMVAClassifierBase::produce
that was 3.7% of tracking (single muon runD) and now is 2.4%

[slava77]

On 2/2/16 7:38 AM, Vincenzo Innocente wrote:

@slava77 https://github.com/slava77 , actually I used perf (I am
writing a note on that)
in any case the 35% is from igprof on TrackMVAClassifierBase::produce
that was 3.7% of tracking (single muon runD) and now is 2.4%

Is this a new version?
In 25202 in 800pre3 I see that TrackMVAClassifierBase is about 1% of the
total reco/aod part of event processing.

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

[VinInn]

consistent: 3.7% of tracking at PU20, 1% of total at PU35

[davidlange6]

+1