Skip to content

Conversation

@Dr15Jones
Copy link
Contributor

@Dr15Jones Dr15Jones commented Nov 19, 2025

PR description:

Memory increases when moving to ROOT 6.36 pointed to these two classes not having dictionaries explicitly defined.

PR validation:

Using the MaxMemoryAllocMonitor shows that running workflow 16834.0 step 2 with this change brought down overall memory by more than 500MB, to approximately the ROOT 6.32 level.

resolves cms-sw/framework-team#1692

Memory increases when moving to ROOT 6.36 pointed to these
 two classes not having dictionaries explicitly defined.
@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 19, 2025

cms-bot internal usage

@cmsbuild
Copy link
Contributor

@Dr15Jones
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • DataFormats/TrackingRecHitSoA (heterogeneous, reconstruction)
  • DataFormats/VertexSoA (heterogeneous, reconstruction)

@cmsbuild, @fwyzard, @jfernan2, @makortel, @mandrenguyen, @srimanob can you please review it and eventually sign? Thanks.
@VinInn, @VourMa, @elusian, @gpetruc, @missirol, @mmasciov, @mmusich, @mtosi, @rovere this is something you requested to watch as well.
@ftenchini, @mandrenguyen, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

@cmsbuild
Copy link
Contributor

+1

Size: This PR adds an extra 24KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-810e9f/49567/summary.html
COMMIT: bddd18f
CMSSW: CMSSW_16_0_X_2025-11-19-1100/el8_amd64_gcc13
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/49422/49567/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 7 lines from the logs
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 7 differences found in the comparisons
  • Reco comparison had 2 failed jobs
  • DQMHistoTests: Total files compared: 53
  • DQMHistoTests: Total histograms compared: 4128813
  • DQMHistoTests: Total failures: 14
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 4128779
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 52 files compared)
  • Checked 226 log files, 198 edm output root files, 53 DQM output files
  • TriggerResults: no differences found

@smuzaffar
Copy link
Contributor

smuzaffar commented Nov 19, 2025

Invalid: please test for CMSSW_16_0_X_2025-11-14-2300

Just to compare the memory usage with the release where we had root 6.32

@makortel
Copy link
Contributor

(from #49422 (comment))
2025.0000001_RunZeroBias2025B_10k shows

image

(2025.0010001_RunJetMET02025C_10k is similar)


16834.0_TTbar_14TeV+2025 shows

image

(17034.0_TTbar_14TeV+2025PU is similar)

@smuzaffar
Copy link
Contributor

smuzaffar commented Nov 19, 2025

abort

test this in CMSSW_16_0_X_2025-11-14-2300 will not give any useful results as in this case both will be comparing root 6.32 vs 6.32 plus this change in CMSSW_16_0_X_2025-11-14-2300

@smuzaffar
Copy link
Contributor

please test

sorry for starting the tests against an old IB (which was not needed). Rerunning the default tests to get the commits statuses properly updated

@cmsbuild
Copy link
Contributor

+1

Size: This PR adds an extra 24KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-810e9f/49570/summary.html
COMMIT: bddd18f
CMSSW: CMSSW_16_0_X_2025-11-19-1100/el8_amd64_gcc13
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/49422/49570/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 4 lines from the logs
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 5 differences found in the comparisons
  • Reco comparison had 2 failed jobs
  • DQMHistoTests: Total files compared: 53
  • DQMHistoTests: Total histograms compared: 4128813
  • DQMHistoTests: Total failures: 3
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 4128790
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 52 files compared)
  • Checked 226 log files, 198 edm output root files, 53 DQM output files
  • TriggerResults: no differences found

@fwyzard
Copy link
Contributor

fwyzard commented Nov 20, 2025

+heterogeneous

@fwyzard
Copy link
Contributor

fwyzard commented Nov 20, 2025

@Dr15Jones so the take away message is that one should always declare the dictionary for the Implementation of MultiCollection ?

Normally I would suggest adding this to the README.md file, but now that #48629 is merged, hopefully we can quickly replace the MultiCollection approach with the SoA blocks approach.

@jfernan2
Copy link
Contributor

+1

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

@mandrenguyen
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit d9e97df into cms-sw:master Nov 20, 2025
10 checks passed
@Dr15Jones
Copy link
Contributor Author

So the take away message is that one should always declare the dictionary for the Implementation of MultiCollection?

The best answer we can give is 'probably' as we really do not understand the actual ROOT requirements and we are just going on 'what seems to work'.

@makortel
Copy link
Contributor

The need for the X::Implementation request in the classes_def.xml to avoid header parsing was understood to be caused by the read rule referring to the X::Implementation (with X being the type alias passed to SET_PORTABLEHOSTMULTICOLLECTION_READ_RULES() macro). See #49399 (comment). Because of root-project/root#19705 its request must be in the classes_def.xml before the type that it aliases.

Why the omission of X::Implementation did not seem to trigger header parsing in ROOT 6.32 is not clear.

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.

Create PR to workaround the issue

7 participants