[cxxmodules][cling] Avoid loading some unnecessary modules #10910
Conversation
|
Can one of the admins verify this patch? |
|
@phsft-bot build! |
|
Starting build on |
|
@phsft-bot build! @Axel-Naumann, can we whitelist @junaire? |
|
Starting build on |
junaire
changed the title
junaire
changed the title
|
We passed the whole testsuite, great! I think it's ready for the review :) |
|
|
@phsft-bot build! |
|
Starting build on |
|
@davidlange6, can we pick up this PR in the CXXMODULES IB and test if we bring down memory footprint? |
|
@phsft-bot build! |
|
Starting build on |
|
After b544a58, the size of master branch: after 44434f0 (without the fix): after b544a58 (with the fix): But consider it still under 1 MB, and relatively small compared to other stuff like |
|
Maybe only register function declarations, I can see it reduce the size from |
|
@vgvassilev - I added this patch for tonight's cmssw modules IB for a test... |
Thanks! |
Thank you, David! BTW how can I see the test result? |
|
It wasn't an automatic test, I just have logs of memory used by an example job.
… On Jul 12, 2022, at 12:14 PM, Jun Zhang ***@***.***> wrote:
@vgvassilev - I added this patch for tonight's cmssw modules IB for a test...
Thank you, David! BTW how can I see the test result?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.
|
|
@smuzaffar, can we trigger a cmssw IB for this change. I doubt there will be any breakage but let's just make sure that's the case. |
There was a problem hiding this comment.
Thanks for working on this! Let's aim to merge this PR - I have added several comments.
We might be thinking how to make the patch in clang's GlobalModuleIndex smaller or orthogonal to the existing functionality as we might be breaking some upstream use cases.
core/metacling/src/TCling.cxx
Outdated
| DefinitionIDs[ND->getName()].push_back(OwningModule->getASTFile()); | ||
| InterestingIDInfo &Info = DefinitionIDs[ND->getName()]; | ||
|
|
||
| if (auto *NSD = dyn_cast_or_null<NamespaceDecl>(ND)) { |
There was a problem hiding this comment.
| if (auto *NSD = dyn_cast_or_null<NamespaceDecl>(ND)) { | |
| if (auto *NSD = dyn_cast<NamespaceDecl>(ND)) { |
I don't think ND can be nullptr.
| @@ -316,10 +316,17 @@ bool TClingCallbacks::findInGlobalModuleIndex(DeclarationName Name, bool loadFir | |||
| // FIXME: We should load only the first available and rely on other callbacks | |||
| // such as RequireCompleteType and LookupUnqualified to load all. | |||
| GlobalModuleIndex::FileNameHitSet FoundModules; | |||
| bool K = false; | |||
| if (Index->lookupIdentifier(Name.getAsString(), K, FoundModules)) { | ||
| if (K) { | ||
| if (gDebug > 2) | ||
| llvm::errs() << "\x1B[31m\"" << Name.getAsString() << "\" is a top level namespace," |
There was a problem hiding this comment.
Can we avoid adding console colors? Let's just use plain text which we can grep if the output is too much.
| // when it's a NamespaceDecl. | ||
| unsigned Data = 0; | ||
|
|
||
| void setDeclKind(clang::Decl::Kind Kd) { |
There was a problem hiding this comment.
| void setDeclKind(clang::Decl::Kind Kd) { | |
| void setDeclKind(clang::Decl::Kind K) { |
| @@ -340,6 +346,12 @@ bool GlobalModuleIndex::lookupIdentifier(StringRef Name, HitSet &Hits) { | |||
| } | |||
|
|
|||
| bool GlobalModuleIndex::lookupIdentifier(StringRef Name, FileNameHitSet &Hits) { | |||
| bool K; | |||
There was a problem hiding this comment.
We need a better name here. Maybe IsThisANamespace?
|
If we only care about if the identifier we look up is a top-level namespace or not, maybe we don't need to store the |
|
Starting build on |
|
Starting build on |
|
Build failed on ROOT-debian10-i386/soversion. |
|
Build failed on ROOT-performance-centos8-multicore/cxx17. Failing tests:
|
|
Build failed on ROOT-ubuntu2004/python3. Failing tests: |
|
Build failed on mac1015/cxx17. Failing tests:
|
|
@junaire, the PS: It looks like we can add one of them in |
|
Starting build on |
|
Build failed on ROOT-debian10-i386/soversion. |
|
Build failed on ROOT-performance-centos8-multicore/cxx17. Failing tests: |
|
Build failed on mac11/cxx14. Failing tests:
|
|
Build failed on mac1015/cxx17. Failing tests: |
|
Seems it saves O(10MB) of RSS. Good, but not the solution to the large memory increase in cmssw jobs.
… On Jul 11, 2022, at 7:59 PM, Vassil Vassilev ***@***.***> wrote:
@vgvassilev - I added this patch for tonight's cmssw modules IB for a test...
Thanks!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.
|
|
@davidlange6 is the RSS increase strictly due to this PR, or could it be due to some other change in master? |
|
This was some mail I sent months ago that just resurfaced. I think its out of context now and should be ignored..
… On Oct 11, 2022, at 3:27 PM, Axel Naumann ***@***.***> wrote:
@davidlange6 is the RSS increase strictly due to this PR, or could it be due to some other change in master?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.
|
When we run into an unkown identifier that is a namespace, we don't really need to load its corresponding modules. Instead, we create a new module that forward declared all namespaces and always load it first. By doing so, we can avoid loading a lot of unnecessary modules. Signed-off-by: Jun Zhang <jun@junz.org>
Signed-off-by: Jun Zhang <jun@junz.org>
Signed-off-by: Jun Zhang <jun@junz.org>
|
Starting build on |
|
Build failed on ROOT-performance-centos8-multicore/cxx17. Errors:
And 3 more Failing tests:
And 15 more |
|
Build failed on ROOT-ubuntu2004/python3. Failing tests:
|
|
Build failed on ROOT-debian10-i386/soversion. Failing tests: |
Signed-off-by: Jun Zhang <jun@junz.org>
|
Starting build on |
|
Build failed on mac11/cxx14. Failing tests:
|
|
Build failed on mac1015/cxx17. Failing tests:
|
When we run into an unkown identifier that is a namespace, we don't
really need to load its corresponding modules. Instead, we create a new
module that forward declared all namespaces and always load it first. By
doing so, we can avoid loading a lot of unnecessary modules.
Signed-off-by: Jun Zhang jun@junz.org