-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[core] Reduce symbol search only to when autoloading is enabled. #14287
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[core] Reduce symbol search only to when autoloading is enabled. #14287
Conversation
|
Starting build on |
|
This patch seems to be a good performance enhancement (skipping some work that will not be used) but I am confused on how it helps with the increase in autoloading as I would have expected that in the case where this patch is active (i.e. autoloading is disabled) the net effect would have been the same (i.e. autoloading not activated). What am I missing? i.e. this patch looks good to me as-is but does not seem to fully address the problem. |
Test Results 9 files 9 suites 1d 16h 8m 44s ⏱️ For more details on these failures, see this check. Results for commit 55145f0. ♻️ This comment has been updated with latest results. |
|
Reporting some numbers from my machine with this patch, building ROOT with the following cmake invocation: TLDR: The situation w.r.t. #14277 is EDIT: Probably we also want to compare the changes of this patch against ROOT master before the two hotfixes regarding the missing symbols were introduced. I will do that and provide a followup comment. Baseline: master@fbd6abbImport timeTaken after a few runs, hot caches strace file statsopenat stats with the two symbols related to #14277After this patchImport timeTaken after a few runs, hot caches strace file statsopenat stats with the two symbols related to #14277 |
|
Don’t spend time on this patch just yet. I have more local changes that I am going to upload soon-ish. |
99d82a3 to
e95dd7f
Compare
|
Starting build on |
|
@vepadulano, can you run your tests again. With the current changes we support |
e95dd7f to
250de1b
Compare
|
Starting build on |
|
Build failed on ROOT-performance-centos8-multicore/soversion. Failing tests: |
|
Build failed on ROOT-ubuntu2204/nortcxxmod. Failing tests: |
|
Build failed on mac12arm/cxx20. Failing tests: |
|
Build failed on ROOT-ubuntu2004/python3. Failing tests: |
|
Build failed on windows10/default. Failing tests: |
250de1b to
9758c2e
Compare
|
Starting build on |
|
Build failed on ROOT-ubuntu2204/nortcxxmod. Failing tests: |
9758c2e to
5444a38
Compare
|
Starting build on |
|
Build failed on ROOT-ubuntu2204/nortcxxmod. Failing tests: |
UpdatesBaseline: master before #14261 and #14276Master after the two hotfixesThis PR |
|
@vgvassilev It seems that the changes in this PR provide the same performance as the current master (after the two hotfixes), while supporting leaving the Another problem which is still present in both the current master and even after this PR is that the number of file operations is still quite high, i.e. which are exactly the same whether current master or this patch. Consider that many of those Note the presence of the |
Thanks for doing this. That sounds good
I agree, we should also land this PR since it supports now constexpr constructs which can be anywhere in external codes.
These are likely garbage that's put by rootcling into the pcm files. We can verify if that theory is right if we make a build with |
|
sure, started PR tests at cms-sw#193 |
|
Hi @smuzaffar , looking at the CMSSW tests, it seems this patch worked for CMS: could you confirm that? |
dpiparo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @vgvassilev for this fix, delivered so quickly. I propose to merge the code if other reviewers approve the PR and if 2 conditions are met:
- We get the confirmation from CMS that this is all what they need.
- We understand why
roottest-python-basic-basicfails on fedora38 and, if possible, fix the problem.
| DeclId_t d; | ||
| TClingClassInfo *cl = (TClingClassInfo*)opaque_cl; | ||
|
|
||
| // Could trigger deserialization of decls. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand exactly why this had to move up in the cxx file: could you perhaps clarify, please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the tests make GetDataMember now deserialize.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems I can just remove this and tests still pass on my system. Can you assess if this is still needed in the latest version of the PR? Shall we try reverting it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it was needed on my system.
|
(btw, since we are looking for ways to speedup ROOT startup, I think this could also be an avenue #7123) |
Yes @dpiparo , this worked for CMS too. Local tests also show that with this update CMSSW/ROOT symbols mentioned in cms-sw/cmssw#43077 (comment) also do not trigger loading of all shared libs. |
I believe this is not related to this PR, some of the Python tests fail from time to time on Fedora 38 and Fedora 39. I suspect there are still GC-related issues with Python 3.11 and 3.12 that were never fully understood... |
e2af2f9 to
8f4c26d
Compare
|
Starting build on |
|
Hi @vgvassilev , thanks once again for this great development, which was delivered and tested on such a short timescale, basically during the end of the year break. On January 23rd, the last open pre-release of CMSSW_14_0_0 will close: after that date, no ROOT version updates will be possible (see cms-sw/cmssw#43077 (comment)). Do you think you can factor and succinctly document the changes in the way @hahnjo proposed in order to be able to merge the PR to make your fixes available to CMS? We see LHCb experiencing similar problems - they would also likely benefit from your code. |
@dpiparo, I have made the changes that I agree with and responded to the rest of the comments. This PR seems to be ready to go from what concerns me. |
The llvm9 JIT issued callbacks when a symbol was missing and we reacted on it by loading the relevant library. In root-project/root@9b2041e3 we have kept the logic but now the JIT started querying more often even for symbols which are okay to be missing. In turn that leads to scanning all libraries causing performance issues. This patch tries to limit this functionality only in contexts where automatic loading is allowed.
Co-authored-by: Jonas Hahnfeld <[email protected]>
Symbol lookup is a quite expensive operation and might result in JIT compilation and library loading. Co-authored-by: Jonas Hahnfeld <[email protected]>
This reverts commit 329fb5a.
|
Hi @hahnjo , @vgvassilev : thanks a lot for the exchanges and work on this topic. I was wondering if you can estimate an ETA for the merging of this nice work, key for addressing CMS (and LHCbs) current issues in a sustainable way and for the adoption of ROOT 6.30 by CMS in 2024. |
8f4c26d to
55145f0
Compare
|
Starting build on |
hahnjo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any case, the organization is now good from my point of view.
| DeclId_t d; | ||
| TClingClassInfo *cl = (TClingClassInfo*)opaque_cl; | ||
|
|
||
| // Could trigger deserialization of decls. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems I can just remove this and tests still pass on my system. Can you assess if this is still needed in the latest version of the PR? Shall we try reverting it?
dpiparo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dear @vgvassilev , Thanks for these changes. Can you merge them asap? We can then make sure @smuzaffar uses this commit for the CMSSW builds in order for CMS to fully benefit from the improvements.
The llvm9 JIT issued callbacks when a symbol was missing and we reacted on it by loading the relevant library. In 9b2041e3 we have kept the logic but now the JIT started querying more often even for symbols which are okay to be missing. In turn that leads to scanning all libraries causing performance issues.
This patch tries to limit this functionality only in contexts where automatic loading is allowed. In addition when computing the offsets of a constant variable declaration we compute the initializers instead of searching in the shared objects.