[DD4hep] Filtered View Performance - Step 2 #31641

ianna · 2020-10-01T15:45:58Z

PR description:

Allow caching of a SpecPar selected by an attribute for the current Node. The assumption is that multiple parameters are associated with the same SpecPar. The first get<double> call would do an expensive search and the subsequent getNextValue will piggyback on it, fetching a value selected by another attribute from the same SpecPar.

This works as follows (see the unit test):

double radLength = fview.get<double>("TrackerRadLength");
double xi = fview.getNextValue("TrackerXi");

The assumption is that both TrackerRadLength and TrackerXi should be part of one SpecPar:

   <SpecPar eval="true" name="TrackerRecMaterialTOBLayer1_Z0">
      <PartSelector path="//Tracker/TOB/TOBLayer1/TOBRod1/TOBRod1L/TOBRodCentral1L/TOBModule0[6]/TOBWaferRphi0/TOBActiveRphi0" />
      <PartSelector path="//Tracker/TOB/TOBLayer1/TOBRod1/TOBRod1L/TOBRodCentral1L/TOBModule0[6]/TOBWaferSter0/TOBActiveSter0" />
      <PartSelector path="//Tracker/TOB/TOBLayer1/TOBRod1/TOBRod1H/TOBRodCentral1H/TOBModule0[1]/TOBWaferRphi0/TOBActiveRphi0" />
      <PartSelector path="//Tracker/TOB/TOBLayer1/TOBRod1/TOBRod1H/TOBRodCentral1H/TOBModule0[1]/TOBWaferSter0/TOBActiveSter0" />
      <Parameter name="TrackerRadLength" value="0.023298" />
      <Parameter name="TrackerXi" value="4.98908e-05" />
    </SpecPar>

The assumption seems to be reasonable, but of cause, there are exceptions:
https://cmssdt.cern.ch/lxr/source/Geometry/TrackerCommonData/data/PhaseI/trackerStructureTopology.xml#0321

   <SpecPar name="PixelROCRowsPar_D1">
      <PartSelector path="//.*:PixelForwardSensor" />
      <Parameter name="PixelROCRows" value="80"/>
  </SpecPar>
  <SpecPar name="PixelROCRowsPar_D2">
      <PartSelector path="//.*:PixelForwardSensor" />
      <Parameter name="PixelROCRows" value="80"/>
  </SpecPar>
  <SpecPar name="PixelROCRowsPar_D3">
      <PartSelector path="//.*:PixelForwardSensor" />
      <Parameter name="PixelROCRows" value="80"/>
  </SpecPar>

@ghugo83 and @vargasa - any idea what should happen here? Is it assumed that PixelROCRows is a vector?

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:

verify that the PR is really intended for the chosen branch
verify that changes follow CMS Naming, Coding, And Style Rules
verify that the PR passes the basic test procedure suggested in the CMSSW PR instructions

cmsbuild · 2020-10-01T15:46:24Z

The code-checks are being triggered in jenkins.

ianna · 2020-10-01T15:51:04Z

please test

cmsbuild · 2020-10-01T15:53:26Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31641/18726

This PR adds an extra 36KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File DetectorDescription/DDCMS/interface/DDFilteredView.h modified in PR(s): [DD4hep] Filtered View is not a Shape #31603
- File DetectorDescription/DDCMS/plugins/dd4hep/DDDefinitions2Objects.cc modified in PR(s): [DD4hep] Filtered View Performance Step 1 #31630
- File DetectorDescription/DDCMS/src/DDFilteredView.cc modified in PR(s): [DD4hep] Filtered View Performance Step 1 #31630
- File DetectorDescription/DDCMS/test/DDFilteredView.goto.cppunit.cc modified in PR(s): [DD4hep] Filtered View Performance Step 1 #31630

cmsbuild · 2020-10-01T15:53:52Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-10-01-1100/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/9686/console Started: 2020/10/01 17:54

cmsbuild · 2020-10-01T15:53:53Z

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

It involves the following packages:

DetectorDescription/DDCMS

@civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild can you please review it and eventually sign? Thanks.
@fabiocos, @slomeo, @vargasa 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

ghugo83 · 2020-10-01T16:00:25Z

   <SpecPar name="PixelROCRowsPar_D1">
      <PartSelector path="//.*:PixelForwardSensor" />
      <Parameter name="PixelROCRows" value="80"/>
  </SpecPar>
  <SpecPar name="PixelROCRowsPar_D2">
      <PartSelector path="//.*:PixelForwardSensor" />
      <Parameter name="PixelROCRows" value="80"/>
  </SpecPar>
  <SpecPar name="PixelROCRowsPar_D3">
      <PartSelector path="//.*:PixelForwardSensor" />
      <Parameter name="PixelROCRows" value="80"/>
  </SpecPar>

I dont think a vector is needed, just an int.
If you go on looping inside the FilteredView, you will just get a vector full of 80.

This represents the number of rows PER ROC, for pixel sensors.
In this file, one group is all sensors belonging to D1, another group is all sensors belonging to D2, etc.

Here in the XML, there is no point having several blocks, because there are the same path anyway.
This is just done for clarity for the reader, I would say.
The SpecPar name does not change anything.

This is exactly equivalent to:

   <SpecPar name="PixelROCRowsPar_AllDisks">
      <PartSelector path="//.*:PixelForwardSensor" />
      <Parameter name="PixelROCRows" value="80"/>
  </SpecPar>

So the behavior of the FilteredView should be: if ever the node path is matching //.*:PixelForwardSensor, I take
<Parameter name="PixelROCRows" value="80"/>
and I can safely stop.

When one wants to assign different values to a specific layer / disk, the paths must be different anyway (that difference sometimes lies only in the namespace, by the way).
And in that case, the FilteredView will not stop until she finds a node matching the path anyway, to get the value specific to that path.

ianna · 2020-10-01T16:09:47Z

@ghugo83 - Thanks! I did not find anywhere a reference to the SpecPar name: for example, PixelROCRowsPar_D1 Is it used somewhere? If not, perhaps the SpecPars could be merged eventually? ;-)

ghugo83 · 2020-10-01T16:20:16Z

@ghugo83 - Thanks! I did not find anywhere a reference to the SpecPar name: for example, PixelROCRowsPar_D1 Is it used somewhere? If not, perhaps the SpecPars could be merged eventually? ;-)

I remember seeing the use of SpecPar name somewhere in the DD4hep filtered view.
From what I had read, it was used to make a lookup up faster (it was used as a parameter to directly look at the specified SpecPar block).
This function does not affect the results though, and am not sure this function is even called.
But that's what I remember, not looking into the code, so can't be sure.

In general, in my opinion, only SpecPar path really matters (and parameter name/value obviously).

It does make some sense to have blocks per disk / ring. There actually are trackers where the sensor topology is different per ring (I also did some exotic Trackers, but in tkLayout only, where the topology was disk-specific).

Though, my guess is that you would like to have a warning when several blocks define the same parameter for the same path, right?
If so, the duplication in the XMLs could be removed.

cmsbuild · 2020-10-01T17:13:55Z

+1
Tested at: cc4ef4c
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b03384/9686/summary.html
CMSSW: CMSSW_11_2_X_2020-10-01-1100
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-10-01T17:13:58Z

Comparison job queued.

cmsbuild · 2020-10-01T18:42:41Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 35
DQMHistoTests: Total histograms compared: 2542225
DQMHistoTests: Total failures: 4
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2542199
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
Checked 149 log files, 22 edm output root files, 35 DQM output files

ianna · 2020-10-02T11:59:04Z

+1

cmsbuild · 2020-10-02T11:59:26Z

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)

silviodonato · 2020-10-02T12:46:52Z

+1

ianna and others added 7 commits September 30, 2020 16:37

make it 10x faster on GeometricDet

db7a0e5

code format

4bffb5a

adress comments

df19623

Merge branch 'master' into dd4hep-filtered-view-performance-v1

2e5ecb5

code format

4b2ea25

allow chaching a SpecPar for a subsequent query

e8999b2

code format

cc4ef4c

cmsbuild added this to the CMSSW_11_2_X milestone Oct 1, 2020

cmsbuild added code-checks-pending comparison-pending geometry-pending orp-pending pending-signatures tests-pending labels Oct 1, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 1, 2020

cmsbuild added tests-started and removed tests-pending labels Oct 1, 2020

cmsbuild added tests-approved and removed tests-started labels Oct 1, 2020

cmsbuild added comparison-available and removed comparison-pending labels Oct 1, 2020

ianna mentioned this pull request Oct 2, 2020

[TEST] Merge Parameters into the same SpecPar #31648

Closed

cmsbuild added fully-signed geometry-approved and removed geometry-pending pending-signatures labels Oct 2, 2020

cmsbuild added orp-approved and removed orp-pending labels Oct 2, 2020

cmsbuild merged commit 7e92b80 into cms-sw:master Oct 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DD4hep] Filtered View Performance - Step 2 #31641

[DD4hep] Filtered View Performance - Step 2 #31641

ianna commented Oct 1, 2020

cmsbuild commented Oct 1, 2020

ianna commented Oct 1, 2020

cmsbuild commented Oct 1, 2020

cmsbuild commented Oct 1, 2020 •

edited

Loading

cmsbuild commented Oct 1, 2020

ghugo83 commented Oct 1, 2020 •

edited

Loading

ianna commented Oct 1, 2020

ghugo83 commented Oct 1, 2020 •

edited

Loading

cmsbuild commented Oct 1, 2020

cmsbuild commented Oct 1, 2020

cmsbuild commented Oct 1, 2020

ianna commented Oct 2, 2020

cmsbuild commented Oct 2, 2020

silviodonato commented Oct 2, 2020

[DD4hep] Filtered View Performance - Step 2 #31641

[DD4hep] Filtered View Performance - Step 2 #31641

Conversation

ianna commented Oct 1, 2020

PR description:

PR validation:

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

cmsbuild commented Oct 1, 2020

ianna commented Oct 1, 2020

cmsbuild commented Oct 1, 2020

cmsbuild commented Oct 1, 2020 • edited Loading

cmsbuild commented Oct 1, 2020

ghugo83 commented Oct 1, 2020 • edited Loading

ianna commented Oct 1, 2020

ghugo83 commented Oct 1, 2020 • edited Loading

cmsbuild commented Oct 1, 2020

cmsbuild commented Oct 1, 2020

cmsbuild commented Oct 1, 2020

ianna commented Oct 2, 2020

cmsbuild commented Oct 2, 2020

silviodonato commented Oct 2, 2020

cmsbuild commented Oct 1, 2020 •

edited

Loading

ghugo83 commented Oct 1, 2020 •

edited

Loading

ghugo83 commented Oct 1, 2020 •

edited

Loading