Don't use pkgimage for package if any includes fall in tracked path for coverage or alloc tracking #48183
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
IanButterworth
added
backport 1.9
IanButterworth
force-pushed
the
ib/pkgimage_coverage
branch
2 times, most recently
from
04a742f
to
07203d2
Compare
IanButterworth
commented
Jan 9, 2023
vchuravy
reviewed
Jan 9, 2023
|
You will also need to make sure that we don't create a "poisoned" cachefile. Cachefile creation happens here Line 1940 in 708d1bd compile-cache from propagating. Ideally the caller to create_expr_cache would get it rid and pass in nothing for the object output.
Then you need to adapt the command line checking and allow |
41 tasks
IanButterworth
force-pushed
the
ib/pkgimage_coverage
branch
from
c69bf43
to
50eadde
Compare
35 tasks
51 tasks
35 tasks
IanButterworth
added
the
backport 1.10
KristofferC
added a commit
that referenced
this pull request
Aug 18, 2023
Backported PRs: - [x] #47782 <!-- Generalize Bool parse method to AbstractString --> - [x] #48634 <!-- Remove unused "deps" mechanism in internal sorting keywords [NFC] --> - [x] #49931 <!-- Lock finalizers' lists at exit --> - [x] #50064 <!-- Fix numbered prompt with input only with comment --> - [x] #50474 <!-- docs: Fix a `!!! note` which was miscapitalized --> - [x] #50516 <!-- Fix visibility of assert on GCC12/13 --> - [x] #50635 <!-- `versioninfo()`: include build info and unofficial warning --> - [x] #49915 <!-- Revert "Remove number / vector (#44358)" --> - [x] #50781 <!-- fix `bit_map!` with aliasing --> - [x] #50845 <!-- fix #50438, use default pool for at-threads --> - [x] #49031 <!-- Update inference.md --> - [x] #50289 <!-- Initialize prev_nold and nold in gc_reset_page --> - [x] #50559 <!-- Expand kwcall lowering positional default check to vararg --> - [x] #49582 <!-- Update HISTORY.md for `DelimitedFiles` --> - [x] #50341 <!-- invokelatest docs should say not exported before 1.9 --> - [x] #50525 <!-- only check that values are finite in `generic_lufact` when `check=true` --> - [x] #50444 <!-- Optimize getfield lowering to avoid boxing in some cases --> - [x] #50523 <!-- Avoid generic call in most cases for getproperty --> - [x] #50860 <!-- Add `Base.get_extension` to docs/API --> - [x] #50164 <!-- codegen: handle dead code with unsafe_store of FCA pointers --> - [x] #50568 <!-- `Array(::AbstractRange)` should return an `Array` --> - [x] #50871 <!-- macOS: Don't inspect dead threadtls during exception handling. --> Need manual backport: - [ ] #48542 <!-- Add docs on task-specific buffering using multithreading --> - [ ] #50591 <!-- build: fix various makefile bugs --> Non-merged PRs with backport label: - [ ] #50842 <!-- Avoid race conditions with recursive rm --> - [ ] #50823 <!-- Make ranges more robust with unsigned indexes. --> - [ ] #50663 <!-- Fix Expr(:loopinfo) codegen --> - [ ] #49716 <!-- Update varinfo() docstring signature --> - [ ] #49713 <!-- prevent REPL from erroring in numbered mode in some situations --> - [ ] #49573 <!-- Implement jl_cpu_pause on PPC64 --> - [ ] #48726 <!-- fix macro expansion of property destructuring --> - [ ] #48642 <!-- Use gc alloc instead of alloc typed in lowering --> - [ ] #48183 <!-- Don't use pkgimage for package if any includes fall in tracked path for coverage or alloc tracking --> - [ ] #48050 <!-- improve `--heap-size-hint` arg handling --> - [ ] #47615 <!-- Allow threadsafe access to buffer of type inference profiling trees -->
This was referenced Aug 19, 2023
IanButterworth
force-pushed
the
ib/pkgimage_coverage
branch
2 times, most recently
from
4641d14
to
3bb455a
Compare
IanButterworth
force-pushed
the
ib/pkgimage_coverage
branch
from
3bb455a
to
9d87f97
Compare
IanButterworth
force-pushed
the
ib/pkgimage_coverage
branch
from
9d87f97
to
a085bf0
Compare
|
@vchuravy this is largely working except that the no pkgimage .ji cache that is generated is recorded as |
vchuravy
reviewed
Nov 5, 2023
| for chi in includes | ||
| startswith(chi.filename, tracked_path) || continue | ||
| @debug "Allowing pkgimages=no for $modkey because it falls in coverage/allocation tracking path $tracked_path" | ||
| current_flags &= 0b11111110 # disable pkgimages flag |
There was a problem hiding this comment.
Can we avoid a magic number here? Maybe CacheFlags(current_flags, pkgimage=no)
There was a problem hiding this comment.
current_flags needs to be passed into the c func jl_match_cache_flags so I didn't think the transform should be done to the julia struct
There was a problem hiding this comment.
The struct is the only thing in Julia that knows the position of the bits. I am trying to avoid a circumstance where we move the bits around and forget to update this.
vchuravy
reviewed
Nov 5, 2023
| @@ -599,20 +599,20 @@ JL_DLLEXPORT uint8_t jl_cache_flags(void) | |||
| { | |||
| // OOICCDDP | |||
| uint8_t flags = 0; | |||
| flags |= (jl_options.use_pkgimages & 1); // 0-bit | |||
| // don't use use_pkgimages here because no outputo overrides it | |||
| flags |= ((jl_options.outputo == NULL || jl_options.outputo[0] == '\0') | 1); // 0-bit | |||
There was a problem hiding this comment.
This seems a bit odd... Are you missing the negation? And | 1 won't it put the pkgimage bit high always?
Merged
1 task
|
Replaced by #52123 |
KristofferC
removed
backport 1.9
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.
Fixes #48071
Makes disabling pkgimages specific to the package being coverage (or alloc) tracked.
i.e. should increase the testing of pkgimages in ecosystem CI
Does more than this need to be done @vchuravy ? I'm guessing so given code_coverage != 0 disables pkgimages globally, I believe?