-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[do not merge] [runtime-cxxmodules] Rework our lazy template specialization deserialization mechanism #14495
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
base: master
Are you sure you want to change the base?
Conversation
Starting build on |
Build failed on ROOT-ubuntu2204/nortcxxmod. Failing tests: |
I believe you need the "clean build" label here to avoid incremental builds tripping over the old pcms... |
Build failed on ROOT-ubuntu2004/python3. Failing tests: |
I am lost -- was only triggering a rebuild enough? If so, then I find it hard to believe we cannot compile root without these patches. |
No you need a new workflow run (just push an empty commit or amend the last to get a new commit hash) - the state of labels is recorded at the beginning to avoid some builds running with different settings than others |
Starting build on |
Build failed on ROOT-ubuntu2204/nortcxxmod. Failing tests: |
Build failed on ROOT-ubuntu2004/python3. Failing tests: |
Test Results 11 files 11 suites 1d 19h 27m 24s ⏱️ For more details on these failures, see this check. Results for commit 552a7b4. ♻️ This comment has been updated with latest results. |
0768c38
to
71937e9
Compare
Starting build on |
Build failed on ROOT-ubuntu2004/python3. Errors:
|
Build failed on ROOT-ubuntu2204/nortcxxmod. Errors:
|
71937e9
to
3387aa4
Compare
Starting build on |
Build failed on ROOT-ubuntu2204/nortcxxmod. Failing tests: |
Build failed on ROOT-ubuntu2004/python3. Failing tests: |
@hahnjo, according to our ci this patch seems fine. @smuzaffar, can we run this PR against cmssw? cc: @ChuanqiXu9 |
IIUC |
CMSSW tests are running via cms-sw#198 (note that cmssw tests are running for non-CXXMODULES builds) |
Starting build on |
@smuzaffar, can you interpret the results? |
Build failed on ROOT-ubuntu2204/nortcxxmod. Failing tests: |
Build failed on ROOT-ubuntu2004/python3. Failing tests: |
637378e
to
f714dda
Compare
Starting build on |
@ChuanqiXu9, that is really awesome! Apologies for the oversight of the ExternalASTSource. I've pushed these changes and let's see what happens. |
@ChuanqiXu9, the tests look good on our side. I will compare performance but I think we should bring these changes to the upstream pull request and ask the google folks to test the scalability of the patch. |
@vgvassilev Great! For performances, it looks like you lack a commit from the branch https://github.com/ChuanqiXu9/root/commits/chuanqi_pr83233/, which tries to reduce the time we calculating the ODR Hash. |
Do you mean this one: ChuanqiXu9@2467fe7 ? |
@vgvassilev FYI |
Yes |
Ha, ok, until now I only saw |
Starting build on |
Starting build on |
Starting build on |
Build failed on ROOT-ubuntu2204/nortcxxmod. Errors:
|
Build failed on windows10/default. Errors:
|
@ChuanqiXu9, looks like the commit I took is broken. Did you forget to push your latest changes? |
Build failed on mac12arm/cxx20. Errors:
|
Weird. It compiles good locally. I don't know why it can work in my local environment as it accesses the protected type actually. I just tried to refactor this as the newest commit in https://github.com/ChuanqiXu9/root/commits/chuanqi_pr83233/ |
Since we need access the protect member in findSpecializationLocally
Starting build on |
Build failed on windows10/default. Failing tests: |
@ChuanqiXu9, this looks good on our side. Performance is also within the same ballpark. If I run
It is surprising that we lose 1MB somewhere between D41416 and your changes... @smuzaffar, can you test this PR against cmssw? |
Great! I'll try to invite Google to test for it.
It looks like it is slightly faster (I know it is not accurate). The size overhead may comes from the hash table I guess. Time/Space trade offs. I guess it makes sense in the high level in some degree.
|
This PR tries to make progress with upstreaming our lazy template specialization patch lives only in our llvm forks ( https://reviews.llvm.org/D41416) in favor of re-implementing it in a way that we can upstream it (llvm/llvm-project#76774)