Try converting RecoParticleFlow/PFTracking to GBRForest + GlobalCache #10143
Conversation
|
A new Pull Request was created by @lgray (Lindsey Gray) for CMSSW_7_6_X. Try converting RecoParticleFlow/PFTracking to GBRForest + GlobalCache It involves the following packages: RecoParticleFlow/PFTracking @cmsbuild, @cvuosalo, @slava77 can you please review it and eventually sign? Thanks. |
| std::unique_ptr<GBRForest> gbrEndcapsLowPt_; | ||
| std::unique_ptr<GBRForest> gbrEndcapsHighPt_; | ||
| std::unique_ptr<PFEnergyCalibration> pfcalib_; | ||
| private: |
There was a problem hiding this comment.
I would feel a lot more confident about the thread safety if these were all std::unique_ptr<const ...>.
There was a problem hiding this comment.
I've decided this should be a stronger statement and these need to be std::unique_ptr<const ...>.
|
@Dr15Jones I have addressed your issues! Please have another look. |
|
|
||
| static std::unique_ptr<convbremhelpers::HeavyObjectCache> | ||
| initializeGlobalCache( const edm::ParameterSet& conf ) { | ||
| return std::unique_ptr<convbremhelpers::HeavyObjectCache>(new convbremhelpers::HeavyObjectCache(conf)); |
There was a problem hiding this comment.
There is a nice way to actually make this shorter (for your future reference)
return std::make_unique<convbremhelpers::HeavyObjectCache>(conf);
it is a C++14 thing.
| @@ -147,6 +171,7 @@ class GoodSeedProducer final : public edm::stream::EDProducer<> { | |||
|
|
|||
| ///READER FOR TMVA | |||
| std::array<std::unique_ptr<TMVA::Reader>,9> reader{}; | |||
| std::array<std::unique_ptr<GBRForest>,9> gbr; | |||
There was a problem hiding this comment.
Do you want these 9s to be goodseedhelpers::HeavyObjectCache::kMaxWeights or is it just a coincidence?
There was a problem hiding this comment.
Deprecated code needs to be removed.
Since this is a test PR, I'll clean it up later.
-L
(Sent from my Nexus 6)
On Jul 11, 2015 3:02 PM, "Chris Jones" notifications@github.com wrote:
In RecoParticleFlow/PFTracking/plugins/GoodSeedProducer.h
#10143 (comment):@@ -147,6 +171,7 @@ class GoodSeedProducer final : public edm::stream::EDProducer<> {
///READER FOR TMVA std::array<std::unique_ptr<TMVA::Reader>,9> reader{};
std::arraystd::unique_ptr<GBRForest,9> gbr;Do you want these 9s to be goodseedhelpers::HeavyObjectCache::kMaxWeights
or is it just a coincidence?—
Reply to this email directly or view it on GitHub
https://github.com/cms-sw/cmssw/pull/10143/files#r34412459.
|
As long as |
|
Could you issue a test in Jenkins? (Sent from my Nexus 6)
|
|
please test |
|
The tests are being triggered in jenkins. |
|
+1 Putting GBRForest objects into edm::GlobalCaches to reduce memory usage for multi-threaded jobs. There should be no significant changes in monitored quantities. The code changes are satisfactory, and Jenkins tests against baseline CMSSW_7_6_X_2015-07-22-2300 show no significant differences, as expected. Extended tests with 70 or 100 events of workflow 25202.0_TTbar_13 against baseline CMSSW_7_6_X_2015-07-21-1100 also show no significant differences other than minor jitter that is expected. Comparisons were done for single-threaded RECO, multi-threaded RECO (4 threads), and comparing the single- vs. multi-threaded PR. |
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_7_6_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @Degano, @smuzaffar |
|
+1 |
Try converting RecoParticleFlow/PFTracking to GBRForest + GlobalCache
Since things look OK on the surface from the DQM plots in #10142, I went ahead and moved all the GBRForest objects into edm::GlobalCaches since the BDTs are always read from the same files and don't depend on run. This should bring some memory usage improvements in multicore jobs.
Trial of converting a package that used TMVA::Reader to GBRForest + use edm::GlobalCache.
Underlying regression weights were gradient boosted, despite method name (read from xml).
At most, some jitter may be expected, otherwise expect no physics impact.
Also expect some reduction in RSS.
@bendavid
@Dr15Jones You might be interested in this too if you want to look for thread safety issues. FYI GBRForest is threadsafe.