[metacling] Optimize TCling*Info interfaces #3616
Conversation
|
Starting build on |
vgvassilev
force-pushed
the
meta-cling-optimize-tclingdatamemberinof
branch
from
d42a2ad
to
5576e44
Compare
|
Starting build on |
vgvassilev
changed the title
|
Build failed on windows10/default. |
|
@phsft-bot build again please. |
|
Starting build on |
|
Build failed on windows10/default. |
vgvassilev
force-pushed
the
meta-cling-optimize-tclingdatamemberinof
branch
from
5576e44
to
91f2ecc
Compare
|
Starting build on |
|
Build failed on ROOT-performance-centos7-multicore/default. Errors:
Failing tests:
And 37 more |
…ied. We cannot merge both codepaths because root.exe should always run with modules if -Druntime_cxxmodules is specified, however, rootcling enables modules only if -cxxmodule flag is passed. This patch fixes asserts when ROOT is built in -Druntime_cxxmodules=On but no -cxxmodule flag is specified (in tree/test for instance).
vgvassilev
force-pushed
the
meta-cling-optimize-tclingdatamemberinof
branch
from
91f2ecc
to
9e85377
Compare
|
Starting build on |
|
Starting build on |
|
Build failed on windows10/default. |
vgvassilev
force-pushed
the
meta-cling-optimize-tclingdatamemberinof
branch
from
72d9c68
to
53d2202
Compare
|
Starting build on |
|
Build failed on windows10/default. |
|
Looks like the windows failure is unrelated. |
vgvassilev
force-pushed
the
meta-cling-optimize-tclingdatamemberinof
branch
from
53d2202
to
ae330ea
Compare
|
Starting build on |
|
@pcanal, this is ready for a review. |
|
Build failed on windows10/default. |
| const clang::Decl *GetDecl() const { return fDecl; } // Underlying representation without Double32_t | ||
| void SetDecl(const clang::Decl* D) { | ||
| // FIXME: We should track down all set and potentially avoid them. | ||
| fDecl = D; |
There was a problem hiding this comment.
Shouldn't this invalidate the name caches?
| return fNameCache.c_str(); | ||
|
|
||
| const clang::TypedefNameDecl *td = llvm::cast<clang::TypedefNameDecl>(fDecl); | ||
| const clang::ASTContext &ctxt = GetDecl()->getASTContext(); |
There was a problem hiding this comment.
Is there a different between the value of fDecl and GetDecl() here?
There was a problem hiding this comment.
No, this class does not override GetDecl and the base class implementation returns fDecl.
There was a problem hiding this comment.
If they are the same, can we use one of the two in both line (otherwise the future reader will be puzzled :) )
vgvassilev
force-pushed
the
meta-cling-optimize-tclingdatamemberinof
branch
from
ae330ea
to
d2112c6
Compare
|
Starting build on |
|
Build failed on windows10/default. |
core/metacling/src/TClingClassInfo.h
Outdated
| void SetDecl(const clang::Decl* D) { | ||
| // FIXME: We should track down all sets and potentially avoid them. | ||
| fDecl = D; | ||
| fNameCache = {}; // invalidate the cache. |
There was a problem hiding this comment.
Is that 'better' than fNameCache.clear();
There was a problem hiding this comment.
Clear should be faster as it does not make a copy I think. Quickbench shows it is 5 times faster on some implementations...
There was a problem hiding this comment.
Cool. I also find it semantically clearer :)
This patch refactors the code by moving the common parts in a base class. It implements proper caching if a TClingDataMemberInfo::Name is called multiple times. The correct caching saves approximately 100 Mb of temporary allocations in runtime_modules but also saves 10Mb on some workflows in standard ROOT.
The base class caching reduces the temporary memory allocations.
This patch removes the redundant norm context method arguments.
vgvassilev
force-pushed
the
meta-cling-optimize-tclingdatamemberinof
branch
from
d2112c6
to
19f1a75
Compare
|
Starting build on |
|
Build failed on ROOT-ubuntu18.04-i386/cxx14. Errors:
|
Currently, if ::Name() interface is called we pretty print the Decl name. This is suboptimal because it causes many memory allocations for something which is essentially immutable.
This PR introduces step-by-step working cache if ::Name() was called. It reduces the temporary memory allocations by 12 Mb in standard ROOT and 130Mb in -Druntime_cxxmodules=On cache. The benchmarking test was provided by @pcanal in #3012.
It is important to reduce the temporary allocations because they can contribute to increasing of the peak memory usage of ROOT.