overriding inefficient CalosubdetectorGeometry::present function

[Sam-Harper]

First, I suspect this is a blocker for all 10_X MC production as it will take a larger than expected CPU time which may or may not be significant.

Also this is a bit of a drive by bug fix as I happened to come across it and needed it fixed for my own purposes. If this fix requires things beyond what is already here, I'm sure @bsunanda as the author of the problem PR will be able to assist in fixing it promptly.

#21808 updated the CalosubdetectorGeometry::present function with an non-optimally coded version (it has a vector of all valid DetIds and basically compares the current DetId one by one with these to see if its valid).
This cripples the HLT from a timing perspective (doubles it) due to PFRecHit associating its neighbours which involves a lot of calo topology operations which each time requires a call to CalosubdetectorGeometry::present to check its a valid ID. It surprises me that it does not also significantly impact RECO timing, something is odd here. Regardless the HLT timing issue is real and needs to be urgently fixed. This PR solves the issue.

igprof: pre this PR (10_0_1, HLT Physics sample): http://sharper.web.cern.ch/sharper/cgi-bin/igprof-navigator/1001HLTTiming/igreport_default
igprof: post this PR (10_0_1, HLT Physics sample): http://sharper.web.cern.ch/sharper/cgi-bin/igprof-navigator/1001HLTTiming/igreport_ecalGeomFix

Time from ~500ms / event to 250 ms / event based on 1K input.

@Martin-Grunewald FYI

[cmsbuild]

The code-checks are being triggered in jenkins.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22205/3352

[cmsbuild]

A new Pull Request was created by @Sam-Harper for master.

It involves the following packages:

Geometry/EcalAlgo

@cmsbuild, @civanch, @Dr15Jones, @ianna, @mdhildreth can you please review it and eventually sign? Thanks.
@ghellwig, @argiro this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

[Martin-Grunewald]

please test

[Martin-Grunewald]

@Sam-Harper
Thanks!!

[cmsbuild]

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/26056/console Started: 2018/02/14 10:27

[Martin-Grunewald]

@Sam-Harper
I suppose we need a backport PR to 10_0_X?!

[fwyzard]

Yes !

[Sam-Harper]

yep, done, same branch works in both

[fabiocos]

@ianna this is urgent, could you please check?

[cmsbuild]

+1
Tested at: 3147de1
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-22205/26056/summary.html

[cmsbuild]

Comparison job queued.

[lgray]

Sam, couldn't you fix this for everyone at once using an std::unordered_set to contain the list of detids Sunanda is currently iterating through? This would avoid virtual functions and fix it everywhere at once.

[franzoni]

@Sam-Harper

@fabozzi reported that the increase in timing in RECO is not noticeable in fullsim pile-up MC relvals of 10_0_0 with respect to 10_0_0_pre3. Consistent w/ the expectation that the time increase is additive and not multiplicative of the overall time of a give cmsRun.

[cmsbuild]

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

@slava77 comparisons for the following workflows were not done due to missing matrix map:

• /build/cmsbld/jenkins/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-22205/1325.7_TTbar_13_94XNanoAODINPUT+TTbar_13_94XNanoAODINPUT+NANOEDMMC2017+HARVESTNANOAODMC2017

Comparison Summary:

• No significant changes to the logs found
• Reco comparison results: 0 differences found in the comparisons
• DQMHistoTests: Total files compared: 27
• DQMHistoTests: Total histograms compared: 2465360
• DQMHistoTests: Total failures: 1
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 2465190
• DQMHistoTests: Total skipped: 169
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 1.26999999998 KiB( 22 files compared)
• Checked 111 log files, 9 edm output root files, 27 DQM output files

[slava77]

Just for the record from the review of the original feature by Sunanda (from Dec 16)

#21440 (comment)

I expect that in HLT context ::present actually dominates and may even be significant there.
The CPU cost of CaloSubdetectorGeometry::present is up by a factor of 2.5 primarily due to cycling through shared pointer creation and destruction.

the cost to reco is small

[Sam-Harper]

okay so glad to hear it doesnt impact reco, this is good. I was wondering why there werent more complaints.

@lgray : its already a virtual function so I didnt see any harm in overriding it. I agree that they should have used an unordered_set or to be honest, anything else! However I leave it to @bsunanda to that. I needed a quick fix for my studies, I made one and if people dont like it, then the person who caused the problem can fix it :)

[fabiocos]

@lgray as far as I can see, ::present it is already overridden in other specialized geometries, so this PR addresses the cases where it was relying directly on the inefficient base implementation.

Pending a possible more general solution by @bsunanda (m_validIds is a vector since Run1) do you see drawbacks in integrating this now? I understand that while this is not an issue for RECO, it is for HLT timing studies, and it should be fixed in a relatively short time.
According to the igprof reports, the ::present function basically disappear from the report.

[fabiocos]

@Dr15Jones @civanch @ianna could someone for geometry check and comment or sign? This is essentially a performance issue not supposed to change the code output (as also shown by tests)

[civanch]

+1

[cmsbuild]

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

[fabiocos]

+1