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.

Sign up for GitHub

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

Jump to bottom

precompile_utils: Don't auto-enqueue macro methods for pre-compilation #57833

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

precompile_utils: Don't auto-enqueue macro methods for pre-compilation #57833

wants to merge 2 commits into from
+9 −0

Conversation

topolarity
Copy link
Member

@topolarity topolarity commented Mar 19, 2025 •
edited
Loading

Despite disabling these from being compiled in gf.c for dynamic invocations, the pre-compilation code was adding macro Methods anyway to the workqueue.

Replaces #57782

@topolarity
precompile_utils: Don't enqueue macro methods for pre-compilation
7d4caa1 
Despite disabling the runtime from compiling `macro` functions in
`gf.c`, the pre-compilation code was adding macro code anyway to
the workqueue.
@topolarity topolarity added needs tests Unit tests are required for this change bugfix This change fixes an existing bug backport 1.10 Change should be backported to the 1.10 release backport 1.11 Change should be backported to release-1.11 backport 1.12 Change should be backported to release-1.12 labels Mar 19, 2025
@topolarity topolarity requested a review from JeffBezanson March 19, 2025 22:29
@topolarity topolarity mentioned this pull request Mar 19, 2025
Open
@topolarity topolarity changed the title precompile_utils: Don't enqueue macro methods for pre-compilation precompile_utils: Don't auto-enqueue macro methods for pre-compilation Mar 19, 2025
@topolarity
Copy link
Member Author

topolarity commented Mar 19, 2025 •
edited
Loading

@JeffBezanson What do you think about deleting the auto-nospecialize for macros in lowering?

Does it serve any purpose if we are generally not compiling this code anyways?

@topolarity
Add basic test
b7b66e6 
This behavior only affects the sysimage, not pkgimages so to avoid
creating a standalone sysimage test just for this phenomenon, just
verify that we don't have unexpected `macro` code cached in the
sysimage.
@topolarity topolarity removed the needs tests Unit tests are required for this change label Mar 20, 2025
@nsajko
Copy link
Contributor

nsajko commented Mar 20, 2025 •
edited
Loading

Decreases the invalidation count on running the following code from 214 to 163 🚀

struct I <: Integer end
Base.Int(::I) = 7
Base.UInt(::I) = 0x7

Data on each invalidation:

@KristofferC
Copy link
Member

KristofferC commented Mar 20, 2025 •
edited
Loading

Does these mean macro methods don't get precompiled at all? Isn't that desirable to avoid having to precompile them at every first use?

@JeffBezanson
Copy link
Member

JeffBezanson commented Mar 20, 2025 •
edited
Loading

Yes let's try removing the inserted nospecialize as an alternative to this and see how it does. If that doesn't help, we should find a way to implement the @ name policy in a single place so we are not playing whack-a-mole. For example, if the policy we want is not to infer them, that should maybe be implemented inside inference so we can still do the rest of precompilation for them.

@vtjnash
Copy link
Member

vtjnash commented Mar 20, 2025

macros do not get used at runtime, so including them in the precompile files is (mostly) a waste of space (as is specializing them)

vtjnash
vtjnash reviewed Mar 20, 2025
View reviewed changes
src/precompile_utils.c
@@ -170,6 +170,10 @@ static void jl_compile_all_defs(jl_array_t *mis, int all)
size_t i, l = jl_array_nrows(allmeths);
for (i = 0; i < l; i++) {
jl_method_t *m = (jl_method_t*)jl_array_ptr_ref(allmeths, i);
int is_macro_method = jl_symbol_name(m->name)[0] == '@';
if (is_macro_method)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (is_macro_method)
if (is_macro_method && !all)

@KristofferC KristofferC mentioned this pull request Mar 20, 2025
Open
87 tasks
@JeffBezanson
Copy link
Member

OK, it's certainly possible that no noticeable latency improvements come from precompiling macros.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vtjnash vtjnash vtjnash left review comments

@JeffBezanson JeffBezanson Awaiting requested review from JeffBezanson

Assignees
No one assigned
Labels
backport 1.10 Change should be backported to the 1.10 release backport 1.11 Change should be backported to release-1.11 backport 1.12 Change should be backported to release-1.12 bugfix This change fixes an existing bug invalidations
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@topolarity @nsajko @KristofferC @JeffBezanson @vtjnash