[RF] Speedup histfactory ParamHistFunc by guitargeek · Pull Request #7838 · root-project/root

guitargeek · 2021-04-12T15:54:57Z

The ParamHistFunc in the histfactory has a persistent std::map<int,int>
data member. It can be replaced with a std::vector<int> where the
previous key is the index in the vector. This is much faster.

As the igprof reports below show, it is even faster to calculate the bin indices on the fly, which is proposed in this PR.

Schema evolution is not a problem here. With this commit, the data
member is made non-persistent because it can be computed from other
persistent data members.

This PR was benchmarked with an example from the ROOT forum:

igprof report before this PR (caching with std::map)
igprof report after the first commit of the PR (caching with std::vector)
igprof report after the PR (calculating on the fly)

The difference is about a 50 % speedup of ParamHistFunc::evaluate() and a 10 % speedup of the full example.

phsft-bot · 2021-04-12T15:55:04Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2021-04-12T16:28:09Z

Build failed on ROOT-fedora31/noimt.
Running on root-fedora-31-2.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

phsft-bot · 2021-04-12T16:35:39Z

Build failed on ROOT-ubuntu16/nortcxxmod.
Running on sft-ubuntu-1604-2.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

hageboeck · 2021-04-12T17:05:06Z

Nice find. This might conflict with my HistFactory branch, but go ahead and I can rebase once it's in.

hageboeck · 2021-04-12T17:11:17Z

roofit/histfactory/src/ParamHistFunc.cxx

  if( numVars == 1 ) {
-    RooRealVar* varX = (RooRealVar*) _dataVars.at(0);
-    numBinsX = varX->numBins();
-    numBinsY = 1;
-    numBinsZ = 1;
+    numBinsX = static_cast<RooRealVar const&>(_dataVars[0]).numBins();
  } else  if( numVars == 2 ) {
-    RooRealVar* varX = (RooRealVar*) _dataVars.at(0);
-    RooRealVar* varY = (RooRealVar*) _dataVars.at(1);
-    numBinsX = varX->numBins();
-    numBinsY = varY->numBins();
-    numBinsZ = 1;
+    numBinsX = static_cast<RooRealVar const&>(_dataVars[0]).numBins();
+    numBinsY = static_cast<RooRealVar const&>(_dataVars[1]).numBins();
  } else  if( numVars == 3 ) {
-    RooRealVar* varX = (RooRealVar*) _dataVars.at(0);
-    RooRealVar* varY = (RooRealVar*) _dataVars.at(1);
-    RooRealVar* varZ = (RooRealVar*) _dataVars.at(2);
-    numBinsX = varX->numBins();
-    numBinsY = varY->numBins();
-    numBinsZ = varZ->numBins();
+    numBinsX = static_cast<RooRealVar const&>(_dataVars[0]).numBins();
+    numBinsY = static_cast<RooRealVar const&>(_dataVars[1]).numBins();
+    numBinsZ = static_cast<RooRealVar const&>(_dataVars[2]).numBins();


Why not replace those three branches with code repetition by:

if (numVars >= 1) numBinsX = static_cast<RooRealVar const&>(_dataVars[0]).numBins(); if (numVars >= 2) numBinsY = static_cast<RooRealVar const&>(_dataVars[1]).numBins(); ...

And I would assume that numVars > 0. You could check.

Or if you want to go more RAII:

const int numBinsX = numVars >= 1 ? static_cast<RooRealVar const&>(_dataVars[0]).numBins() : 1; ...

hageboeck · 2021-04-12T17:12:59Z

roofit/histfactory/src/ParamHistFunc.cxx

+      coutE(InputArguments) << "ParamHistFunc::(" << GetName() << ") ERROR: component " 
+                            << comp->GetName() << " in variables list is not of type RooRealVar" 
+                            << std::endl;
+      RooErrorHandler::softAbort() ;


Please remove that. The idea was that you would be dropped back to the terminal, but it doesn't work in interpreter mode.

Better throw an exception, because that will actually drop you back to the terminal.

When used from a compiled program, it just ends the program with an uncaught exception.

Context: This used to work in ROOT 5, but those times are gone.

phsft-bot · 2021-04-12T17:45:29Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

hageboeck · 2021-04-12T18:23:11Z

roofit/histfactory/src/ParamHistFunc.cxx

-      return -1;
+      auto errorMsg = std::string("ParamHistFunc::GetNumBins") + vars.GetName() + ") ERROR: component "
+                      + comp->GetName() + " in vars list is not of type RooRealVar";
+      std::cout << errorMsg << std::endl;


coutE as well?

It unfortunately doesn't work in a static function. Do you have an alternative suggestion? There is oocoutE, but I'm not sure if it makes sense because you have to give it a caller.

A static_cast<TObject*>(nullptr) works fine. The caller is only necessary to print its name. If it's nullptr, that name printing is skipped.

phsft-bot · 2021-04-13T08:59:48Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

hageboeck · 2021-04-13T16:44:33Z

roofit/histfactory/src/ParamHistFunc.cxx

  RooFIter varIter = vars.fwdIterator() ;
  RooAbsArg* comp ;
  while((comp = (RooAbsArg*) varIter.next())) {


If you are at it anyway:

for (auto arg : vars) { ...

Those loops are ~20% faster (which doesn't make a difference here), but well. 🙂

The ParamHistFunc in the histfactory has a persistent std::map<int,int> data member. It can be replaced with a std::vector<int> where the previous key is the index in the vector. This is much faster. Schema evolution is not a problem here. With this commit, the data member is made non-persistent because it can be computed from other persistent data members.

Benchmarking showed that calculating the bin indices in ParamHistFunc on the fly is faster than caching them in a std::vector.

phsft-bot · 2021-04-13T17:31:19Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

hageboeck

LGTM!

guitargeek requested review from hageboeck and lmoneta as code owners April 12, 2021 15:54

guitargeek added the in:RooFit label Apr 12, 2021

hageboeck reviewed Apr 12, 2021

View reviewed changes

guitargeek force-pushed the hf_ParamHistFunc_1 branch from 75e44bf to 3bba3f3 Compare April 12, 2021 17:45

hageboeck reviewed Apr 12, 2021

View reviewed changes

guitargeek force-pushed the hf_ParamHistFunc_1 branch from 3bba3f3 to 460cc0b Compare April 13, 2021 08:59

eguiraud assigned guitargeek Apr 13, 2021

hageboeck reviewed Apr 13, 2021

View reviewed changes

guitargeek force-pushed the hf_ParamHistFunc_1 branch from 460cc0b to 418288c Compare April 13, 2021 17:31

guitargeek added 2 commits April 13, 2021 19:31

[RF] Replace softAbort with std Exceptions in ParamHistFunc

34c2a3c

[RF] Calculate bin indices on the fly in ParamHistFunc

418288c

Benchmarking showed that calculating the bin indices in ParamHistFunc on the fly is faster than caching them in a std::vector.

guitargeek mentioned this pull request Apr 13, 2021

[RF] Speed up getting weights from RooDataHist #7859

Merged

hageboeck approved these changes Apr 14, 2021

View reviewed changes

guitargeek merged commit 3657e7c into root-project:master Apr 14, 2021

guitargeek deleted the hf_ParamHistFunc_1 branch April 14, 2021 09:24

Conversation

guitargeek commented Apr 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phsft-bot commented Apr 12, 2021

Uh oh!

phsft-bot commented Apr 12, 2021

Failing tests:

Uh oh!

phsft-bot commented Apr 12, 2021

Failing tests:

Uh oh!

hageboeck commented Apr 12, 2021

Uh oh!

hageboeck Apr 12, 2021

Choose a reason for hiding this comment

Uh oh!

hageboeck Apr 12, 2021

Choose a reason for hiding this comment

Uh oh!

hageboeck Apr 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hageboeck Apr 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phsft-bot commented Apr 12, 2021

Uh oh!

hageboeck Apr 12, 2021

Choose a reason for hiding this comment

Uh oh!

guitargeek Apr 13, 2021

Choose a reason for hiding this comment

Uh oh!

hageboeck Apr 13, 2021

Choose a reason for hiding this comment

Uh oh!

phsft-bot commented Apr 13, 2021

Uh oh!

hageboeck Apr 13, 2021

Choose a reason for hiding this comment

Uh oh!

phsft-bot commented Apr 13, 2021

Uh oh!

hageboeck left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

guitargeek commented Apr 12, 2021 •

edited

Loading

hageboeck Apr 12, 2021 •

edited

Loading

hageboeck Apr 12, 2021 •

edited

Loading