SiStripRecHitMatcher.h: remove non-POD VLA

[davidlt]

Clang 3.7.0 will reject non-POD VLAs:

RecoLocalTracker/SiStripRecHitConverter/interface/SiStripRecHitMatcher.h:188:19:
error: variable length array of non-POD element type
'matcherDetails::StereoInfo'

Unknown why this is not triggered in ~3.6.0. Have not checked if SiStripRecHitMatcher::doubleMatch performance changed. I could run IgProf if someone could provide workflow number.

Signed-off-by: David Abdurachmanov David.Abdurachmanov@cern.ch

[cmsbuild]

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

SiStripRecHitMatcher.h: remove non-POD VLA

It involves the following packages:

RecoLocalTracker/SiStripRecHitConverter

@cmsbuild, @cvuosalo, @slava77 can you please review it and eventually sign? Thanks.
@makortel, @forthommel, @yduhm, @GiacomoSguazzoni, @gbenelli, @rovere, @VinInn, @nickmccoll, @jlagram, @gpetruc, @cerati, @threus 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.

[slava77]

igprof on 4.77 step3 out of the box
or 25202.0 step3 with "VALIDATION" removed from the steps

[slava77]

@cmsbuild please test

[cmsbuild]

The tests are being triggered in jenkins.

[cmsbuild]

+1
Tested at: 4833558
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-10860/7482/summary.html

[cmsbuild]

+1
Tested at: 4833558
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-10860/7484/summary.html

[cmsbuild]

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

[cmsbuild]

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

[davidlt]

Tested with 4.77, but could not find SiStripRecHitMatcher::doubleMatch in stack traces. Either it's not collected or below IgProf threshold and does not visible impact.

[slava77]

Hi David,
doubleMatch shows up only with

 #if defined(USE_SSEVECT) || defined(USE_EXTVECT)
 #define DOUBLE_MATCH

[davidlt]

Isn't SSEVECT a default on x86_64?

[slava77]

nm, scratch that comment, I think it's just inlined and doesn't show up in the call stack
https://slava77sk.web.cern.ch/slava77sk/reco/cgi-bin/igprof-navigator/CMSSW_7_6_0_pre3-orig.251721.pp/110

[davidlt]

401 - Unauthorized, which symbol should I check?

[slava77]

TkGluedMeasurementDet::doubleMatch

[slava77]

@davidlt try the link again, I think it should work for you now

[davidlt]

Worked. Done 3 different runs, but probably one longer would have been enough.

Flat profile (cumulative >= 1%)
# Before

    2.9      19.08  void TkGluedMeasurementDet::doubleMatch<TkGluedMeasurementDet::HitCollectorForFastMeasurements>(TrajectoryStateOnSurface const&, MeasurementTrackerEvent const&, TkGluedMeasurementDet::HitCollectorForFastMeasurements&) const [187]
    2.6      16.98  void TkGluedMeasurementDet::doubleMatch<TkGluedMeasurementDet::HitCollectorForFastMeasurements>(TrajectoryStateOnSurface const&, MeasurementTrackerEvent const&, TkGluedMeasurementDet::HitCollectorForFastMeasurements&) const [212]
    2.6      16.82  void TkGluedMeasurementDet::doubleMatch<TkGluedMeasurementDet::HitCollectorForFastMeasurements>(TrajectoryStateOnSurface const&, MeasurementTrackerEvent const&, TkGluedMeasurementDet::HitCollectorForFastMeasurements&) const [222]

# After
    2.7      17.34  void TkGluedMeasurementDet::doubleMatch<TkGluedMeasurementDet::HitCollectorForFastMeasurements>(TrajectoryStateOnSurface const&, MeasurementTrackerEvent const&, TkGluedMeasurementDet::HitCollectorForFastMeasurements&) const [208]
    2.6      17.27  void TkGluedMeasurementDet::doubleMatch<TkGluedMeasurementDet::HitCollectorForFastMeasurements>(TrajectoryStateOnSurface const&, MeasurementTrackerEvent const&, TkGluedMeasurementDet::HitCollectorForFastMeasurements&) const [212]
    2.7      17.87  void TkGluedMeasurementDet::doubleMatch<TkGluedMeasurementDet::HitCollectorForFastMeasurements>(TrajectoryStateOnSurface const&, MeasurementTrackerEvent const&, TkGluedMeasurementDet::HitCollectorForFastMeasurements&) const [191]

This was a single run over 100 events, I can re-run, but I guess nothing changed much as expected.

[VinInn]

In any case, if we decide to engage to remove all VLA I suggest to move
http://cmslxr.fnal.gov/lxr/source/CommonTools/Utils/interface/DynArray.h?v=CMSSW_7_6_0_pre3
in a more central place and use that.

[slava77]

+1

for #10860 4833558

• change is pretty simple
• jenkins tests OK

[cmsbuild]

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

[VinInn]

Let me object to this change done in this way.
I suggested what I think it is a better approach.
At least a comment in the code would have been useful.
Now it is lost forever

[davidlange6]

sorry didn’t read carefully.. we can revert if desired on Monday so that its not lost.

On Aug 23, 2015, at 7:59 AM, Vincenzo Innocente notifications@github.com wrote:

Let me object to this change done in this way.
I suggested a better approach.
At least a comment in the code would have been useful.
Now it is lost forever

—
Reply to this email directly or view it on GitHub.

[slava77]

@VinInn
you had more than 2 days to make a more explicit comment that you don't want the switch to a vector.
Please submit a new PR with a change you prefer instead of this or let David (A) know what to change.

[VinInn]

I am sorry if my suggestion did not come across in a clear way.
I suggest to move DynArray.h in a more central place and use it consistently in place of C-style VLA.

[slava77]

I suppose a "central place" would mean some place under CORE?
@Dr15Jones, any suggestions?

CommonTools/Utils is not the worst place to be.

I recall there were other changes to vector(0) from vla[0].

Regarding this PR, I suggest not to revert (which will bring back the issue/error message), but instead to modify in a better way.

[davidlt]

The problem is only with non-POD VLAs. @VinInn suggest to change that to POD VLAs, which would be stored on stack instead of heap. Technically this avoid allocation on heap and maybe could lower register pressure. Because once it's on a stack compiler will always know the location of item (you don't need an extra register for storing base address). Again, depends on how compiler does its job.

I don't see significant impact on performance between non-POD VLA and std::vector with fixed size on IgProf. Thus it's your call.

[Dr15Jones]

CommonTools/Utils doesn't seem so bad to me. We definitely would not want it in any DataFormats package. It could also go into Utilities/General.
One note on DynArray, shouldn't the copy constructor and operator= be deleted? If the defaults were used the contents of the array would be deleted twice.