Prevent tainting native code loading from propagating #53457
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When we use options like code coverage, we can't use the native code present in the cache file since it is not instrumented. PR #52123 introduced the capability of skipping the native code during loading, but created the issue that subsequent packages could have an explicit or implicit dependency on the native code. PR #53439 tainted the current process by setting `use_sysimage_native_code`, but this flag is propagated to subprocesses and lead to a regression in test time. Move this to a process local flag to avoid the regression. In the future we might be able to change the calling convention for cross-image calls to `invoke(ci::CodeInstance, args...)` instead of `ci.fptr(args...)` to handle native code not being present.
vtjnash
reviewed
Feb 24, 2024
|
Ohoho, you've done me one better than just fixing it, you've seemingly improved CI times too! Before the regression: With the regression: After the fix: Much ❤️ for the quick turnaround on this! |
KristofferC
pushed a commit
that referenced
this pull request
Feb 26, 2024
When we use options like code coverage, we can't use the native code present in the cache file since it is not instrumented. PR #52123 introduced the capability of skipping the native code during loading, but created the issue that subsequent packages could have an explicit or implicit dependency on the native code. PR #53439 tainted the current process by setting `use_sysimage_native_code`, but this flag is propagated to subprocesses and lead to a regression in test time. Move this to a process local flag to avoid the regression. In the future we might be able to change the calling convention for cross-image calls to `invoke(ci::CodeInstance, args...)` instead of `ci.fptr(args...)` to handle native code not being present. --------- Co-authored-by: Jameson Nash <[email protected]> (cherry picked from commit b8a0a39)
|
Being faster is suspicious. Are coverage results as expected? |
|
For what is worth, also the sequential total time of PkgEval is sensibly lower on b8a0a39 than on previous runs:
I don't know what's the variability of these times, maybe @maleadt has more insights about this. |
Not 100% sure whether I'm looking at the right thing, but the coverage report seems fine to me. Might be that codecov is mixing it between nightly and 1.10 though, not sure 🤔 |
This may also be caused by precompilation of the test environment failing before #53387, which is only part of the latest evaluation. However, it still looks better than before that bug started happening, at which point PkgEval took around 30 days. |
LilithHafner
removed
the
merge me
tecosaur
pushed a commit
to tecosaur/julia
that referenced
this pull request
Mar 4, 2024
When we use options like code coverage, we can't use the native code present in the cache file since it is not instrumented. PR JuliaLang#52123 introduced the capability of skipping the native code during loading, but created the issue that subsequent packages could have an explicit or implicit dependency on the native code. PR JuliaLang#53439 tainted the current process by setting `use_sysimage_native_code`, but this flag is propagated to subprocesses and lead to a regression in test time. Move this to a process local flag to avoid the regression. In the future we might be able to change the calling convention for cross-image calls to `invoke(ci::CodeInstance, args...)` instead of `ci.fptr(args...)` to handle native code not being present. --------- Co-authored-by: Jameson Nash <[email protected]>
mkitti
pushed a commit
to mkitti/julia
that referenced
this pull request
Mar 7, 2024
When we use options like code coverage, we can't use the native code present in the cache file since it is not instrumented. PR JuliaLang#52123 introduced the capability of skipping the native code during loading, but created the issue that subsequent packages could have an explicit or implicit dependency on the native code. PR JuliaLang#53439 tainted the current process by setting `use_sysimage_native_code`, but this flag is propagated to subprocesses and lead to a regression in test time. Move this to a process local flag to avoid the regression. In the future we might be able to change the calling convention for cross-image calls to `invoke(ci::CodeInstance, args...)` instead of `ci.fptr(args...)` to handle native code not being present. --------- Co-authored-by: Jameson Nash <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When we use options like code coverage, we can't use the native code
present in the cache file since it is not instrumented.
PR #52123 introduced the capability of skipping the native
code during loading, but created the issue that subsequent packages
could have an explicit or implicit dependency on the native code.
PR #53439 tainted the current process by setting
use_sysimage_native_code, but this flag is propagated to subprocessesand lead to a regression in test time.
Move this to a process local flag to avoid the regression.
In the future we might be able to change the calling convention for
cross-image calls to
invoke(ci::CodeInstance, args...)instead ofci.fptr(args...)to handle native code not being present.