Fixing an O(N^2) scaling problem caused by TTree::Draw() #650

sawenzel · 2017-06-14T10:27:56Z

This is a first attempt at solving a scaling problem observed during TTree::Draw of a branch that contains a TClonesArray where each element contains another small vector container.

The fix works for me.

phsft-bot · 2017-06-14T10:27:58Z

Can one of the admins verify this patch?

sawenzel · 2017-06-14T10:28:36Z

@pcanal : This is roughly what we discussed. Can you please take a look?

pcanal · 2017-06-14T10:32:21Z

build please

pcanal · 2017-06-14T10:40:52Z

@phsft-bot build, please

pcanal · 2017-06-14T10:41:14Z

tree/treeplayer/src/TTreeFormula.cxx

+	      // caches to keep track of where we are across function invocations
+	      // will avoid an O(N^2) scaling
+	      // reset them if instance=0 is seen
+              static Int_t local_index_cache = 0;


This need to be thread_local :)

Yes of course. Let us focus on the correctness for one thread for the moment. The question is then if we want to use static stuff for the cache or a member variable of the TTreeFormula. The latter would not have a penalty to access (probably).

actually I am very wrong :( ...

Those actually need to be data member. More than once TTreeFormula can be handled/processed during the same operation and they can also be nested. For example

tree->Draw("px:pz:fTrack[index]");

the call to EvalInstance for each i will be interleaved and the one with the index will be nested.

phsft-bot · 2017-06-14T10:41:16Z

Starting build on gcc49/centos7, native/mac1012, gcc49/slc6, gcc62/slc6, native/ubuntu14 with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

phsft-bot · 2017-06-14T11:11:08Z

Build failed on ubuntu14/native.
See console output.

Warnings:

JENKINS-19022: warning: possible memory leak due to Git plugin usage; see: https://wiki.jenkins-ci.org/display/JENKINS/Remove+Git+Plugin+BuildsByBranch+BuildData
/mnt/vdb/lsf/workspace/root-pullrequests-build/root/hist/hist/src/TF1.cxx:2693:173: warning: format ‘%d’ expects argument of type ‘int’, but argument 5 has type ‘std::vector<double>::size_type {aka long unsigned int}’ [-Wformat=]
/mnt/vdb/lsf/workspace/root-pullrequests-build/root/tree/treeplayer/src/TTreeFormula.cxx:3424:14: warning: unused variable ‘counter’ [-Wunused-variable]

Failing tests:

phsft-bot · 2017-06-14T11:21:06Z

Build failed on slc6/gcc49.
See console output.

Warnings:

JENKINS-19022: warning: possible memory leak due to Git plugin usage; see: https://wiki.jenkins-ci.org/display/JENKINS/Remove+Git+Plugin+BuildsByBranch+BuildData
/mnt/build/jenkins/workspace/root-pullrequests-build/root/hist/hist/src/TF1.cxx:2693:173: warning: format ‘%d’ expects argument of type ‘int’, but argument 5 has type ‘std::vector<double>::size_type {aka long unsigned int}’ [-Wformat=]
/mnt/build/jenkins/workspace/root-pullrequests-build/root/tree/treeplayer/src/TTreeFormula.cxx:3424:14: warning: unused variable ‘counter’ [-Wunused-variable]

Failing tests:

phsft-bot · 2017-06-14T11:34:12Z

Build failed on slc6/gcc62.
See console output.

Warnings:

JENKINS-19022: warning: possible memory leak due to Git plugin usage; see: https://wiki.jenkins-ci.org/display/JENKINS/Remove+Git+Plugin+BuildsByBranch+BuildData
/mnt/build/jenkins/workspace/root-pullrequests-build/root/hist/hist/src/TF1.cxx:2693:173: warning: format ‘%d’ expects argument of type ‘int’, but argument 5 has type ‘std::vector<double>::size_type {aka long unsigned int}’ [-Wformat=]
/mnt/build/jenkins/workspace/root-pullrequests-build/root/tree/treeplayer/src/TTreeFormula.cxx:3424:14: warning: unused variable ‘counter’ [-Wunused-variable]

Failing tests:

pcanal · 2017-06-14T11:44:04Z

tree/treeplayer/src/TTreeFormula.cxx

+	      // reset them if instance=0 is seen
+              static Int_t local_index_cache = 0;
+              static Int_t virt_accum_cache = 0;
+	      if (instance==0) {


It might be more stable to have here

if (instance < instance_cache)

Implemented caching using data members + did some cleanup.

pcanal · 2017-06-14T11:45:38Z

tree/treeplayer/src/TTreeFormula.cxx

+	      // caches to keep track of where we are across function invocations
+	      // will avoid an O(N^2) scaling
+	      // reset them if instance=0 is seen
+              static Int_t local_index_cache = 0;


actually I am very wrong :( ...

Those actually need to be data member. More than once TTreeFormula can be handled/processed during the same operation and they can also be nested. For example

tree->Draw("px:pz:fTrack[index]");

the call to EvalInstance for each i will be interleaved and the one with the index will be nested.

phsft-bot · 2017-06-14T11:58:07Z

Build failed on mac1012/native.
See console output.

Warnings:

JENKINS-19022: warning: possible memory leak due to Git plugin usage; see: https://wiki.jenkins-ci.org/display/JENKINS/Remove+Git+Plugin+BuildsByBranch+BuildData
/Volumes/MacintoshHD2/build/workspace/root-pullrequests-build/root/hist/hist/src/TF1.cxx:2693:161: warning: format specifies type 'int' but the argument has type 'size_type' (aka 'unsigned long') [-Wformat]
/Volumes/MacintoshHD2/build/workspace/root-pullrequests-build/root/tree/treeplayer/src/TTreeFormula.cxx:3424:14: warning: unused variable 'counter' [-Wunused-variable]

Failing tests:

sawenzel · 2017-06-14T13:24:58Z

I will be able to do some formatting. git-clang-format causes some global change however.

pcanal · 2017-06-14T13:30:19Z

tree/treeplayer/inc/TTreeFormula.h

@@ -200,7 +210,7 @@ friend class TTreeFormulaManager;
   virtual TTree*      GetTree() const {return fTree;}
   virtual void        UpdateFormulaLeaves();

-   ClassDef(TTreeFormula, 10);  //The Tree formula
+   ClassDef(TTreeFormula, 11); // The Tree formula


No need to increment. The onfile format did not change.

pcanal · 2017-06-16T07:05:15Z

@phsft-bot build

phsft-bot · 2017-06-16T07:05:55Z

Starting build on gcc49/centos7, native/mac1012, gcc49/slc6, gcc62/slc6, native/ubuntu14 with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

phsft-bot · 2017-06-16T07:32:45Z

Build failed on slc6/gcc62.
See console output.

Warnings:

JENKINS-19022: warning: possible memory leak due to Git plugin usage; see: https://wiki.jenkins-ci.org/display/JENKINS/Remove+Git+Plugin+BuildsByBranch+BuildData

Failing tests:

projectroot.roottest.root.meta.roottest_root_meta_drawing

phsft-bot · 2017-06-16T07:48:16Z

Build failed on ubuntu14/native.
See console output.

Warnings:

JENKINS-19022: warning: possible memory leak due to Git plugin usage; see: https://wiki.jenkins-ci.org/display/JENKINS/Remove+Git+Plugin+BuildsByBranch+BuildData

Failing tests:

projectroot.roottest.root.meta.roottest_root_meta_drawing

phsft-bot · 2017-06-16T08:00:05Z

Build failed on mac1012/native.
See console output.

Warnings:

JENKINS-19022: warning: possible memory leak due to Git plugin usage; see: https://wiki.jenkins-ci.org/display/JENKINS/Remove+Git+Plugin+BuildsByBranch+BuildData

Failing tests:

projectroot.roottest.root.meta.roottest_root_meta_drawing

phsft-bot · 2017-06-16T08:13:26Z

Build failed on slc6/gcc49.
See console output.

Warnings:

JENKINS-19022: warning: possible memory leak due to Git plugin usage; see: https://wiki.jenkins-ci.org/display/JENKINS/Remove+Git+Plugin+BuildsByBranch+BuildData

Failing tests:

projectroot.roottest.root.meta.roottest_root_meta_drawing

phsft-bot · 2017-06-16T09:14:32Z

Build failed on centos7/gcc49.
See console output.

Warnings:

JENKINS-19022: warning: possible memory leak due to Git plugin usage; see: https://wiki.jenkins-ci.org/display/JENKINS/Remove+Git+Plugin+BuildsByBranch+BuildData

Failing tests:

projectroot.roottest.root.meta.roottest_root_meta_drawing

* TTreeFormala::GetRealInstance repeatedly went trough a while loop that was trasversed again and again for each new query (with lineary increasing instances). This led to a real problem when drawing on a branch that consistent of many data objects (themselves containing small variable sized containers). * The problem is fixed by caching the state of a previous call.

pcanal · 2017-06-21T13:34:40Z

@phsft-bot build, please

phsft-bot · 2017-06-21T13:34:54Z

Starting build on gcc49/centos7, native/mac1012, gcc49/slc6, gcc62/slc6, native/ubuntu14 with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

phsft-bot · 2017-06-22T07:46:23Z

Build failed on centos7/gcc49.
See console output.

Warnings:

JENKINS-19022: warning: possible memory leak due to Git plugin usage; see: https://wiki.jenkins-ci.org/display/JENKINS/Remove+Git+Plugin+BuildsByBranch+BuildData

amadio · 2017-07-25T07:50:58Z

Hi @sawenzel, has this been merged in another PR? If not, could you please rebase and push so that we can test against the latest master? Most unrelated test failures from above have been fixed. Thanks!

sawenzel · 2017-08-03T08:35:20Z

I think this has been merged can be closed.

pcanal reviewed Jun 14, 2017

View reviewed changes

pcanal requested changes Jun 14, 2017

View reviewed changes

sawenzel force-pushed the swenzel/fixslowTTreeDraw branch from c470183 to 1e67dea Compare June 14, 2017 13:18

sawenzel changed the title ~~WIP: Fixing an O(N^2) scaling problem caused by TTree::Draw()~~ Fixing an O(N^2) scaling problem caused by TTree::Draw() Jun 14, 2017

sawenzel force-pushed the swenzel/fixslowTTreeDraw branch from 1e67dea to 229c886 Compare June 14, 2017 13:23

pcanal reviewed Jun 14, 2017

View reviewed changes

sawenzel force-pushed the swenzel/fixslowTTreeDraw branch 3 times, most recently from 491bd41 to 0531abb Compare June 14, 2017 13:45

sawenzel force-pushed the swenzel/fixslowTTreeDraw branch from 0531abb to a2b3866 Compare June 21, 2017 13:15

sawenzel force-pushed the swenzel/fixslowTTreeDraw branch from a2b3866 to c172e22 Compare June 21, 2017 13:18

sawenzel closed this Aug 3, 2017

This was referenced Nov 17, 2017

Modules test pr #1340

Closed

[cxxmodules] Use the API to load top level system modules. #1344

Closed

[cxxmodules] Disable module file validation. #1345

Merged

[testing] assert on fwd delcs with modules #1349

Closed

phsft-bot mentioned this pull request Mar 29, 2019

[metacling] Optimize TCling*Info interfaces #3616

Merged

Fixing an O(N^2) scaling problem caused by TTree::Draw() #650

Fixing an O(N^2) scaling problem caused by TTree::Draw() #650

Uh oh!

Conversation

sawenzel commented Jun 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phsft-bot commented Jun 14, 2017

Uh oh!

sawenzel commented Jun 14, 2017

Uh oh!

pcanal commented Jun 14, 2017

Uh oh!

pcanal commented Jun 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phsft-bot commented Jun 14, 2017

Uh oh!

phsft-bot commented Jun 14, 2017

Warnings:

Failing tests:

Uh oh!

phsft-bot commented Jun 14, 2017

Warnings:

Failing tests:

Uh oh!

phsft-bot commented Jun 14, 2017

Warnings:

Failing tests:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phsft-bot commented Jun 14, 2017

Warnings:

Failing tests:

Uh oh!

sawenzel commented Jun 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pcanal commented Jun 16, 2017

Uh oh!

phsft-bot commented Jun 16, 2017

Uh oh!

phsft-bot commented Jun 16, 2017

Warnings:

Failing tests:

Uh oh!

phsft-bot commented Jun 16, 2017

Warnings:

Failing tests:

Uh oh!

phsft-bot commented Jun 16, 2017

Warnings:

Failing tests:

Uh oh!

phsft-bot commented Jun 16, 2017

Warnings:

Failing tests:

Uh oh!

phsft-bot commented Jun 16, 2017

Warnings:

Failing tests:

Uh oh!

pcanal commented Jun 21, 2017

Uh oh!

phsft-bot commented Jun 21, 2017

Uh oh!

phsft-bot commented Jun 22, 2017

sawenzel commented Jun 14, 2017 •

edited

Loading