Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DD4hep] DD Filtered View: Add Debug Print, RegExp Speed Up and Clean Up #29208

Merged
merged 8 commits into from Mar 18, 2020

Conversation

ianna
Copy link
Contributor

@ianna ianna commented Mar 15, 2020

PR description:

  • Remove the need to an obscure mergeSpecifics() method because it does not merge anything, but filters the specifics registry. Note, the method is still accessible, but will be cleaned up in a later PR.
  • Instead of mergeSpecifics(), now DDFilteredView can be constructed from a DDCompactView and a DDFilter:
  const cms::DDFilter filter("OnlyForHcalSimNumbering", "HCAL");
  cms::DDFilteredView fv(cpv, filter);
  • Use regexp as keys in a Filter to speed it up a bit
  • Add a print filter method to allow easier debugging
  • Remove methods that are not needed any more
  • Use Hcal parameters builder as a user story study

@bsunanda - you may want to remove the BenchmarkGrd and disable the MessageLogger for the Geometry.

It's slightly slower to merge the filter registry:

< Benchmark 'DTGeometryESProducer Filter Registry' took 0.057576 millis
---
> Benchmark 'DTGeometryESProducer Filter Registry' took 0.052404 millis

but, it's ~8% faster to build the DT geometry:

< Benchmark 'DTGeometryESProducer' took 8082.02 millis
---
> Benchmark 'DTGeometryESProducer' took 8713.86 millis

PR validation:

if this PR is a backport please specify the original PR and why you need to backport that PR:

Before submitting your pull requests, make sure you followed this checklist:

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29208/14197

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @ianna (Ianna Osborne) for master.

It involves the following packages:

DetectorDescription/DDCMS
Geometry/DTGeometryBuilder

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

cms-bot commands are listed here

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29208/14209

  • This PR adds an extra 40KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

Copy link
Contributor Author

@ianna ianna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bsunanda - time retrieving material names has been reduced from 70996.3 millis to 49157.4 millis

SpecPar names OnlyForHcalSimNumbering, HCAL:
hcal
Benchmark 'HcalSimParametersFromDD HCal materials OnlyForHcalSimNumbering, HCAL' took 70996.3 mill-is

due to the change in the selection from //.*HEScintillator.* to //HEScintillator.*:

hcal
   //HBS.*
   //HTS.*
   //HES.*
   //HVQF
   //HCal
   //HBS.*
   //HTS.*
   //.*HEScintillator.*
   //HVQX
   //VcalElecCathode
   //FibreBundle.*
   //VcalFibreBundleL.*
   //VcalFibreBundleR.*
   //CALO
   //MBAT
   //MBBT.*
   //VCAL
hcal
   //HBS.*
   //HTS.*
   //HES.*
   //HVQF
   //HCal
   //HBS.*
   //HTS.*
   //HEScintillator.*
   //HVQX
   //VcalElecCathode
   //FibreBundle.*
   //VcalFibreBundleL.*
   //VcalFibreBundleR.*
   //CALO
   //MBAT
   //MBBT.*
   //VCAL
Benchmark 'HcalSimParametersFromDD HCal materials OnlyForHcalSimNumbering, HCAL' took 48402.1 millis

The paths seem to be fine to use this type of selection:

world_volume/OCMS_1/CMSE_1/OQUA_1/VcalElectronics_1/VcalEleBox_1/VcalElecRBX_1/VcalElecPMT_1/VcalElecCathode_1
    world_volume/OCMS_1/CMSE_1/OQUA_1/FibreContainer_1/FibreBoxTrap_1/FibreBoxL_1/FibreBundle12_2
    world_volume/OCMS_1/CMSE_1/OQUA_1/VcalFibreBundleContainer_1/VcalFibreBundleSector_1/VcalFibreBundleHalfSectorL_1/VcalFibreBundleL11_2

    world_volume/OCMS_1/CMSE_1/VCAL_5002/HVQF_1/HVQX_18

Here is the summary before the changes:

TimeModule> 1 1 hpa HcalParametersAnalyzer 183.178
TimeModule> 1 1 p1 PathStatusInserter 0.00127196
TimeModule> 1 1 TriggerResults TriggerResultInserter 3.60012e-05
TimeEvent> 1 1 183.183
MemoryReport> Peak virtual size 536.102 Mbytes
 Key events increasing vsize: 
[0] run: 0 lumi: 0 event: 0  vsize = 0 deltaVsize = 0 rss = 0 delta = 0
[0] run: 0 lumi: 0 event: 0  vsize = 0 deltaVsize = 0 rss = 0 delta = 0
[1] run: 1 lumi: 1 event: 1  vsize = 536.102 deltaVsize = 0 rss = 96.9062 delta = 0
[0] run: 0 lumi: 0 event: 0  vsize = 0 deltaVsize = 0 rss = 0 delta = 0
[0] run: 0 lumi: 0 event: 0  vsize = 0 deltaVsize = 0 rss = 0 delta = 0
[0] run: 0 lumi: 0 event: 0  vsize = 0 deltaVsize = 0 rss = 0 delta = 0
[0] run: 0 lumi: 0 event: 0  vsize = 0 deltaVsize = 0 rss = 0 delta = 0
[1] run: 1 lumi: 1 event: 1  vsize = 536.102 deltaVsize = 0 rss = 96.9062 delta = 0
TimeReport> Time report complete in 186.166 seconds
 Time Summary: 
 - Min event:   183.183
 - Max event:   183.183
 - Avg event:   183.183
 - Total loop:  183.186
 - Total init:  2.9769
 - Total job:   186.166
 - EventSetup Lock:   6.19888e-06
 - EventSetup Get:   191.965
 Event Throughput: 0.00545894 ev/s
 CPU Summary: 
 - Total loop:  183.018
 - Total init:  1.43986
 - Total extra: 0
 - Total job:   184.461
 Processing Summary: 
 - Number of Events:  1
 - Number of Global Begin Lumi Calls:  1
 - Number of Global Begin Run Calls: 1

and after:

TimeModule> 1 1 hpa HcalParametersAnalyzer 140.614
TimeModule> 1 1 p1 PathStatusInserter 0.00015378
TimeModule> 1 1 TriggerResults TriggerResultInserter 2.40803e-05
TimeEvent> 1 1 140.614
MemoryReport> Peak virtual size 542.105 Mbytes
 Key events increasing vsize: 
[0] run: 0 lumi: 0 event: 0  vsize = 0 deltaVsize = 0 rss = 0 delta = 0
[0] run: 0 lumi: 0 event: 0  vsize = 0 deltaVsize = 0 rss = 0 delta = 0
[1] run: 1 lumi: 1 event: 1  vsize = 542.105 deltaVsize = 0 rss = 137.961 delta = 0
[0] run: 0 lumi: 0 event: 0  vsize = 0 deltaVsize = 0 rss = 0 delta = 0
[0] run: 0 lumi: 0 event: 0  vsize = 0 deltaVsize = 0 rss = 0 delta = 0
[0] run: 0 lumi: 0 event: 0  vsize = 0 deltaVsize = 0 rss = 0 delta = 0
[0] run: 0 lumi: 0 event: 0  vsize = 0 deltaVsize = 0 rss = 0 delta = 0
[1] run: 1 lumi: 1 event: 1  vsize = 542.105 deltaVsize = 0 rss = 137.961 delta = 0
TimeReport> Time report complete in 143.36 seconds
 Time Summary: 
 - Min event:   140.614
 - Max event:   140.614
 - Avg event:   140.614
 - Total loop:  140.616
 - Total init:  2.74258
 - Total job:   143.36
 - EventSetup Lock:   1.38283e-05
 - EventSetup Get:   149.858
 Event Throughput: 0.00711159 ev/s
 CPU Summary: 
 - Total loop:  140.488
 - Total init:  1.4459
 - Total extra: 0
 - Total job:   141.936
 Processing Summary: 
 - Number of Events:  1
 - Number of Global Begin Lumi Calls:  1
 - Number of Global Begin Run Calls: 1

DetectorDescription/DDCMS/interface/Filter.h Outdated Show resolved Hide resolved
@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29208/14210

  • This PR adds an extra 36KB to repository

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@ianna
Copy link
Contributor Author

ianna commented Mar 17, 2020

please test

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29208/14227

  • This PR adds an extra 96KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 17, 2020

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/5218/console Started: 2020/03/17 12:23

@cmsbuild
Copy link
Contributor

Pull request #29208 was updated. @civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild can you please check and sign again.

@cmsbuild
Copy link
Contributor

+1
Tested at: 0bb2c0c
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-36cfbe/5218/summary.html
CMSSW: CMSSW_11_1_X_2020-03-16-2300
SCRAM_ARCH: slc7_amd64_gcc820

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2680593
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2680273
  • DQMHistoTests: Total skipped: 319
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
  • Checked 147 log files, 16 edm output root files, 34 DQM output files

@civanch
Copy link
Contributor

civanch commented Mar 18, 2020

+1

Carl, I think now PR is fine.

@cmsbuild
Copy link
Contributor

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, @silviodonato, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

@silviodonato
Copy link
Contributor

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants