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

add Base.isprecompiled(pkg::PkgId) #50218

Merged
merged 3 commits into from
Jun 29, 2023

Conversation

IanButterworth
Copy link
Sponsor Member

@IanButterworth IanButterworth commented Jun 19, 2023

This is based on the Pkg internal _is_stale but inverted, with caching optional via kwargs so that Pkg can instead use this and remain efficient.

It's more generally useful, and good to keep in-line with Base functionality.

julia> @time Base.isprecompiled(Base.PkgId(Revise))
  0.006545 seconds (7.82 k allocations: 392.570 KiB)
true

julia> @time Base.isprecompiled(Base.PkgId(Revise))
  0.005438 seconds (7.82 k allocations: 392.570 KiB)
true

shell> rm -rf ~/.julia/compiled/v1.10/Revise

julia> @time Base.isprecompiled(Base.PkgId(Revise))
  0.000313 seconds (275 allocations: 17.773 KiB)
false

julia> @time Base.isprecompiled(Base.PkgId(Revise))
  0.000287 seconds (275 allocations: 17.773 KiB)
false

@IanButterworth
Copy link
Sponsor Member Author

Bump @KristofferC

@KristofferC
Copy link
Sponsor Member

I would have thought this function would also be used by loading to check if a precompile file is stale before trying to precompile it. Is there a reason that couldn't be done? Perhaps it needs to return a bit more than just true/false then?

@IanButterworth
Copy link
Sponsor Member Author

I gave that a go twice and failed twice because the staledeps caching and conversion from Any is quite different in the Pkg and Base usage patterns. You may have a clearer vision on the loading system to come up with a simple approach.

At least, it will need to be broken up into an internal function that can be used inside the loading loop, I think.

This is blocking making Pkg.precompile use the pidlocks. Perhaps that could be a follow on PR?

Comment on lines +1407 to +1419
try
# update timestamp of precompilation file so that it is the first to be tried by code loading
touch(path_to_try)
catch ex
# file might be read-only and then we fail to update timestamp, which is fine
ex isa IOError || rethrow()
end
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

being simultaneously mutating (rather than a pure check) seems out-of-character for this function

Suggested change
try
# update timestamp of precompilation file so that it is the first to be tried by code loading
touch(path_to_try)
catch ex
# file might be read-only and then we fail to update timestamp, which is fine
ex isa IOError || rethrow()
end

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it seems like a reasonable thing to do for systemic efficiency

base/loading.jl Outdated Show resolved Hide resolved
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Jun 29, 2023
@DilumAluthge DilumAluthge requested review from KristofferC and removed request for KristofferC June 29, 2023 03:00
@IanButterworth IanButterworth merged commit f6f3553 into JuliaLang:master Jun 29, 2023
2 checks passed
@IanButterworth IanButterworth deleted the ib/isprecompiled branch June 29, 2023 17:15
@IanButterworth IanButterworth removed the merge me PR is reviewed. Merge when all tests are passing label Jul 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants