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

Code-coverage should be more precise in disabling package images #48071

Closed
vchuravy opened this issue Jan 1, 2023 · 8 comments · Fixed by #52123
Closed

Code-coverage should be more precise in disabling package images #48071

vchuravy opened this issue Jan 1, 2023 · 8 comments · Fixed by #52123
Labels
Milestone

Comments

@vchuravy
Copy link
Member

vchuravy commented Jan 1, 2023

Currently using anyform of code-coverage turns of support for package images.
Code-coverage is implemented by emitting LLVM IR that tracks the use of lines,
and it wasn't clear for me if this is something we would want to cache.

Most of the user CI is run with code-coverage enabled and as such does not use pkgimages yet, giving us little coverage outside PkgEval of how this feature interacts with users. So we are not testing the default user experience on package CI.

I was thinking about how we could fix that and we recently introduced --code-coverage=@path which only filters the paths on which we will enable code-coverage. Sadly this is still a global flag.

Ideally we would be able to create and load a pkgimage for a dependency of the user package that is being instrumented.

cc: @timholy @IanButterworth

@IanButterworth
Copy link
Member

IanButterworth commented Jan 1, 2023

Sadly this is still a global flag.

Do you mean that because setting --code-coverage=@path makes Base.JLOptions().code_coverage == 3 (and sets Base.JLOptions().tracked_path) that turns off pkgimages for all packages?

If that's true, then the check should be something like

use_pkgimage_for_pkg = JLOptions().code_coverage == 0 || (JLOptions().code_coverage == 3 && occursin(JLOptions().tracked_path, file_path))

Am I following correctly?

@vchuravy
Copy link
Member Author

vchuravy commented Jan 2, 2023

Yeah, but I am not sure we can write that condition since a package can include lots of files and while it should only be the package source you could also include across packages

@IanButterworth
Copy link
Member

IanButterworth commented Jan 2, 2023

Would it be possible to ahead of time figure out all the files included by a package?

By ahead of time I mean ahead of the point in which the decision to use pkgimages needs to be made?

@Roger-luo
Copy link
Contributor

I'm not fully certain if analyze include ahead of time would help. But I also thought about this before, since include is a function technically thus it is not possible to require it fully statical analyzable? E.g it's possible to include files inside a macro expansion or by calling a function dynamically. A real world example is how LiveServer evaluate Documenter's make.jl files in the same process.

This was part of reason why I have been advocating an explicit static syntax for local module/file loading (#4600). So one cannot call it from a function dynamically.

@vchuravy
Copy link
Member Author

vchuravy commented Jan 2, 2023

Would it be possible to ahead of time figure out all the files included by a package?

By ahead of time I mean ahead of the point in which the decision to use pkgimages needs to be made?

So you could look at the cache file, determine all the files it includes (which we do as a validity check) and reject the cachefile if it includes a path we care about, that wouldn't be to horrid

@IanButterworth
Copy link
Member

And you'd only need to do it if code_coverage == 3

@vchuravy
Copy link
Member Author

vchuravy commented Jan 2, 2023

The relevant code is here

for chi in includes

@IanButterworth
Copy link
Member

IanButterworth commented Nov 4, 2023

This has been shown to be a cause of +50s in every julia-runtest run because of Pkg being out of the sysimage, and pkgimages not used when coverage is on, causing subprocesses that use Pkg stuff to spend ages JIT compiling.

@IanButterworth IanButterworth added this to the 1.11 milestone Nov 4, 2023
@IanButterworth IanButterworth changed the title Code-coverage and package images Code-coverage should be more precise in disabling package images Nov 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment