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

Fix package precompilation (PrecompileTools) #57828

Merged
merged 2 commits into from
Mar 20, 2025
Merged

Conversation

timholy
Copy link
Member

@timholy timholy commented Mar 19, 2025

With the "classic" inference timing disabled, PrecompileTools and SnoopCompile are non-functional on 1.12 & master. #57074 added timing data to the CodeInstances themselves, which should help restore SnoopCompile. However, without the tree structure provided by the old inference timing system, we still need a way to tag MethodInstances that get inferred inside PrecompileTools.@compile_workload blocks.

This adds a simple mechanism modeled after @trace_compile that switches to tagging MethodInstances in jl_push_newly_inferred. JuliaLang/PrecompileTools.jl#47 contains (or will contain) the corresponding changes to PrecompileTools.jl.

With the "classic" inference timing disabled, PrecompileTools and
SnoopCompile are non-functional on 1.12 & master. #57074 added timing
data to the CodeInstances themselves, which should help restore
SnoopCompile. However, without the tree structure provided by the old
inference timing system, we still need a way to tag MethodInstances that
get inferred inside `PrecompileTools.@compile_workload` blocks.

This adds a simple mechanism modeled after `@trace_compile` that
switching to tagging MethodInstances in `jl_push_newly_inferred`.
JuliaLang/PrecompileTools.jl#47 contains the
corresponding changes to PrecompileTools.jl.
@vtjnash vtjnash added the backport 1.12 Change should be backported to release-1.12 label Mar 19, 2025
@IanButterworth
Copy link
Member

Will Pkg's internalized @compile_workload need updating?

https://github.com/JuliaLang/Pkg.jl/blob/master/src/precompile.jl#L217-L244

@timholy
Copy link
Member Author

timholy commented Mar 19, 2025

Yes, I'll push a very simple test in the next commit. (PrecompileTools is a test of sorts, but good to have one here too.)

@timholy timholy added the merge me PR is reviewed. Merge when all tests are passing label Mar 19, 2025
@timholy
Copy link
Member Author

timholy commented Mar 19, 2025

Corresponding changes in JuliaLang/PrecompileTools.jl#47 are now up too

@IanButterworth
Copy link
Member

I just noticed that #54899 deleted the corresponding internalized @compile_workload code from REPL. So that will need reinstating also.

Shall we make a barebones version for Base so that we're not duplicating code between Pkg and REPL and any other stdlibs that want to benefit?

@vtjnash
Copy link
Member

vtjnash commented Mar 19, 2025

I deleted it there because I noticed it seemed to be making the quality of the REPL script worse not better

@KristofferC KristofferC mentioned this pull request Mar 20, 2025
87 tasks
@JeffBezanson JeffBezanson merged commit c89b1ff into master Mar 20, 2025
7 of 8 checks passed
@JeffBezanson JeffBezanson deleted the teh/pctagging branch March 20, 2025 21:01
KristofferC pushed a commit that referenced this pull request Mar 20, 2025
With the "classic" inference timing disabled, PrecompileTools and
SnoopCompile are non-functional on 1.12 & master. #57074 added timing
data to the CodeInstances themselves, which should help restore
SnoopCompile. However, without the tree structure provided by the old
inference timing system, we still need a way to tag MethodInstances that
get inferred inside `PrecompileTools.@compile_workload` blocks.

This adds a simple mechanism modeled after `@trace_compile` that
switches to tagging MethodInstances in `jl_push_newly_inferred`.
JuliaLang/PrecompileTools.jl#47 contains (or
will contain) the corresponding changes to PrecompileTools.jl.

(cherry picked from commit c89b1ff)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.12 Change should be backported to release-1.12 merge me PR is reviewed. Merge when all tests are passing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants