Use of edm::RandomNumberGenerator in HGCDigiProducer

[makortel]

HGCDigiProducer (which is/can be a member variable of MixingModule, which itself is a stream::EDProducer) has a local "stream cache" for CLHEP::HepRandomEngines.

cmssw/SimCalorimetry/HGCalSimProducers/plugins/HGCDigiProducer.cc

Lines 47 to 50 in 301a35e

	void HGCDigiProducer::accumulate(edm::Event const& event, edm::EventSetup const& es)
	{
	theDigitizer_->accumulate(event, es, randomEngine(event.streamID()));
	}

cmssw/SimCalorimetry/HGCalSimProducers/plugins/HGCDigiProducer.cc

Lines 69 to 81 in 301a35e

	CLHEP::HepRandomEngine* HGCDigiProducer::randomEngine(edm::StreamID const& streamID) {
	unsigned int index = streamID.value();
	if(index >= randomEngines_.size()) {
	randomEngines_.resize(index + 1, nullptr);
	}
	CLHEP::HepRandomEngine* ptr = randomEngines_[index];
	if(!ptr) {
	edm::Service<edm::RandomNumberGenerator> rng;
	ptr = &rng->getEngine(streamID);
	randomEngines_[index] = ptr;
	}
	return ptr;
	}

A private e-mail thread with @Dr15Jones and @wddgit concludes that this construct should be safe, even if it is a bit weird.

The only benefit of this construct we could think of is being able to obtain the CLHEP::HepRandomEngine on O(1) time instead of O(log n) time.

I nevertheless decided to create an issue on this to understand why the cache is needed, and if we are happy with it or not.

[cmsbuild]

A new Issue was created by @makortel Matti Kortelainen.

@davidlange6, @Dr15Jones, @smuzaffar, @fabiocos can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

[makortel]

@lgray It seems that this cache was created in your commit 9a538de of #11880. Do you have a recollection why the cache was needed?

[wddgit]

There are actually two separate questions.

1. Why do we need a vector of pointers instead of just one pointer in HGCProducer if there is already an HGCDigiProducer per stream? This was confusing to me and unless I am missing something unnecessary, although I know very little about the MixingModule.

2. Is it worth the effort to cache the pointer? This actually does improve performance slightly so maybe this is OK. It might also be a negligible improvement.

[makortel]

@wddgit I spotted the same pattern in many other classes within the MixingModule plugins, e.g.

cmssw/SimCalorimetry/CastorSim/plugins/CastorDigiProducer.cc

Lines 166 to 178 in 285a1d8

	CLHEP::HepRandomEngine* CastorDigiProducer::randomEngine(edm::StreamID const& streamID) {
	unsigned int index = streamID.value();
	if(index >= randomEngines_.size()) {
	randomEngines_.resize(index + 1, nullptr);
	}
	CLHEP::HepRandomEngine* ptr = randomEngines_[index];
	if(!ptr) {
	edm::Service<edm::RandomNumberGenerator> rng;
	ptr = &rng->getEngine(streamID);
	randomEngines_[index] = ptr;
	}
	return ptr;
	}

cmssw/SimCalorimetry/EcalSimProducers/src/EcalDigiProducer.cc

Lines 631 to 643 in 285a1d8

	CLHEP::HepRandomEngine* EcalDigiProducer::randomEngine(edm::StreamID const& streamID) {
	unsigned int index = streamID.value();
	if(index >= randomEngines_.size()) {
	randomEngines_.resize(index + 1, nullptr);
	}
	CLHEP::HepRandomEngine* ptr = randomEngines_[index];
	if(!ptr) {
	edm::Service<edm::RandomNumberGenerator> rng;
	ptr = &rng->getEngine(streamID);
	randomEngines_[index] = ptr;
	}
	return ptr;
	}

cmssw/SimCalorimetry/HcalSimProducers/src/HcalDigiProducer.cc

Lines 88 to 100 in 285a1d8

	CLHEP::HepRandomEngine* HcalDigiProducer::randomEngine(edm::StreamID const& streamID) {
	unsigned int index = streamID.value();
	if(index >= randomEngines_.size()) {
	randomEngines_.resize(index + 1, nullptr);
	}
	CLHEP::HepRandomEngine* ptr = randomEngines_[index];
	if(!ptr) {
	edm::Service<edm::RandomNumberGenerator> rng;
	ptr = &rng->getEngine(streamID);
	randomEngines_[index] = ptr;
	}
	return ptr;
	}

cmssw/SimCalorimetry/HcalTestBeam/src/HcalTBDigiProducer.cc

Lines 246 to 258 in 285a1d8

	CLHEP::HepRandomEngine* HcalTBDigiProducer::randomEngine(edm::StreamID const& streamID) {
	unsigned int index = streamID.value();
	if(index >= randomEngines_.size()) {
	randomEngines_.resize(index + 1, nullptr);
	}
	CLHEP::HepRandomEngine* ptr = randomEngines_[index];
	if(!ptr) {
	edm::Service<edm::RandomNumberGenerator> rng;
	ptr = &rng->getEngine(streamID);
	randomEngines_[index] = ptr;
	}
	return ptr;
	}

cmssw/SimTracker/SiPixelDigitizer/plugins/SiPixelDigitizer.cc

Lines 303 to 315 in 285a1d8

	CLHEP::HepRandomEngine* SiPixelDigitizer::randomEngine(edm::StreamID const& streamID) {
	unsigned int index = streamID.value();
	if(index >= randomEngines_.size()) {
	randomEngines_.resize(index + 1, nullptr);
	}
	CLHEP::HepRandomEngine* ptr = randomEngines_[index];
	if(!ptr) {
	edm::Service<edm::RandomNumberGenerator> rng;
	ptr = &rng->getEngine(streamID);
	randomEngines_[index] = ptr;
	}
	return ptr;
	}

cmssw/SimTracker/SiStripDigitizer/plugins/SiStripDigitizer.cc

Lines 290 to 302 in 285a1d8

	CLHEP::HepRandomEngine* SiStripDigitizer::randomEngine(edm::StreamID const& streamID) {
	unsigned int index = streamID.value();
	if(index >= randomEngines_.size()) {
	randomEngines_.resize(index + 1, nullptr);
	}
	CLHEP::HepRandomEngine* ptr = randomEngines_[index];
	if(!ptr) {
	edm::Service<edm::RandomNumberGenerator> rng;
	ptr = &rng->getEngine(streamID);
	randomEngines_[index] = ptr;
	}
	return ptr;
	}

cmssw/Validation/EcalDigis/src/EcalMixingModuleValidation.cc

Lines 811 to 823 in 285a1d8

	CLHEP::HepRandomEngine* EcalMixingModuleValidation::randomEngine(edm::StreamID const& streamID) {
	unsigned int index = streamID.value();
	if(index >= randomEngines_.size()) {
	randomEngines_.resize(index + 1, nullptr);
	}
	CLHEP::HepRandomEngine* ptr = randomEngines_[index];
	if(!ptr) {
	edm::Service<edm::RandomNumberGenerator> rng;
	ptr = &rng->getEngine(streamID);
	randomEngines_[index] = ptr;
	}
	return ptr;
	}

so maybe the pattern in HGCDigiProducer is just copy-paste from elsewhere?

Then there is

cmssw/Mixing/Base/src/PileUp.cc

Lines 343 to 349 in 285a1d8

	CLHEP::HepRandomEngine* PileUp::randomEngine(StreamID const& streamID) {
	if(!randomEngine_) {
	Service<RandomNumberGenerator> rng;
	randomEngine_ = &rng->getEngine(streamID);
	}
	return randomEngine_;
	}

which caches the random engine to a member variable. All these were added in commit
6696e5d of #2392. If we now think the pattern could be improved, maybe we should just do it everywhere?

@Dr15Jones, could you remind me if an instance of a stream module is always run in the same stream (i.e. is the pattern of PileUp::randomEngine() safe)?

[Dr15Jones]

@Dr15Jones, could you remind me if an instance of a stream module is always run in the same stream (i.e. is the pattern of PileUp::randomEngine() safe)?

An instance of a stream module is always run in the same stream.

[wddgit]

Looks like I created this pattern back in 2014. I did not remember
that.

After staring at CastorDigiProducer more, I cannot understand or remember
why I used a vector to cache the pointers in. If I understand this correctly,
the MixingModule is a stream module and each stream instance has its
own CastorDigiProducer so there is no reason for a vector with an entry
per stream. (And if there had been a reason for the vector, then the call
to the resize function would have been a date race. Sigh.) On the positive
side, it should work correctly and its performance should be OK.

Removing the vector in all these cases would be a good thing. It is confusing
to someone reading, maintaining or copying the code. The PileUp
class does not have this problem.

There is the separate issue of whether we should cache or not.
Probably back in 2014, I was trying to make it perform as fast
as possible and caching is probably faster. But I agree the
difference is likely negligible. I do not feel strongly about this either
way.

[wddgit]

Also note EcalMixingModuleValidation is a one type module. For that
type of module the vector is needed and correct.

[wddgit]

I found the reason it was coded that way. Back in 2014 the
MixingModule was a one module. In 2015 it was changed to
be a stream type module. Now I feel better about it ...

[makortel]

@wddgit Thanks for reminding of the history. Should we then migrate everything else than EcalMixingModuleValidation to the same caching pattern as in PileUp?

[wddgit]

That sounds like the best course to me. I see that Dan Riley did that for the PileUp class back in 2015.

I don't feel strongly about this though. If you or Chris advocate removing the caching entirely I would be happy with that also. It might be worth testing that we do not slow it down significantly if we remove the caching.

Matti, were you going to make this change? @Dr15Jones If you want me to do it, let me know and what its priority is compared to what I am currently working on.

[makortel]

@wddgit I talked briefly with @Dr15Jones and we felt as well to keep the caching (without strong feelings). I can take care of it at some point in the near future.

[makortel]

The cleanup is in #22874.