ngrenz: testing of stereo optimization #10963

ngrenz · 2015-08-27T04:44:22Z

Optimization by reducing of operations for the stereo test, as well as avoiding of
object creation within TrackerHitAssociator.cc

The outputs are supposed to be unchanged.
Any comments and suggestions are welcome.

ngrenz · 2015-08-27T04:44:43Z

@boudoul FYI

cmsbuild · 2015-08-27T04:46:10Z

A new Pull Request was created by @ngrenz for CMSSW_7_6_X.

ngrenz: testing of stereo optimization

It involves the following packages:

DataFormats/SiStripDetId
DataFormats/TrackerCommon
SimTracker/TrackerHitAssociation

@cmsbuild, @cvuosalo, @civanch, @mdhildreth, @slava77 can you please review it and eventually sign? Thanks.
@makortel, @GiacomoSguazzoni, @rovere, @VinInn, @wmtford, @gpetruc, @cerati, @threus, @dgulhan 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.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.
@Degano you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

civanch · 2015-08-27T05:02:04Z

@ngrenz , having many "return" in an inline method disables inline. From my point of view, each inline method should have only one "return".

ngrenz · 2015-08-27T06:32:44Z

DataFormats/SiStripDetId/interface/SiStripDetId.h

@@ -112,18 +112,18 @@ class SiStripDetId : public DetId {
 // ---------- inline methods ----------


@civanch sorry, do you refer to this comment

The functions have the same number of returns as in the previous program version
and the inline expression is never used.

I had thought to change the SiStripDetId::stereo() function to
inline uint32_t SiStripDetId::stereo() const { return ( ((id_>>sterStartBit_ ) & sterMask_ ) == 1 ) ? 1:0; }
but I am not sure whether the readability would suffer

@ngrenz , yes, it is the most effective c++ but I agree the readability is not the best. It is possible to have something like:

inline uit32_t SiStripDetId::stereo const
{
uit32_t k = 0;
if(1 == ((id_>>sterStartBit_ ) & sterMask_ )) { k = 1; }
return k;
}
similar solutions may be applied in other inlined methods from this header.

Thanks, I will rewrite the function.

Do I need the inline expression?
Or rather I do not understand why there is the comment

// ---------- inline methods ----------

but no inline expressions, in the original code

If implementation of a method is inside header file it is assumed to be inlined even without a declaration.

i find
return 1 == (id_>>sterStartBit_ ) & sterMask_ ) ? 1:0;

very clear and obvious

ngrenz · 2015-08-27T10:41:54Z

DataFormats/TrackerCommon/interface/TrackerTopology.h

-      return ( id.rawId() - 2 );
-    } else { return 0; }
+    uint32_t testId = (id.rawId()>>tidVals_.sterStartBit_) & tidVals_.sterMask_;
+    return ( testId == 1 || testId == 2) ? ( id.rawId() - testId ) : 0;


@VinInn your last comment was "It can only be 0, 1 or 2 .."
do you mean the output of tidGlued or the result of the calculation (id.rawId()>>tidVals_.sterStartBit_) & tidVals_.sterMask_

if it is the calculation, then it is more efficient to compare testId with 0.

Thanks

to make clear
id.rawId()>>tidVals_.sterStartBit_) & tidVals_.sterMask_
for ANY strip det is
0 for r-phi
1 for stereo
2 for glued
3 never

slava77 · 2015-08-27T11:54:08Z

@cmsbuild please test

cmsbuild · 2015-08-27T11:56:22Z

The tests are being triggered in jenkins.

cmsbuild · 2015-08-27T11:56:23Z

Pull request #10963 was updated. @cmsbuild, @cvuosalo, @civanch, @mdhildreth, @slava77 can you please check and sign again.

civanch · 2015-08-27T13:21:08Z

please test

cmsbuild · 2015-08-27T15:35:53Z

+1
Tested at: b092a27
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-10963/7686/summary.html

cmsbuild · 2015-08-27T16:36:12Z

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

slava77 · 2015-08-27T16:47:39Z

DataFormats/SiStripDetId/interface/SiStripDetId.h

-    case 1: return W1A;
-    case 2: return W2A;
-    case 3: return W3A;
+    case 1: geometry = W1A;


you are missing "break;" after each assignment (otherwise everything below a selected "case" is executed

slava77 · 2015-08-28T23:24:38Z

@cmsbuild please test

cmsbuild · 2015-08-28T23:26:15Z

The tests are being triggered in jenkins.

cmsbuild · 2015-08-28T23:26:17Z

Pull request #10963 was updated. @cmsbuild, @cvuosalo, @civanch, @mdhildreth, @slava77 can you please check and sign again.

cmsbuild · 2015-08-29T02:48:34Z

+1
Tested at: 5dda346
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-10963/7739/summary.html

cmsbuild · 2015-08-29T03:39:15Z

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

civanch · 2015-08-29T11:34:58Z

+1

slava77 · 2015-09-01T00:44:33Z

+1

for #10963 5dda346

changes in the code are visually OK (although the impact of optimization is not very clear, beyond lexically better expressions; after the compiler optimization there probably isn't that much of a difference)
- is there a confirmed place where these changes help?
jenkins tests pass and comparisons with the baseline show no difference
checked locally with igprof running on 100 events or run2 data with prompt reco. At this level, there is no significant change. I checked a few methods that have DetId or TrackerTopology in the interface, but the CPU cost appeared to be about the same (within apparent per-job fluctuations of performance counts).

cmsbuild · 2015-09-01T00:46:27Z

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

davidlange6 · 2015-09-02T06:25:19Z

+1

ngrenz: testing of stereo optimization

ngrenz added 2 commits August 18, 2015 13:51

reduce calculation in DataFormats + avoid opject creation in SimTracker

95334b3

small change

01f74b5

cmsbuild added this to the Next CMSSW_7_6_X milestone Aug 27, 2015

cmsbuild added reconstruction-pending simulation-pending pending-signatures tests-pending orp-pending comparison-pending labels Aug 27, 2015

ngrenz reviewed Aug 27, 2015
View reviewed changes

inline

7cec228

ngrenz reviewed Aug 27, 2015
View reviewed changes

test of r-phi instead of stereo and glued

b092a27

cmsbuild added tests-started and removed tests-pending labels Aug 27, 2015

cmsbuild added tests-approved and removed tests-started labels Aug 27, 2015

cmsbuild added comparison-available and removed comparison-pending labels Aug 27, 2015

slava77 reviewed Aug 27, 2015
View reviewed changes

ngrenz added 2 commits August 29, 2015 01:22

set geometry at the beginning

6d24341

space

5dda346

cmsbuild added tests-pending and removed tests-rejected labels Aug 28, 2015

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

cmsbuild added tests-approved and removed tests-started labels Aug 29, 2015

cmsbuild added comparison-available and removed comparison-pending labels Aug 29, 2015

cmsbuild added simulation-approved and removed simulation-pending labels Aug 29, 2015

cmsbuild added reconstruction-approved fully-signed and removed pending-signatures reconstruction-pending labels Sep 1, 2015

cmsbuild added orp-approved and removed orp-pending labels Sep 2, 2015

cmsbuild added a commit that referenced this pull request Sep 2, 2015

Merge pull request #10963 from ngrenz/ngrenz_stereo

97c4b3d

ngrenz: testing of stereo optimization

cmsbuild merged commit 97c4b3d into cms-sw:CMSSW_7_6_X Sep 2, 2015

ngrenz deleted the ngrenz_stereo branch December 18, 2015 10:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ngrenz: testing of stereo optimization #10963

ngrenz: testing of stereo optimization #10963

ngrenz commented Aug 27, 2015

ngrenz commented Aug 27, 2015

cmsbuild commented Aug 27, 2015

civanch commented Aug 27, 2015

ngrenz Aug 27, 2015

civanch Aug 27, 2015

ngrenz Aug 27, 2015

civanch Aug 27, 2015

VinInn Aug 27, 2015

ngrenz Aug 27, 2015

VinInn Aug 27, 2015

slava77 commented Aug 27, 2015

cmsbuild commented Aug 27, 2015

cmsbuild commented Aug 27, 2015

civanch commented Aug 27, 2015

cmsbuild commented Aug 27, 2015

cmsbuild commented Aug 27, 2015

slava77 Aug 27, 2015

ngrenz Aug 28, 2015

slava77 commented Aug 28, 2015

cmsbuild commented Aug 28, 2015

cmsbuild commented Aug 28, 2015

cmsbuild commented Aug 29, 2015

cmsbuild commented Aug 29, 2015

civanch commented Aug 29, 2015

slava77 commented Sep 1, 2015

cmsbuild commented Sep 1, 2015

davidlange6 commented Sep 2, 2015

		@@ -112,18 +112,18 @@ class SiStripDetId : public DetId {
		// ---------- inline methods ----------

ngrenz: testing of stereo optimization #10963

ngrenz: testing of stereo optimization #10963

Conversation

ngrenz commented Aug 27, 2015

ngrenz commented Aug 27, 2015

cmsbuild commented Aug 27, 2015

civanch commented Aug 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slava77 commented Aug 27, 2015

cmsbuild commented Aug 27, 2015

cmsbuild commented Aug 27, 2015

civanch commented Aug 27, 2015

cmsbuild commented Aug 27, 2015

cmsbuild commented Aug 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slava77 commented Aug 28, 2015

cmsbuild commented Aug 28, 2015

cmsbuild commented Aug 28, 2015

cmsbuild commented Aug 29, 2015

cmsbuild commented Aug 29, 2015

civanch commented Aug 29, 2015

slava77 commented Sep 1, 2015

cmsbuild commented Sep 1, 2015

davidlange6 commented Sep 2, 2015