Skip to content
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

coverage: count coverage where explicitly requested by inference only #53125

Closed
wants to merge 2 commits into from

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Jan 30, 2024

Helps somewhat to avoid coverage from being counted for any execution of any part of a line, instead of for executing the start of a line, which we have noticed over-counts coverage in some cases.

Helps somewhat to avoid coverage from being counted for any execution of
any part of a line, instead of for executing the start of a line.
@vtjnash vtjnash added compiler:codegen Generation of LLVM IR and native code testsystem The unit testing framework and Test stdlib labels Jan 30, 2024
@vtjnash vtjnash marked this pull request as draft January 30, 2024 20:36
@vtjnash
Copy link
Member Author

vtjnash commented Jan 30, 2024

Marked as a draft since the CI coverage worker has been broken for a couple months (JuliaCI/julia-buildkite#320 (comment)) so we need to get that green first a few times before merging

@sjkelly sjkelly added the backport 1.10 Change should be backported to the 1.10 release label Jan 30, 2024
@oscardssmith oscardssmith added the bugfix This change fixes an existing bug label Jan 31, 2024
@KristofferC
Copy link
Member

-20% sounds a bit much?

@vtjnash
Copy link
Member Author

vtjnash commented Feb 4, 2024

That is about what I expected in the current state of affairs. We have roughly 3 options I think:

  1. put this behind a flag (runtime or buildtime) and merge as-is
  2. have this automatically only apply to code in base (in the system image) and not apply to user code (& revert Use pkgimages for coverage & malloc tracking by ignoring native code in tracked packages #52123)
  3. merge enough of the boundschecking work needed to represent effect preconditions, so that we can properly model coverage as the effect that it is in the world-age-system model (@Keno RFC: Effect Preconditions - or - the future of @inbounds #50641)

@IanButterworth
Copy link
Member

Re. plan 2, reverting #52123 alone without a replacement fix would cause a big slow down in user CI when coverage is on (the default for the julia-runtest action), largely because Pkg being fast relies on its pkgimage now.

@ViralBShah ViralBShah added this to the 1.11 milestone Feb 5, 2024
@KristofferC KristofferC mentioned this pull request Feb 6, 2024
9 tasks
KristofferC added a commit that referenced this pull request Feb 6, 2024
A few stragglers.

Backported PRs:
- [x] #53091 <!-- Ensure elision of `require_one_based_indexing` with
high-dim array views -->
- [x] #53117 <!-- Try to fix incorrect documentation of `nthreads` -->
- [x] #52855 <!-- Fix variable name in scaling an `AbstractTriangular`
with zero alpha -->
- [x] #52952 <!-- [REPLCompletions] enable completions for `using
Module.Inner|` -->
- [x] #53101 <!-- Inplace transpose for unit Triangular may skip
diagonal -->

Need manual backport:
- [ ] #52505 <!-- fix alignment of emit_unbox_store copy -->

Non-merged PRs with backport label:
- [ ] #53125 <!-- coverage: count coverage where explicitly requested by
inference only -->
- [ ] #52694 <!-- Reinstate similar for AbstractQ for backward
compatibility -->
- [ ] #51479 <!-- prevent code loading from lookin in the versioned
environment when building Julia -->
@KristofferC KristofferC added the backport 1.11 Change should be backported to release-1.11 label Feb 16, 2024
This was referenced Feb 20, 2024
KristofferC added a commit that referenced this pull request Feb 27, 2024
Backported PRs:
- [x] #53205 <!-- Profile: add notes to `print()` docs -->
- [x] #53170 <!-- Remove outdated discussion about externally changing
module bindings -->
- [x] #53228 <!-- SubArray: avoid invalid elimination of singleton
indices -->
- [x] #51361 <!-- code_warntype docs: more neutral reference to
@code_warntype -->
- [x] #50480 <!-- Document --heap-size-hint in Command-line Interface
-->
- [x] #53301 <!-- Fix typo in `Sys.total_memory` docstring. -->
- [x] #53354 <!-- fix code coverage bug in tail position and `else` -->
- [x] #53388 <!-- Fix documentation: thread pool of main thread -->
- [x] #53429 <!-- Subtype: skip slow-path in `local_∀_∃_subtype` if
inputs contain no ∃ typevar. -->
- [x] #53437 <!-- Add debug variant of loader_trampolines.o -->

Need manual backport:
- [ ] #52505 <!-- fix alignment of emit_unbox_store copy -->
- [ ] #53373 <!-- fix sysimage-native-code=no option with pkgimages -->
- [ ] #53439 <!-- staticdata: fix assert from partially disabled native
code -->

Contains multiple commits, manual intervention needed:
- [ ] #52913 <!-- Added docstring for Artifacts.jl -->
- [ ] #53218 <!-- Fix interpreter_exec.jl test -->

Non-merged PRs with backport label:
- [ ] #53424 <!-- yet more atomics & cache-line fixes on work-stealing
queue -->
- [ ] #53125 <!-- coverage: count coverage where explicitly requested by
inference only -->
- [ ] #52694 <!-- Reinstate similar for AbstractQ for backward
compatibility -->
- [ ] #51479 <!-- prevent code loading from lookin in the versioned
environment when building Julia -->
@sjkelly
Copy link
Contributor

sjkelly commented Feb 28, 2024

@vtjnash is this still an outstanding bug or is this supplanted by: #53354

@vtjnash vtjnash closed this Feb 28, 2024
@vtjnash vtjnash deleted the jn/over-coverage-fix branch February 28, 2024 17:18
@vtjnash
Copy link
Member Author

vtjnash commented Feb 28, 2024

I don't know if #53354 was sufficient to resolve the bug, but I don't think we would need to take this approach anymore regardless

KristofferC added a commit that referenced this pull request Mar 1, 2024
Backported PRs:
- [x] #53361 <!-- 🤖 [master] Bump the SparseArrays stdlib from c9f7293
to cb602d7 -->
- [x] #53300 <!-- allow external absint to hold custom data in
`codeinst.inferred` -->
- [x] #53342 <!-- Add `Base.wrap` to docs -->
- [x] #53372 <!-- Silence warnings in `test/file.jl` -->
- [x] #53357 <!-- 🤖 [master] Bump the Pkg stdlib from 6dd0e7c9e to
76070d295 -->
- [x] #53373 <!-- fix sysimage-native-code=no option with pkgimages -->
- [x] #53333 <!-- More consistent return value for annotations, take 2
-->
- [x] #53354 <!-- fix code coverage bug in tail position and `else` -->
- [x] #53407 <!-- fix sysimage-native-code=yes option -->
- [x] #53388 <!-- Fix documentation: thread pool of main thread -->
- [x] #53355 <!-- Fix synchronization issues on the GC scheduler. -->
- [x] #53429 <!-- Subtype: skip slow-path in `local_∀_∃_subtype` if
inputs contain no ∃ typevar. -->
- [x] #53437 <!-- Add debug variant of loader_trampolines.o -->
- [x] #53284 <!-- Add annotate! method for AnnotatedIOBuffer -->
- [x] #53466 <!-- [MozillaCACerts_jll] Update to v2023-12-12 -->
- [x] #53467 <!-- [LibGit2_jll] Update to v1.7.2 -->
- [x] #53326 <!-- RFC: when loading code for internal purposes, load
stdlib files directly, bypassing DEPOT_PATH, LOAD_PATH, and stale checks
-->
- [x] #53332
- [x] #53320 <!-- Add `Sys.isreadable, Sys.iswriteable`, update `ispath`
-->
- [x] #53476

Contains multiple commits, manual intervention needed:
- [ ] #53285 <!-- Add update mechanism for Terminfo, and common
user-alias data -->

Non-merged PRs with backport label:
- [ ] #53424 <!-- yet more atomics & cache-line fixes on work-stealing
queue -->
- [ ] #53408 <!-- task splitting: change additive accumulation to
multiplicative -->
- [ ] #53403 <!-- Move parallel precompilation to Base -->
- [ ] #53402 <!-- Add `jl_getaffinity` and `jl_setaffinity` -->
- [ ] #53391 <!-- Default to the medium code model in x86 linux -->
- [ ] #53125 <!-- coverage: count coverage where explicitly requested by
inference only -->
- [ ] #52694 <!-- Reinstate similar for AbstractQ for backward
compatibility -->
@KristofferC KristofferC removed the backport 1.11 Change should be backported to release-1.11 label Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.10 Change should be backported to the 1.10 release bugfix This change fixes an existing bug compiler:codegen Generation of LLVM IR and native code testsystem The unit testing framework and Test stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants