CMSEmStandardPhysicsEMMT, don't hold ref into main ParameterSet

[wddgit]

PR description:

In an effort to reduce memory usage we are attempting to remove references into the main ParameterSet from modules. When that effort is complete, the intent is to submit a separate PR deleting that ParameterSet when all module construction is complete.

This PR addresses the last module in the Sim* subsystems with such a reference.

This copies the required parameters into a struct and holds that until it is needed instead of holding a reference to the entire ParameterSet.

PR validation:

The change is fairly trivial, but I'd like to run a manual test that the values are going through OK. The parameters are set in g4SimHits_cfi.py (only there). I've so far unsuccessfully been looking for an existing test that executes the function CMSEmStandardPhysicsEMMT::ConstructProcess(). This looks like code that might still be actively used. I am still looking. Could someone point me to a test that runs it, maybe a RelVal number?

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42535/36551

• This PR adds an extra 16KB to repository

[cmsbuild]

A new Pull Request was created by @wddgit (W. David Dagenhart) for master.

It involves the following packages:

• SimG4Core/PhysicsLists (simulation)

@cmsbuild, @civanch, @mdhildreth can you please review it and eventually sign? Thanks.
@makortel, @bsunanda, @rovere, @fabiocos, @slomeo this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

[wddgit]

please test

[cmsbuild]

-1

Failed Tests: HeaderConsistency
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-706a7c/34214/summary.html
COMMIT: 3e01c60
CMSSW: CMSSW_13_3_X_2023-08-10-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/42535/34214/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

• You potentially added 164 lines to the logs
• Reco comparison results: 42 differences found in the comparisons
• DQMHistoTests: Total files compared: 48
• DQMHistoTests: Total histograms compared: 3150947
• DQMHistoTests: Total failures: 275
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3150650
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
• Checked 207 log files, 159 edm output root files, 48 DQM output files
• TriggerResults: no differences found

[civanch]

@wddgit , what is proposed in this PR to me is not a good variant to resolve problems: we have ~20 PhysicsLists, which use few different values of parameter to configure CMS physics simulation. Creation of 20 different structs seems to be an overhead. All these parameters are used only at initialisation.

I would close this PR. We will discuss with @hahnjo how to proceed with PhysicsLists. We may use this opportunity to remove unused parameters and simplify class initialisation in SimG4Core and SimG4CMS.

I would expect that we should use ParameterSet without copy to local objects, we only fill local data. So, the ParameterSet may be removed after initialisation and do not save locally ParameterSet or SubParameterSet. Would this be enough?

[civanch]

@makortel , @Dr15Jones , I am not sure if the idea of removing ParameterSet will work for SIM. In my memory, at super computes a new thread may appear not immediately but with big delay, so RunManagerMTWorker will be created with big delay. How it would be possible to understand that ParameterSet are not needed anymore? The only solution would be to exclude ParameterSet from RunManagerMTWorker and all thread local classes downstream. Is it a good idea? The reason of having ParameterSet is in easy configuration of CMSSW, making limitation of removing it will require changing of practically all SIM classes and creation of different mechanism of defining parameters. Do we really need this? How much of memory will be saved?

[wddgit]

If I understand this correctly, CMSEmStandardPhysicsEMMT is one of the approximately 20 PhysicsLists (correct me if I am wrong, I'm not familiar with this code). CMSEmStandardPhysicsEMMT is different than all the other PhysicsLists in the way it handles its parameters. It is the only one that saves a reference into the main ParameterSet and then uses it when ConstructProcess() is called. So we are only talking about creating one structure, not ~20.

For example, one of the other PhysicsList, CMSEmStandardPhysicsEMH, copies the parameters directly into data members. It does not save a reference into the main ParameterSet.

https://cmssdt.cern.ch/dxr/CMSSW/source/SimG4Core/PhysicsLists/interface/CMSEmStandardPhysicsEMH.h#19

(Although in this particular case the values are never used at all, so I'm not sure why we even bother...)

One question, when is ConstructProcess called? I was trying, but didn't yet succeed in understanding that. Is it during module construction, beginJob, run transitions, lumi transitions, event transitions, or something else. The reference might not be an actual problem if it is only used early enough.

An alternative might be to copy the ParameterSet by value. There is other unrelated code doing that. Although it is a moderately large ParameterSet.

Related to your second comment, there is more discussion in issue #42503. There it says 40 MB, although this varies with different jobs.

I will look at RunManagerMTWorker. I am not familiar with that. Possibly @makortel will be able to comment on that. I agree that one difficulty with this approach is finding all these references (existing ones and worrying about new ones being added).

[makortel]

I am not sure if the idea of removing ParameterSet will work for SIM. In my memory, at super computes a new thread may appear not immediately but with big delay, so RunManagerMTWorker will be created with big delay.

After #34899 this is actually no longer the case, as the "worker threads" corresponding to each framework stream are created in the OscarMTProducer constructor (as are the RunManagerMTWorker objects)

cmssw/SimG4Core/Application/plugins/OscarMTProducer.cc

Lines 115 to 120 in ed6d0cc

	auto token = edm::ServiceRegistry::instance().presentToken();
	m_handoff.runAndWait([this, &p, token]() {
	edm::ServiceRegistry::Operate guard{token};
	StaticRandomEngineSetUnset random(nullptr);
	m_runManagerWorker = std::make_unique<RunManagerMTWorker>(p, consumesCollector());
	});

So in principle simulation should be able to parse the configuration at the module construction time?

But note that we are only prohibiting holding references to the ParameterSet object that is given as argument to the module constructors. Making copies from it (that is being done e.g. in RunManagerMTWorker) continues to be technically allowed (although discouraged in general).

Maybe the argument ParameterSet is already owned elsewhere within OscarMTProducer? If that is the case, this PR is indeed not needed.

Within the classes inheriting from G4VPhysicsConstructor I saw a few that hold ParameterSet by value (i.e. making a copy), while most of the parse the values into their member variables. Instead of creating a new struct, this PR could have stored the information also in member variables of CMSEmStandardPhysicsEMMT.

How much of memory will be saved?

Depends on the complexity of the job configuration. In the case of a Run 3 re-reco job (that was having memory problems) the process ParameterSet took about 40 MB.

[wddgit]

From @makortel:

"Maybe the argument ParameterSet is already owned elsewhere within OscarMTProducer? If that is the case, this PR is indeed not needed."

I'm looking to see if that is the case. If it is, I'll close the PR. That possibility didn't occur to me but I should have thought of that possibility.

[wddgit]

Closing, because this PR is not needed. Apologies for wasting time on this.

The ParameterSet being used here is a by value data member of RunManagerMT. It is not the one owned by the Framework. This is the only usage of this class currently in CMSSW.

See:

https://cmssdt.cern.ch/dxr/CMSSW/source/SimG4Core/Application/interface/RunManagerMT.h#108

  edm::ParameterSet m_pPhysics;

[civanch]

May be we should review OscarMTProducer, RunManagerMT, and RunManagerMTWorker coping and storing locally only branches of ParameterSet locally? Our branches are limited in size. Full ParameterSet should not be copied.

[wddgit]

m_pPhysics is not storing the entire top level process ParameterSet and also not the top level ParameterSet for the module either. I'm not sure if that is what you meant. It is only storing the nested ParameterSet named Physics. There are other similar ParameterSets stored in the same class that I didn't look at.

Maybe there can be some improvements. I didn't get deep enough into it to know the answer to that. m_pPhysics is only storing the following nested ParameterSet from g4SimHits_cfi.py. I don't know how much of it is needed or used. Down the one stack code I looked only 5 parameters were used, but there could be other places in the code that use much more of it.

    Physics = cms.PSet(
        common_maximum_time,
        # NOTE : if you want EM Physics only,
        #        please select "SimG4Core/Physics/DummyPhysics" for type
        #        and turn ON DummyEMPhysics
        #
        type = cms.string('SimG4Core/Physics/FTFP_BERT_EMM'),
        DummyEMPhysics = cms.bool(False),
        # 1 will print cuts as they get set from DD
        # 2 will do as 1 + will dump Geant4 table of cuts
        Verbosity = cms.untracked.int32(0),
        # EM physics options
        CutsPerRegion = cms.bool(True),
        CutsOnProton  = cms.bool(True),
        DefaultCutValue = cms.double(1.0), ## cuts in cm
        G4BremsstrahlungThreshold = cms.double(0.5), ## cut in GeV
        G4MuonBremsstrahlungThreshold = cms.double(10000.), ## cut in GeV
        G4TrackingCut = cms.double(0.025), ## cut in MeV
        G4MscRangeFactor = cms.double(0.04),
        G4MscGeomFactor = cms.double(2.5), 
        G4MscSafetyFactor = cms.double(0.6), 
        G4MscLambdaLimit = cms.double(1.0), # in mm 
        G4MscStepLimit = cms.string("UseSafety"),
        G4GammaGeneralProcess = cms.bool(True),
        G4ElectronGeneralProcess = cms.bool(False),
        G4TransportWithMSC = cms.int32(2),  # 1 - fEnabled, 2 - fMultipleSteps
        PhotoeffectBelowKShell = cms.bool(False),
        G4HepEmActive = cms.bool(False),
        G4MuonPairProductionByMuon = cms.bool(False),
        ReadMuonData = cms.bool(False), 
        Region      = cms.string(''),
        TrackingCut = cms.bool(False),
        SRType      = cms.bool(True),
        FlagMuNucl  = cms.bool(False),
        FlagFluo    = cms.bool(False),
        EMPhysics   = cms.untracked.bool(True),
        # Hadronic physics options
        HadPhysics  = cms.untracked.bool(True),
        FlagBERT    = cms.untracked.bool(False),
        EminFTFP    = cms.double(3.), # in GeV
        EmaxBERT    = cms.double(6.), # in GeV
        EminQGSP    = cms.double(12.), # in GeV
        EmaxFTFP    = cms.double(25.), # in GeV
        EmaxBERTpi  = cms.double(12.), # in GeV
        G4NeutronGeneralProcess = cms.bool(False),
        G4BCHadronicProcess = cms.bool(False),
        G4LightHyperNucleiTracking = cms.bool(False),
        ThermalNeutrons = cms.untracked.bool(False),
        # Exotica
        MonopoleCharge       = cms.untracked.int32(1),
        MonopoleDeltaRay     = cms.untracked.bool(True),
        MonopoleMultiScatter = cms.untracked.bool(False),
        MonopoleTransport    = cms.untracked.bool(True),
        MonopoleMass         = cms.untracked.double(0),
        ExoticaTransport     = cms.untracked.bool(False),
        ExoticaPhysicsSS     = cms.untracked.bool(False),
        RhadronPhysics       = cms.bool(False),
        DarkMPFactor         = cms.double(1.0),
        # GFlash methods
        LowEnergyGflashEcal = cms.bool(False),
        LowEnergyGflashEcalEmax = cms.double(0.02), # in GeV
        GflashEcal    = cms.bool(False),
        GflashHcal    = cms.bool(False),
        GflashEcalHad = cms.bool(False),
        GflashHcalHad = cms.bool(False),
        bField        = cms.double(3.8),
        energyScaleEB = cms.double(1.032),
        energyScaleEE = cms.double(1.024),
        # Russian roulette
        RusRoElectronEnergyLimit  = cms.double(0.0),
        RusRoEcalElectron         = cms.double(1.0),
        RusRoHcalElectron         = cms.double(1.0),
        RusRoMuonIronElectron     = cms.double(1.0),
        RusRoPreShowerElectron    = cms.double(1.0),
        RusRoCastorElectron       = cms.double(1.0),
        RusRoWorldElectron        = cms.double(1.0),
        # Tracking and step limiters
        ElectronStepLimit         = cms.bool(False),
        ElectronRangeTest         = cms.bool(False),
        PositronStepLimit         = cms.bool(False),
        ProtonRegionLimit         = cms.bool(False),
        PionRegionLimit           = cms.bool(False),
        LimitsPerRegion = cms.vstring('EcalRegion','HcalRegion'),
        EnergyLimitsE   = cms.vdouble(0.,0.0),
        EnergyLimitsH   = cms.vdouble(0.,0.0),
        EnergyFactorsE  = cms.vdouble(1.,0.0),
        EnergyRMSE      = cms.vdouble(0.0,0.0),
        MinStepLimit              = cms.double(1.0),
        ModifyTransportation      = cms.bool(False),
        ThresholdWarningEnergy    = cms.untracked.double(100.0), #in MeV
        ThresholdImportantEnergy  = cms.untracked.double(250.0), #in MeV
        ThresholdTrials           = cms.untracked.int32(10)
    ),

[civanch]

@wddgit , there is "m_p" inside RunMangerMT and/or inside RunManagerMTWorker, which may be a problem. I will try to substitute it by branches or by few variables. Also may be a part of initialisation may be moved to the constructor.