Improve DD4hep workflow perf, step 3: Also reduced the number of paths (with regex) looped over for HCAL #32540

ghugo83 · 2020-12-18T11:16:34Z

This is based on the exact same observation as in #32505 : when SpecPar blocks have the same name, all their paths are stacked together. This results in irrelevant paths (with regex) being looped over, hence a performance penalty.

#32505 proposed a simple fix for the Tracker (just addressing the issue directly in the XMLs); this is the equivalent for HCAL.
Unnecessary HCAL paths (with regex) were looped over, for example at https://github.com/cms-sw/cmssw/blob/master/SimG4Core/Geometry/src/DD4hep_DDG4Builder.cc#L62

Summary of performance gains for FULL step 1 with DD4hep workflow (entire step with all initializations + XML parsing + CMS geo construction, up to event generation).
ZMM, Run3, on my local (obviously only the order of magnitude is relevant).

Improve DD4hep workflow perf, step 1: All paths from SpecPar blocks with same name are stacked together #32505: ~205s -> ~140s
Improve DD4hep workflow perf, step 2: Remove regex from Phase 1 trackerRecoMaterial.xml #32511: ~140s -> ~85s
This PR: ~85s -> ~80s
DDD is at ~42 s

NB: The last occurrence of this issue (for Run 3) is in ZDC (zdcsens.xml and zdcProdCuts.xml). I will not address the ZDC case, as the paths there have no regex, hence the perf penalty is completely negligible, not worth the XMLs files versioning, risk of regression, etc.

NB 2: #32511 should be merged first. This branch is on top of my working branch, hence the diff with master will become relevant as soon as #32511 is merged.

@ianna @civanch @cvuosalo

…erial.xml. I presume the pixfwdblade.*Zplus had been done to distinguish from the pixfwdblade[0-9]:PixelForwardBlade. But this is not recessary, because the pixfwdblade[0-9]:PixelForwardBlade are not called in any geo scenario where data/PhaseI/trackerRecoMaterial.xml is used.

…these paths are not looped over when not needed anyway, hence this commit has no effect on workflow perf).

… has very small, but non-null, impact on perf.

…erial.xml. Very significant perf improvement. This version is compatible to work on both DDD and dd4hep cases.

…ngMaterialGroups_ForPhaseI.xml, version working for both DDD and dd4hep.

…try scenarios.

…al.xml. This has no visible impact on perf though (~85s before and after this commit), but does not harm to have a more compact file. Regex removal instead (in the previous commits) had a big impact: full DD4hep step 1 up to event generation: ~140s -> ~85s !!

…s groups.

…lysis/test

…ow (~5 s gain in my local for overall step 1 XML parsing / geo construction). When SpecPar blocks jave same name, all paths are gathered unnecessarily, leading to loop over more paths (with regex) than needed).

…alsenspmf.xml, HcalProdCuts.xml

cmsbuild · 2020-12-18T11:22:02Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32540/20492

This PR adds an extra 128KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File Configuration/Geometry/python/dict2021Geometry.py modified in PR(s): Improve DD4hep workflow perf, step 2: Remove regex from Phase 1 trackerRecoMaterial.xml #32511
- File Geometry/CMSCommonData/data/dd4hep/cmsExtendedGeometry2021.xml modified in PR(s): Improve DD4hep workflow perf, step 2: Remove regex from Phase 1 trackerRecoMaterial.xml #32511
- File Geometry/CMSCommonData/data/dd4hep/cmsExtendedGeometry2021FlatMinus05Percent.xml modified in PR(s): Improve DD4hep workflow perf, step 2: Remove regex from Phase 1 trackerRecoMaterial.xml #32511
- File Geometry/CMSCommonData/data/dd4hep/cmsExtendedGeometry2021FlatMinus10Percent.xml modified in PR(s): Improve DD4hep workflow perf, step 2: Remove regex from Phase 1 trackerRecoMaterial.xml #32511
- File Geometry/CMSCommonData/data/dd4hep/cmsExtendedGeometry2021FlatPlus05Percent.xml modified in PR(s): Improve DD4hep workflow perf, step 2: Remove regex from Phase 1 trackerRecoMaterial.xml #32511
- File Geometry/CMSCommonData/data/dd4hep/cmsExtendedGeometry2021FlatPlus10Percent.xml modified in PR(s): Improve DD4hep workflow perf, step 2: Remove regex from Phase 1 trackerRecoMaterial.xml #32511
- File Geometry/CMSCommonData/data/dd4hep/cmsExtendedGeometry2021ZeroMaterial.xml modified in PR(s): Improve DD4hep workflow perf, step 2: Remove regex from Phase 1 trackerRecoMaterial.xml #32511
- File Geometry/CMSCommonData/python/cmsExtendedGeometry2021FlatMinus05PercentXML_cfi.py modified in PR(s): Improve DD4hep workflow perf, step 2: Remove regex from Phase 1 trackerRecoMaterial.xml #32511
- File Geometry/CMSCommonData/python/cmsExtendedGeometry2021FlatMinus10PercentXML_cfi.py modified in PR(s): Improve DD4hep workflow perf, step 2: Remove regex from Phase 1 trackerRecoMaterial.xml #32511
- File Geometry/CMSCommonData/python/cmsExtendedGeometry2021FlatPlus05PercentXML_cfi.py modified in PR(s): Improve DD4hep workflow perf, step 2: Remove regex from Phase 1 trackerRecoMaterial.xml #32511
- File Geometry/CMSCommonData/python/cmsExtendedGeometry2021FlatPlus10PercentXML_cfi.py modified in PR(s): Improve DD4hep workflow perf, step 2: Remove regex from Phase 1 trackerRecoMaterial.xml #32511
- File Geometry/CMSCommonData/python/cmsExtendedGeometry2021XML_cfi.py modified in PR(s): Improve DD4hep workflow perf, step 2: Remove regex from Phase 1 trackerRecoMaterial.xml #32511
- File Geometry/CMSCommonData/python/cmsExtendedGeometry2021ZeroMaterialXML_cfi.py modified in PR(s): Improve DD4hep workflow perf, step 2: Remove regex from Phase 1 trackerRecoMaterial.xml #32511
- File Geometry/TrackerRecoData/data/PhaseI/v1/trackerRecoMaterial.xml modified in PR(s): Improve DD4hep workflow perf, step 2: Remove regex from Phase 1 trackerRecoMaterial.xml #32511
- File SimTracker/TrackerMaterialAnalysis/data/v1/trackingMaterialGroups_ForPhaseI.xml modified in PR(s): Improve DD4hep workflow perf, step 2: Remove regex from Phase 1 trackerRecoMaterial.xml #32511
- File SimTracker/TrackerMaterialAnalysis/test/dumpFullXML_ForPhaseI.py modified in PR(s): Improve DD4hep workflow perf, step 2: Remove regex from Phase 1 trackerRecoMaterial.xml #32511
- File SimTracker/TrackerMaterialAnalysis/test/listGroups.py modified in PR(s): Improve DD4hep workflow perf, step 2: Remove regex from Phase 1 trackerRecoMaterial.xml #32511
- File SimTracker/TrackerMaterialAnalysis/test/trackingMaterialAnalyser_ForPhaseI.py modified in PR(s): Improve DD4hep workflow perf, step 2: Remove regex from Phase 1 trackerRecoMaterial.xml #32511

cmsbuild · 2020-12-18T11:22:26Z

A new Pull Request was created by @ghugo83 for master.

It involves the following packages:

Configuration/Geometry
Geometry/CMSCommonData
Geometry/HcalCommonData
Geometry/HcalSimData
Geometry/TrackerRecoData
SimTracker/TrackerMaterialAnalysis

@civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @kpedro88 can you please review it and eventually sign? Thanks.
@fabiocos, @vargasa, @makortel, @threus, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @Martin-Grunewald, @apsallid, @ebrondol, @mtosi, @dgulhan, @slomeo this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

cvuosalo · 2020-12-18T18:32:23Z

Geometry/HcalCommonData/data/v1/hcalsenspmf.xml

@@ -0,0 +1,18 @@
+<?xml version="1.0"?>


Full path is wrong. It should be Geometry/HcalCommonData/data/hcalsenspmf/v1/hcalsenspmf.xml.

The previous path was Geometry/HcalCommonData/data/hcalsenspmf.xml

Yes, the new directory hcalsenspmf has to be created.

cvuosalo · 2020-12-18T18:36:31Z

Geometry/HcalSimData/data/v1/HcalProdCuts.xml

@@ -0,0 +1,15 @@
+<?xml version="1.0"?>


Full path needs to be Geometry/HcalSimData/data/HcalProdCuts/2021/v2/HcalProdCuts.xml. Make sure you derive v2 from the v1 file.

ghugo83 · 2020-12-18T18:39:20Z

Geometry/CMSCommonData/data/dd4hep/cmsExtendedGeometry2021ZeroMaterial.xml

@@ -355,7 +355,7 @@
    <Include ref='Geometry/VeryForwardData/data/CTPPS_Pixel_Sens.xml'/>
    <Include ref='Geometry/VeryForwardData/data/CTPPS_2018/RP_Dist_Beam_Cent.xml'/>
    <Include ref='Geometry/EcalSimData/data/ecalsens.xml'/>
-    <Include ref='Geometry/HcalCommonData/data/hcalsenspmf.xml'/>


cmsbuild · 2021-01-08T01:00:44Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b22d83/12015/summary.html
COMMIT: 8121aeb
CMSSW: CMSSW_11_3_X_2021-01-07-1100/slc7_amd64_gcc900

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 3038 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2716967
DQMHistoTests: Total failures: 7799
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2709146
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
Checked 156 log files, 37 edm output root files, 37 DQM output files

ghugo83 · 2021-01-08T09:53:38Z

@ghugo83 If I understand correctly, the SpecPar name is is used to group SpecPars together. That is, SpecPars with the same name in different files are all grouped together.

Yes exactly

Otherwise, the SpecPar name is not used. Is that correct?

No, in the codebase apparently, the SpecPar name is sometimes used to access a specific SpecPar directly.
Using DDFilteredView::get<std::vector<std::string>>, specPar(name), etc

ghugo83 · 2021-01-08T10:18:59Z

What version of Geometry/TrackerRecoData/data/PhaseI/v1/trackerRecoMaterial.xml is included in this PR? There is already a "v1" in the master.
Why is SimTracker/TrackerMaterialAnalysis/data/trackingMaterialGroups_ForPhaseI.xml included in this PR, along with SimTracker/TrackerMaterialAnalysis/data/trackingMaterialGroups_ForPhaseI/v1/trackingMaterialGroups_ForPhaseI.xml?

@cvuosalo Yes that's a weird feature of cms-git, which shows the diff of the PR branch with master at the time the PR was created, instead of at the present time.
The early commits in this PR are already included in master on other topics. But they still appear in the diff with master, because master did not contain them at PR creation time.
I invite you to look at the real diff of this PR branch (ghugo83:dd4hep_perf_3) with present master: master...ghugo83:dd4hep_perf_3

As you can see, this PR does not touch trackerRecoMaterial.xml.
It also does not change any trackingMaterialGroups XML file, but it just places them in a dedicated trackingMaterialGroups_ForPhaseI repo: data/v1/ -> data/trackingMaterialGroups_ForPhaseI/v1, as you suggested in a previous comment

cvuosalo · 2021-01-08T20:11:47Z

@ghugo83 If I understand correctly, the SpecPar name is is used to group SpecPars together. That is, SpecPars with the same name in different files are all grouped together.

Yes exactly

Otherwise, the SpecPar name is not used. Is that correct?

No, in the codebase apparently, the SpecPar name is sometimes used to access a specific SpecPar directly.
Using DDFilteredView::get<std::vector<std::string>>, specPar(name), etc

@ghugo83 It looks like in the case of the SpecPar names you changed, they are not used anywhere in the code, so it is safe to change the names.

cvuosalo · 2021-01-08T20:24:56Z

There are a lot of comparison test differences. There are differences in GEM quantities, which would not seem to have anything to do with the PR changes. Some of the differences may just be statistical fluctuations. @ghugo83 Can you comment?

ghugo83 · 2021-01-11T14:08:12Z

@ghugo83 It looks like in the case of the SpecPar names you changed, they are not used anywhere in the code, so it is safe to change the names.

Yes it practice, there were few use cases of "hcal' as a SpecPar name, but only for 1 XML (out of the 4 XMLs with same SpecPar names), so I have just renamed the 3 others.

ghugo83 · 2021-01-11T14:11:48Z

There are a lot of comparison test differences. There are differences in GEM quantities, which would not seem to have anything to do with the PR changes. Some of the differences may just be statistical fluctuations. @ghugo83 Can you comment?

Yes with 10 events the results themselves, in a sense, are statistical fluctuations.
Though, this should point the dependencies of the PR diff to the results.

This PR should change the ECal SimHits.
It is not clear to me why GEM would be affected though: maybe because once SimHits in a certain area are modified, it can have an impact elsewhere?

kpedro88 · 2021-01-11T16:22:23Z

Any changes in the SIM step will impact the random number sequence, which can cause fluctuations in any detector for any subsequent events.

ghugo83 · 2021-01-11T16:26:21Z

Any changes in the SIM step will impact the random number sequence, which can cause fluctuations in any detector for any subsequent events.

Ok thanks for confirming this

cvuosalo · 2021-01-11T17:01:20Z

I guess we can leave it to the validation process to check whether there is any real problem in the GEMs.

cvuosalo · 2021-01-11T17:01:26Z

+1

civanch · 2021-01-11T17:26:05Z

+1

the difference is seen in DD4Hep WF, so it is the bug fix for DD4Hep.

civanch · 2021-01-11T17:26:16Z

urgent

srimanob · 2021-01-12T02:14:50Z

+upgrade

To follow up after this PR:

Keep eyes on validation report @chayanit

cmsbuild · 2021-01-12T02:15:12Z

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

qliphy · 2021-01-12T02:27:42Z

+1

ghugo83 added 14 commits December 16, 2020 15:19

Also remove regex from trackingMaterialGroups_ForPhaseI.xml (but now …

f52b55b

…these paths are not looped over when not needed anyway, hence this commit has no effect on workflow perf).

Truncate the unnecessary Tracker/subdetector heads in the paths. This…

cc83df3

… has very small, but non-null, impact on perf.

Remove regex from Geometry/TrackerRecoData/data/PhaseI/trackerRecoMat…

51565c8

…erial.xml. Very significant perf improvement. This version is compatible to work on both DDD and dd4hep cases.

Also remove regex from SimTracker/TrackerMaterialAnalysis/data/tracki…

aaf4029

…ngMaterialGroups_ForPhaseI.xml, version working for both DDD and dd4hep.

Create v1 for Phase 1 trackerRecoMaterial.xml

0880035

Call no-regex version of Phase1/trackerRecoMaterial.xml in 2021 geome…

95053db

…try scenarios.

Also load the no-regex XML file in materials test.

0ff94d6

Moved trackingMaterialGroups_ForPhaseI.xml to v1

c2f90bb

Update 2021 geometry scenarios to call v1 version of trackingMaterial…

f7702d3

…s groups.

Also update MaterialAnalyzer scripts in SimTracker/TrackerMaterialAna…

8b3d69f

…lysis/test

Magic: rename SpecPar sections in HCAL allow speedup in DD4hep workfl…

b0854c4

…ow (~5 s gain in my local for overall step 1 XML parsing / geo construction). When SpecPar blocks jave same name, all paths are gathered unnecessarily, leading to loop over more paths (with regex) than needed).

Update 2021 scenarios to call v1 versions of hcalSimNumbering.xml, hc…

389c28e

…alsenspmf.xml, HcalProdCuts.xml

cmsbuild added this to the CMSSW_11_3_X milestone Dec 18, 2020

cmsbuild added code-checks-pending geometry-pending orp-pending pending-signatures simulation-pending tests-pending upgrade-pending labels Dec 18, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 18, 2020

ghugo83 mentioned this pull request Dec 18, 2020

Improve DD4hep workflow perf, step 4: Improve performance of GeometricDet construction in old DD (DDD) and DD4hep #32544

Merged

cvuosalo reviewed Dec 18, 2020

View reviewed changes

ghugo83 commented Dec 18, 2020

View reviewed changes

cmsbuild added tests-approved and removed tests-started labels Jan 8, 2021

cmsbuild added geometry-approved and removed geometry-pending labels Jan 11, 2021

cmsbuild added simulation-approved urgent and removed simulation-pending labels Jan 11, 2021

cmsbuild added fully-signed upgrade-approved and removed pending-signatures upgrade-pending labels Jan 12, 2021

cmsbuild added orp-approved and removed orp-pending labels Jan 12, 2021

cmsbuild merged commit 0797983 into cms-sw:master Jan 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve DD4hep workflow perf, step 3: Also reduced the number of paths (with regex) looped over for HCAL #32540

Improve DD4hep workflow perf, step 3: Also reduced the number of paths (with regex) looped over for HCAL #32540

ghugo83 commented Dec 18, 2020 •

edited

cmsbuild commented Dec 18, 2020

cmsbuild commented Dec 18, 2020

cvuosalo Dec 18, 2020

ghugo83 Dec 18, 2020

cvuosalo Dec 18, 2020

cvuosalo Dec 18, 2020

ghugo83 Dec 18, 2020

cmsbuild commented Jan 8, 2021

ghugo83 commented Jan 8, 2021 •

edited

ghugo83 commented Jan 8, 2021 •

edited

cvuosalo commented Jan 8, 2021

cvuosalo commented Jan 8, 2021

ghugo83 commented Jan 11, 2021

ghugo83 commented Jan 11, 2021 •

edited

kpedro88 commented Jan 11, 2021

ghugo83 commented Jan 11, 2021

cvuosalo commented Jan 11, 2021

cvuosalo commented Jan 11, 2021

civanch commented Jan 11, 2021

civanch commented Jan 11, 2021

srimanob commented Jan 12, 2021

cmsbuild commented Jan 12, 2021

qliphy commented Jan 12, 2021

Improve DD4hep workflow perf, step 3: Also reduced the number of paths (with regex) looped over for HCAL #32540

Improve DD4hep workflow perf, step 3: Also reduced the number of paths (with regex) looped over for HCAL #32540

Conversation

ghugo83 commented Dec 18, 2020 • edited

cmsbuild commented Dec 18, 2020

cmsbuild commented Dec 18, 2020

cvuosalo Dec 18, 2020

Choose a reason for hiding this comment

ghugo83 Dec 18, 2020

Choose a reason for hiding this comment

cvuosalo Dec 18, 2020

Choose a reason for hiding this comment

cvuosalo Dec 18, 2020

Choose a reason for hiding this comment

ghugo83 Dec 18, 2020

Choose a reason for hiding this comment

cmsbuild commented Jan 8, 2021

Comparison Summary

ghugo83 commented Jan 8, 2021 • edited

ghugo83 commented Jan 8, 2021 • edited

cvuosalo commented Jan 8, 2021

cvuosalo commented Jan 8, 2021

ghugo83 commented Jan 11, 2021

ghugo83 commented Jan 11, 2021 • edited

kpedro88 commented Jan 11, 2021

ghugo83 commented Jan 11, 2021

cvuosalo commented Jan 11, 2021

cvuosalo commented Jan 11, 2021

civanch commented Jan 11, 2021

civanch commented Jan 11, 2021

srimanob commented Jan 12, 2021

cmsbuild commented Jan 12, 2021

qliphy commented Jan 12, 2021

ghugo83 commented Dec 18, 2020 •

edited

ghugo83 commented Jan 8, 2021 •

edited

ghugo83 commented Jan 8, 2021 •

edited

ghugo83 commented Jan 11, 2021 •

edited