Reduce/improve includes usage in ROOT header files #5535

linev · 2020-05-04T17:44:11Z

Replace includes by forward declarations were class definitions not required.
In places where includes may be required by user code keep it under R__LESS_INCLUDES definition.
RooFit changes extracted into #5542 - there was need for rebase after recent changes

phsft-bot · 2020-05-04T17:44:23Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos7-multicore/default, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1015/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2020-05-04T18:21:09Z

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

Failing tests:

vgvassilev · 2020-05-04T18:26:02Z

core/base/inc/TError.h


-#include "RtypesCore.h"
-#include <stdarg.h>
+#include "Rtypes.h"


That is incorrect. RtypesCore.h allows us to include this header in -fno-rtti context.

In general, I am not quite sure if we should be doing such a header cleanup...

I put here Rtypes.h to also access RConfigure.h immediately, but ok - one can do it explicitly. I will change it.

core/cont/inc/TBits.h

vgvassilev · 2020-05-04T18:29:05Z

core/cont/inc/TClonesArray.h

 //                                                                      //
 //////////////////////////////////////////////////////////////////////////

-#include "Rtypes.h"


phsft-bot · 2020-05-04T18:57:23Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos7-multicore/default, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1015/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2020-05-05T03:54:45Z

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft10.dyndns.cern.ch:/build/workspace/root-pullrequests-build
See cdash.
See console output.

Failing tests:

projectroot.test.test_stressIOPlugins_xroot

phsft-bot · 2020-05-05T09:56:18Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos7-multicore/default, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1015/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2020-05-05T09:57:05Z

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

Errors:

[2020-05-05T09:57:03.459Z] CMake Error at /build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:914 (message):

phsft-bot · 2020-05-05T09:57:07Z

Build failed on ROOT-fedora30/cxx14.
Running on root-fedora30-2.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See cdash.
See console output.

Errors:

[2020-05-05T09:57:05.855Z] CMake Error at /home/sftnight/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:914 (message):

hageboeck · 2020-05-05T09:57:51Z

@linev
I tried to resolve the conflicts on github. The editing mode is actually nice, but I'm not sure if this merge commit that now appeared is what we want. It probably makes sense to squash this into the roofit commit of your branch.

linev · 2020-05-05T09:58:16Z

@hageboeck Do you want to rebase PR against master? If yes, I can do it

linev · 2020-05-05T10:04:50Z

@hageboeck Lets do better. I can apply roofit changes as separate PR. And then rebase this one

phsft-bot · 2020-05-05T10:06:30Z

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See cdash.
See console output.

Errors:

[2020-05-05T10:06:28.916Z] CMake Error at C:/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:914 (message):

phsft-bot · 2020-05-05T17:05:13Z

Build failed on ROOT-fedora29/python3.
Running on root-fedora29-3.cern.ch:/build/workspace/root-pullrequests-build
See cdash.
See console output.

Failing tests:

phsft-bot · 2020-05-06T07:07:47Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos7-multicore/default, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1015/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2020-05-06T10:33:40Z

Build failed on ROOT-fedora29/python3.
Running on root-fedora29-3.cern.ch:/build/workspace/root-pullrequests-build
See cdash.
See console output.

Failing tests:

phsft-bot · 2020-05-07T16:14:04Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos7-multicore/default, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1015/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2020-05-07T16:45:15Z

Build failed on ROOT-fedora30/cxx14.
Running on root-fedora30-2.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See cdash.
See console output.

Failing tests:

phsft-bot · 2020-05-07T17:25:38Z

Build failed on ROOT-fedora29/python3.
Running on root-fedora29-2.cern.ch:/build/workspace/root-pullrequests-build
See cdash.
See console output.

Failing tests:

phsft-bot · 2020-05-07T20:08:31Z

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft10.dyndns.cern.ch:/build/workspace/root-pullrequests-build
See cdash.
See console output.

Failing tests:

linev · 2020-05-11T06:51:08Z

@phsft-bot build

phsft-bot · 2020-05-11T06:51:16Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos7-multicore/default, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1015/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2020-05-13T20:49:15Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos7-multicore/default, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1015/cxx17, windows10/cxx14
How to customize builds

pcanal · 2020-05-27T14:25:39Z

core/base/inc/TROOT.h

 #include "TDirectory.h"
-#include "TList.h"
-#include "RConfigure.h"
+// #include "TList.h"  // included in TDirectory.h


But it is just directly here, so we ought to keep it.

See TDirectory.h - "TList.h" included only without R__LESS_INCLUDES. Means in the dev build TList is just forward declarations. I just want to avoid same construct here.

pcanal · 2020-05-27T14:26:31Z

core/base/inc/TROOT.h

   TVirtualPad      *GetSelectedPad() const { return fSelectPad; }
-   Int_t             GetNclasses() const { return fClasses->GetSize(); }
-   Int_t             GetNtypes() const { return fTypes->GetSize(); }
+   Int_t             GetNclasses() const;


Why make that change?

Because TList.h is not included with R__LESS_INLCUDES - see my previous comment

I see. This illustrate a trade off: small speedup at parsing time (removing the the include of TList.h for the (rare?) file that uses TROOT but not TList) vs a small performance increase (for the code that uses GetNclasses and GetNtypes).

Whether one is better than the other is .. well .. hard to know (and in probably in both case the actual performance change is likely indistinguishable) .. I would probably not have made that change but reverting it is also probably not worth it.

@pcanal Should I revert these changes in TROOT.h?

Up to you in the end. I slightly lean for revert but not if it is too much hassle.

I prefer to keep changes. These methods are not crucial for performance.

linev requested a review from Axel-Naumann May 4, 2020 17:44

linev requested review from agheata, ashlaban, bellenot, couet, dpiparo, etejedor, gganis, hageboeck, lmoneta, osschar, pcanal, sitongan and stwunsch as code owners May 4, 2020 17:44

linev self-assigned this May 4, 2020

vgvassilev reviewed May 4, 2020

View reviewed changes

core/cont/inc/TBits.h Show resolved Hide resolved

vgvassilev reviewed May 4, 2020

View reviewed changes

linev force-pushed the less_includes3 branch from 90f8cd9 to 3b9ddc3 Compare May 4, 2020 18:57

linev force-pushed the less_includes3 branch from 9798b00 to 59510f4 Compare May 6, 2020 07:07

linev force-pushed the less_includes3 branch from 59510f4 to ab826c9 Compare May 7, 2020 16:13

linev added 12 commits May 13, 2020 22:48

Add some missing includes

dc57d84

Add missing includes in tmva

8b46297

Optimize gui includes

134d8cc

Optimize html includes

fdb1994

Optimize graf3d includes

14b12ba

Optimize geom includes

07c7207

Optimize graf2d includes

cfc14de

Optimize hist includes

129d913

Optimize tree includes

ec0f5e7

Optimize io includes

7af8987

Optimize math includes

ab789d4

Optimize core includes

a361721

linev force-pushed the less_includes3 branch from ab826c9 to a361721 Compare May 13, 2020 20:49

pcanal reviewed May 27, 2020

View reviewed changes

pcanal approved these changes May 28, 2020

View reviewed changes

linev merged commit 8dc556f into root-project:master May 28, 2020

linev deleted the less_includes3 branch May 28, 2020 18:38

Reduce/improve includes usage in ROOT header files #5535

Reduce/improve includes usage in ROOT header files #5535

Uh oh!

Conversation

linev commented May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phsft-bot commented May 4, 2020

Uh oh!

phsft-bot commented May 4, 2020

Failing tests:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phsft-bot commented May 4, 2020

Uh oh!

phsft-bot commented May 5, 2020

Failing tests:

Uh oh!

phsft-bot commented May 5, 2020

Uh oh!

phsft-bot commented May 5, 2020

Errors:

Uh oh!

phsft-bot commented May 5, 2020

Errors:

Uh oh!

hageboeck commented May 5, 2020

Uh oh!

linev commented May 5, 2020

Uh oh!

linev commented May 5, 2020

Uh oh!

phsft-bot commented May 5, 2020

Errors:

Uh oh!

phsft-bot commented May 5, 2020

Failing tests:

Uh oh!

phsft-bot commented May 6, 2020

Uh oh!

phsft-bot commented May 6, 2020

Failing tests:

Uh oh!

phsft-bot commented May 7, 2020

Uh oh!

phsft-bot commented May 7, 2020

Failing tests:

Uh oh!

phsft-bot commented May 7, 2020

Failing tests:

Uh oh!

phsft-bot commented May 7, 2020

Failing tests:

Uh oh!

linev commented May 11, 2020

Uh oh!

phsft-bot commented May 11, 2020

Uh oh!

phsft-bot commented May 13, 2020

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!

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!

Choose a reason for hiding this comment

Uh oh!

linev commented May 4, 2020 •

edited

Loading