Various optimization in tracking #18457

VinInn · 2017-04-25T11:18:21Z

Optimization in various areas, not affecting algorithm behavior.
initialStepPreSplitting made faster w/o affecting physics.
Expect negligible regressions due to numerics...
MTV:
ttbar 0PU with latest IB:
http://innocent.home.cern.ch/innocent/RelVal/pu0_TkSpeed91X/plots_ootb/effandfake1.pdf
ttbar PU50 with pre2
http://innocent.home.cern.ch/innocent/RelVal/pu50_fastTk91_2/plots_ootb/effandfake1.pdf

cmsbuild · 2017-04-25T11:18:43Z

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

It involves the following packages:

DataFormats/SiPixelCluster
Geometry/CommonTopologies
RecoLocalTracker/SiPixelRecHits
RecoTracker/FinalTrackSelectors
RecoTracker/IterativeTracking
TrackPropagation/RungeKutta

@perrotta, @civanch, @Dr15Jones, @ianna, @mdhildreth, @cmsbuild, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @makortel, @felicepantaleo, @GiacomoSguazzoni, @rovere, @VinInn, @mschrode, @dkotlins, @gpetruc, @ebrondol, @threus, @dgulhan this is something you requested to watch as well.
@Muzaffar, @davidlange6, @smuzaffar you are the release manager for this.

cms-bot commands are listed here

VinInn · 2017-04-25T11:20:57Z

@cmsbuild, please test

cmsbuild · 2017-04-25T11:21:09Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/19368/console Started: 2017/04/25 13:22

ianna · 2017-04-25T12:39:46Z

DataFormats/SiPixelCluster/interface/SiPixelCluster.h

-  inline float charge() const {
-    float qm = 0.0;
+  inline int charge() const {
+    int qm = 0;
    int isize = thePixelADC.size();
    for (int i=0; i<isize; ++i) 


@VinInn - it can be simplified:

for( auto i : thePixelADC ) qm += i;

or
return std::accumulate(thePixelADC.begin(),thePixelADC.end(),int(0));

even better

cmsbuild · 2017-04-25T13:18:42Z

+1
Tested at: 8254ad4
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18457/19368/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
1a93973
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18457/19368/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18457/19368/git-merge-result

cmsbuild · 2017-04-29T14:08:31Z

Pull request #18457 was updated. @perrotta, @civanch, @Dr15Jones, @ianna, @mdhildreth, @cmsbuild, @slava77, @davidlange6 can you please check and sign again.

slava77 · 2017-04-29T15:16:23Z

On 4/29/17 6:58 AM, Chris Jones wrote: We've also seen related cases where |memset| was called on a memory location that was then used to hold an object and the gcc compiler optimized away the |memset| because they assumed constructors would properly initialize the data that needed to be initialized.

Would it similarly outsmart you and remove memset if the data has no default constructors? [about vtable pointer, it's a matter of using the right offset instead of a plain "this"]

…

How about adding comments to the initializer list for the uninitialized member functions saying that they are intended to not be initialized there for optization reasons? That way future maintainers of this class will not forget about those members or unoptimize the code? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18457 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEdcboNzuIHySMgogDJIuwo1-1U1TY8Kks5r00HxgaJpZM4NHVlo>.

slava77 · 2017-05-02T12:51:54Z

@cmsbuild please test

cmsbuild · 2017-05-02T12:52:21Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/19514/console Started: 2017/05/02 15:00

cmsbuild · 2017-05-02T15:44:52Z

-1

Tested at: 974524a

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

I found follow errors while testing this PR

Failed tests: UnitTests

Unit Tests:

I found errors in the following unit tests:

---> test testAlignmentOfflineValidation had ERRORS

cmsbuild · 2017-05-02T15:44:54Z

Comparison job queued.

VinInn · 2017-05-02T16:05:56Z

An exception of category 'Configuration' occurred while
   [0] Constructing the EventProcessor
   [1] Constructing module: class=PrimaryVertexValidation label='PVValidation'
Exception Message:
MissingParameter: Parameter 'Tpurge' not found.
----- End Fatal Exception -------------------------------------------------
Failure running test_OfflineValidaiton_cfg.py: status 73
status = 18688

---> test testAlignmentOfflineValidation had ERRORS

not from this PR

cmsbuild · 2017-05-02T18:59:15Z

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

Comparison Summary:

You potentially added 1979969 lines to the logs
Reco comparison results: 5567 differences found in the comparisons
DQMHistoTests: Total files compared: 24
DQMHistoTests: Total histograms compared: 1831629
DQMHistoTests: Total failures: 76180
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 1755268
DQMHistoTests: Total skipped: 180
DQMHistoTests: Total Missing objects: 0
Checked 98 log files, 14 edm output root files, 24 DQM output files

makortel · 2017-05-03T07:18:56Z

The unit test failure is likely caused by #18330. It added that parameter, and I see the unit test first failing in CMSSW_9_1_X_2017-04-28-1100 where the PR was merged.

slava77 · 2017-05-03T18:02:48Z

+1

for #18457 974524a

changes look OK, primarily technical/numerical speedups
jenkins tests pass and comparisons with baseline show numerous small changes starting from tracks and propagating to downstream objects, nothing is significant to mention (expected)
local tests with larger samples confirm the expected behavior. The reduction in CPU time in ttbar PU35 (wf 10224) is about 3% total of the reco time with the tracking modules speedup by about 5% (*Step* modules ). Tested in CMSSW_9_1_X_2017-04-28-1100.

This PR looks safe for 91X which is essentially closed.
It more naturally falls in the scope of 92X.

VinInn · 2017-05-04T11:58:23Z

I am only afraid the it may clash with changes in pixel geometry and localreco

slava77 · 2017-05-04T12:30:38Z

On 5/4/17 4:58 AM, Vincenzo Innocente wrote: I am only afraid the it may clash with changes in pixel geometry and localreco

OK doing nothing (not merging now in 91X) should just advance this PR to the next development release.

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18457 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEdcbiqc5Mm9FocwPqRlPOx4gxhiFT2cks5r2b1ggaJpZM4NHVlo>.

VinInn · 2017-05-04T14:39:55Z

ok, let's merge as soon as the first 9_2_X IB appears...

VinInn · 2017-05-05T09:14:07Z

now that 9_2_X IB is out is this PR candidate for merging or the whole test & validation procedure should start from scratch?

davidlange6 · 2017-05-05T09:18:50Z

no? did something get reset?

…

On May 5, 2017, at 11:14 AM, Vincenzo Innocente ***@***.***> wrote: now that 9_2_X IB is out is this PR candidate for merging or the whole test & validation procedure should start from scratch? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

VinInn · 2017-05-05T09:20:46Z

Not to my knowledge...

davidlange6 · 2017-05-05T09:23:50Z

so all is working as expected and nothing needs to start over

…

On May 5, 2017, at 11:20 AM, Vincenzo Innocente ***@***.***> wrote: Not to my knowledge... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

slava77 · 2017-05-08T22:04:08Z

here, jenkins said You potentially added 1979969 lines to the logs
... maybe we should put some color coding?

(the fix is in #18617 )

VinInn added 15 commits April 20, 2017 10:37

make persplitting a bit faster

31f4891

clean code, speed up

85942dc

Merged FasterMerger91 from repository VinInn

a8a97a1

use optimized tan

91f0be4

avoid useless type conversions

e3dc77a

avoid to compute reduntant quantities

e444b3d

identify the type of cluster

833f5bc

avoid multiple conversions

132defe

count w/o filter

450edf4

clean and speedup duplicate merging

e03a96c

avoid to fill zeros

bad9b89

use my version

1a93973

fix for bx and virtual dest.

923cf12

rollback all my changes to pixel cpe

61ac89d

first set of changes

5c4c5c4

cmsbuild added this to the Next CMSSW_9_1_X milestone Apr 25, 2017

cmsbuild added comparison-pending geometry-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Apr 25, 2017

protect irradiation damage

8254ad4

cmsbuild added tests-started and removed tests-pending labels Apr 25, 2017

ianna reviewed Apr 25, 2017

View reviewed changes

cmsbuild added tests-started and removed tests-pending labels May 2, 2017

cmsbuild added tests-rejected and removed tests-started labels May 2, 2017

cmsbuild added comparison-available and removed comparison-pending labels May 2, 2017

cmsbuild added reconstruction-approved and removed reconstruction-pending labels May 3, 2017

smuzaffar modified the milestones: CMSSW_9_2_X, CMSSW_9_1_X May 4, 2017

davidlange6 merged commit f9e782e into cms-sw:master May 5, 2017

makortel mentioned this pull request Jan 15, 2019

Fix clang warnings in RecoLocalTracker/SiPixelRecHits #25675

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various optimization in tracking #18457

Various optimization in tracking #18457

VinInn commented Apr 25, 2017

cmsbuild commented Apr 25, 2017 •

edited by smuzaffar

Loading

VinInn commented Apr 25, 2017

cmsbuild commented Apr 25, 2017 •

edited

Loading

ianna Apr 25, 2017

VinInn Apr 25, 2017

ianna Apr 25, 2017

cmsbuild commented Apr 25, 2017

cmsbuild commented Apr 29, 2017

slava77 commented Apr 29, 2017 via email

slava77 commented May 2, 2017

cmsbuild commented May 2, 2017 •

edited

Loading

cmsbuild commented May 2, 2017

cmsbuild commented May 2, 2017

VinInn commented May 2, 2017

cmsbuild commented May 2, 2017

makortel commented May 3, 2017

slava77 commented May 3, 2017

VinInn commented May 4, 2017

slava77 commented May 4, 2017 via email

VinInn commented May 4, 2017

VinInn commented May 5, 2017

davidlange6 commented May 5, 2017 via email

VinInn commented May 5, 2017

davidlange6 commented May 5, 2017 via email

slava77 commented May 8, 2017

Various optimization in tracking #18457

Various optimization in tracking #18457

Conversation

VinInn commented Apr 25, 2017

cmsbuild commented Apr 25, 2017 • edited by smuzaffar Loading

VinInn commented Apr 25, 2017

cmsbuild commented Apr 25, 2017 • edited Loading

ianna Apr 25, 2017

Choose a reason for hiding this comment

VinInn Apr 25, 2017

Choose a reason for hiding this comment

ianna Apr 25, 2017

Choose a reason for hiding this comment

cmsbuild commented Apr 25, 2017

cmsbuild commented Apr 29, 2017

slava77 commented Apr 29, 2017 via email

slava77 commented May 2, 2017

cmsbuild commented May 2, 2017 • edited Loading

cmsbuild commented May 2, 2017

cmsbuild commented May 2, 2017

VinInn commented May 2, 2017

cmsbuild commented May 2, 2017

makortel commented May 3, 2017

slava77 commented May 3, 2017

VinInn commented May 4, 2017

slava77 commented May 4, 2017 via email

VinInn commented May 4, 2017

VinInn commented May 5, 2017

davidlange6 commented May 5, 2017 via email

VinInn commented May 5, 2017

davidlange6 commented May 5, 2017 via email

slava77 commented May 8, 2017

cmsbuild commented Apr 25, 2017 •

edited by smuzaffar

Loading

cmsbuild commented Apr 25, 2017 •

edited

Loading

cmsbuild commented May 2, 2017 •

edited

Loading