Speed up makeRef and sorting of trajectory #12587

VinInn · 2015-11-28T18:13:01Z

Two independent optimizations
makeRef avoiding multiple deferencing of Handle
use nth_element to "sort" Trajectory (it may produce regression)

VinInn · 2015-11-28T18:13:31Z

@cmsbuild, please test

cmsbuild · 2015-11-28T18:16:11Z

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

cmsbuild · 2015-11-28T18:16:13Z

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

It involves the following packages:

DataFormats/Common
RecoLocalTracker/SiStripRecHitConverter
RecoTracker/CkfPattern
RecoTracker/MeasurementDet

@smuzaffar, @Dr15Jones, @cvuosalo, @cmsbuild, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @wmtan, @makortel, @forthommel, @yduhm, @GiacomoSguazzoni, @gbenelli, @rovere, @VinInn, @nickmccoll, @mschrode, @jlagram, @wddgit, @cerati, @gpetruc, @istaslis, @threus, @dgulhan this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

Following commands in first line of a comment are recognized

+1|approve[d]|sign[ed]: L1/L2's to approve it
-1|reject[ed]: L1/L2's to reject it
assign <category>[,<category>[,...]]: L1/L2's to request signatures from other categories
unassign <category>[,<category>[,...]]: L1/L2's to remove signatures from other categories
hold: L1/all L2's/release manager to mark it as on hold
unhold: L1/user who put this PR on hold
merge: L1/release managers to merge this request
[@cmsbuild,] please test: L1/L2 and selected users to start jenkins tests
[@cmsbuild,] please test with cms-sw/cmsdist#<PR>: L1/L2 and selected users to start jenkins tests using externals from cmsdist

cmsbuild · 2015-11-28T20:52:54Z

+1
Tested at: b8c4cdd
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-12587/9997/summary.html

cmsbuild · 2015-11-28T23:45:30Z

+1
Tested at: b8c4cdd
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-12587/9998/summary.html

VinInn · 2015-11-29T08:36:38Z

@cmsbuild, please test

cmsbuild · 2015-11-29T08:42:13Z

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

cmsbuild · 2015-11-29T08:42:14Z

Pull request #12587 was updated. @smuzaffar, @Dr15Jones, @cvuosalo, @cmsbuild, @slava77, @davidlange6 can you please check and sign again.

cmsbuild · 2015-11-29T11:20:14Z

-1

Tested at: 2dff611
I found errors in the following unit tests:

---> test TestRunnerPhysicsToolsCondLiteIO had ERRORS

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

cmsbuild · 2015-11-29T12:04:42Z

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

VinInn · 2015-12-01T13:08:11Z

@cmsbuild, please test

cmsbuild · 2015-12-01T13:08:12Z

Pull request #12587 was updated. @smuzaffar, @Dr15Jones, @cvuosalo, @cmsbuild, @slava77, @davidlange6 can you please check and sign again.

cmsbuild · 2015-12-01T13:08:25Z

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

Dr15Jones · 2015-12-01T14:19:13Z

+1
fine for changes in Core

cmsbuild · 2015-12-01T16:29:35Z

+1
Tested at: 8c08643
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-12587/10037/summary.html

cmsbuild · 2015-12-01T17:34:30Z

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

slava77 · 2015-12-02T06:05:11Z

it's not so nice to mix in technical changes which do not produce regressions but need full CMSSW rebuild
and logical changes which produce regressions but are limited to just one .cc file.

slava77 · 2015-12-02T06:10:46Z

RecoTracker/CkfPattern/plugins/GroupedCkfTrajectoryBuilder.cc

@@ -361,7 +361,7 @@ GroupedCkfTrajectoryBuilder::groupedLimitedCandidates (const TrajectorySeed& see
      if ((int)newCand.size() > theMaxCand) {
 	//ShowCand()(newCand);

- 	sort( newCand.begin(), newCand.end(), GroupedTrajCandLess(theLostHitPenalty,theFoundHitBonus));
+ 	std::nth_element( newCand.begin(), newCand.begin()+theMaxCand, newCand.end(), GroupedTrajCandLess(theLostHitPenalty,theFoundHitBonus));


Does the remaining code using the "newCand" care about the order of the trajectories (the part that's left from begin to +maxCand)?

no, it is a side effect

slava77 · 2015-12-02T06:11:34Z

... just 174 packages, peanuts.

slava77 · 2015-12-02T16:28:26Z

+1

for #12587 8c08643

code changes are in line with the description
jenkins tests pass and comparisons with baseline show minor differences that start from tracks and propagate downstream (triggered by different ordering of trajectories after cleaning; logically equivalent but different: neither std::sort or std::nth_element specify the order of equal elements)
additionally tested locally in CMSSW_8_0_X_2015-11-30-1100 to check on 25202 and 1313: observations from jenkins are confirmed
- technical performance in 25202: time comparisons suggest reduction in track seeding and track candidate reco with a total of about 100ms/evt (1% of total reco)
- logically, the improvements are made in the good direction

cmsbuild · 2015-12-02T16:28:43Z

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

VinInn · 2015-12-02T16:35:17Z

Thanks for confirming.
Still, the real origin of the makeRefTo speedup remains obscure to me...

davidlange6 · 2015-12-03T05:47:35Z

+1

Speed up makeRef and sorting of trajectory

VinInn added 2 commits November 28, 2015 17:45

speedup makeRef

93026e8

avoid wasting time sorting

b8c4cdd

cmsbuild added this to the Next CMSSW_8_0_X milestone Nov 28, 2015

cmsbuild added reconstruction-pending core-pending pending-signatures tests-pending orp-pending comparison-pending labels Nov 28, 2015

cmsbuild added tests-started and removed tests-pending labels Nov 28, 2015

cmsbuild added tests-approved and removed tests-started labels Nov 28, 2015

nth_element is faster, no regression

2dff611

cmsbuild added tests-pending and removed tests-approved labels Nov 29, 2015

cmsbuild added tests-started and removed tests-pending labels Nov 29, 2015

cmsbuild added tests-rejected and removed tests-started labels Nov 29, 2015

cmsbuild added the comparison-pending label Dec 1, 2015

cmsbuild added tests-started and removed tests-pending labels Dec 1, 2015

cmsbuild added core-approved and removed core-pending labels Dec 1, 2015

cmsbuild added tests-approved and removed tests-started labels Dec 1, 2015

cmsbuild added comparison-available and removed comparison-pending labels Dec 1, 2015

slava77 reviewed Dec 2, 2015
View reviewed changes

cmsbuild added reconstruction-approved fully-signed and removed pending-signatures reconstruction-pending labels Dec 2, 2015

cmsbuild added orp-approved and removed orp-pending labels Dec 3, 2015

cmsbuild added a commit that referenced this pull request Dec 3, 2015

Merge pull request #12587 from VinInn/SpeedUp80

399188d

Speed up makeRef and sorting of trajectory

cmsbuild merged commit 399188d into cms-sw:CMSSW_8_0_X Dec 3, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speed up makeRef and sorting of trajectory #12587

Speed up makeRef and sorting of trajectory #12587

VinInn commented Nov 28, 2015

VinInn commented Nov 28, 2015

cmsbuild commented Nov 28, 2015

cmsbuild commented Nov 28, 2015

cmsbuild commented Nov 28, 2015

cmsbuild commented Nov 28, 2015

VinInn commented Nov 29, 2015

cmsbuild commented Nov 29, 2015

cmsbuild commented Nov 29, 2015

cmsbuild commented Nov 29, 2015

cmsbuild commented Nov 29, 2015

VinInn commented Dec 1, 2015

cmsbuild commented Dec 1, 2015

cmsbuild commented Dec 1, 2015

Dr15Jones commented Dec 1, 2015

cmsbuild commented Dec 1, 2015

cmsbuild commented Dec 1, 2015

slava77 commented Dec 2, 2015

slava77 Dec 2, 2015

VinInn Dec 2, 2015

slava77 commented Dec 2, 2015

slava77 commented Dec 2, 2015

cmsbuild commented Dec 2, 2015

VinInn commented Dec 2, 2015

davidlange6 commented Dec 3, 2015

Speed up makeRef and sorting of trajectory #12587

Speed up makeRef and sorting of trajectory #12587

Conversation

VinInn commented Nov 28, 2015

VinInn commented Nov 28, 2015

cmsbuild commented Nov 28, 2015

cmsbuild commented Nov 28, 2015

cmsbuild commented Nov 28, 2015

cmsbuild commented Nov 28, 2015

VinInn commented Nov 29, 2015

cmsbuild commented Nov 29, 2015

cmsbuild commented Nov 29, 2015

cmsbuild commented Nov 29, 2015

cmsbuild commented Nov 29, 2015

VinInn commented Dec 1, 2015

cmsbuild commented Dec 1, 2015

cmsbuild commented Dec 1, 2015

Dr15Jones commented Dec 1, 2015

cmsbuild commented Dec 1, 2015

cmsbuild commented Dec 1, 2015

slava77 commented Dec 2, 2015

slava77 Dec 2, 2015

Choose a reason for hiding this comment

VinInn Dec 2, 2015

Choose a reason for hiding this comment

slava77 commented Dec 2, 2015

slava77 commented Dec 2, 2015

cmsbuild commented Dec 2, 2015

VinInn commented Dec 2, 2015

davidlange6 commented Dec 3, 2015