-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want these 9
s to be goodseedhelpers::HeavyObjectCache::kMaxWeights
or is it just a coincidence?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.