Skip to content

Conversation

topolarity
Copy link
Member

@topolarity topolarity commented Mar 19, 2025

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 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 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

@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 topolarity removed the needs tests Unit tests are required for this change label Mar 20, 2025
@nsajko
Copy link
Member

nsajko commented Mar 20, 2025

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

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

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)

@JeffBezanson
Copy link
Member

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

@KristofferC
Copy link
Member

KristofferC 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)

They get used at runtime in scripts (notebooks etc) and by other packages during precompile time. I don't see why it doesn't make sense to compile e.g. the JuMP macros once instead of adding latency to every single script a user wants to run with it.

@nsajko
Copy link
Member

nsajko commented Mar 21, 2025

Would it make sense to resolve the tradeoff by:

  • doing the inference
  • preserving the results while the Julia process is running
  • not preserving the results in the precompile files?

@KristofferC KristofferC mentioned this pull request Mar 24, 2025
27 tasks
topolarity added a commit to topolarity/julia that referenced this pull request Mar 25, 2025
There's not much purpose of this de-specialization, since macros are
generally excluded from inference / compilation anyway (except for
JuliaLang#57833 which was a bug)

Since our AST is already relatively homogeneous, it's not clear that
we gained much by this de-specialization when it was used. Arbitrarily
de-specializing a function also often leads to bad (wide) edges in
inference etc., causing frequent invalidations.
topolarity added a commit to topolarity/julia that referenced this pull request Mar 25, 2025
There's not much purpose of this de-specialization, since macros are
generally excluded from inference / compilation anyway (except for
JuliaLang#57833 which was a bug)

Since our AST is already relatively homogeneous, it's not clear that
we gained much by this de-specialization when it was used. Arbitrarily
de-specializing a function also often leads to bad (wide) edges in
inference etc., causing frequent invalidations.
topolarity added a commit to topolarity/julia that referenced this pull request Mar 25, 2025
There's not much purpose of this de-specialization, since macros are
generally excluded from inference / compilation anyway (except for
JuliaLang#57833 which was a bug)

Since our AST is already relatively homogeneous, it's not clear that
we gained much by this de-specialization when it was used. Arbitrarily
de-specializing a function also often leads to bad (wide) edges in
inference etc., causing frequent invalidations.
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 force-pushed the ct/no-hint-macro-methods branch from 511502c to 1a91678 Compare March 26, 2025 14:42
@topolarity topolarity merged commit 6ce51d3 into JuliaLang:master Mar 26, 2025
7 checks passed
nsajko added a commit to nsajko/julia that referenced this pull request Mar 27, 2025
KristofferC pushed a commit that referenced this pull request Mar 31, 2025
…ion (#57833)

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

(cherry picked from commit 6ce51d3)
This was referenced Mar 31, 2025
KristofferC pushed a commit that referenced this pull request Mar 31, 2025
…ion (#57833)

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

(cherry picked from commit 6ce51d3)
KristofferC pushed a commit that referenced this pull request Mar 31, 2025
…ion (#57833)

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

(cherry picked from commit 6ce51d3)
topolarity added a commit that referenced this pull request Apr 1, 2025
…ion (#57833)

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 topolarity removed backport 1.11 Change should be backported to release-1.11 backport 1.12 Change should be backported to release-1.12 labels Apr 1, 2025
topolarity added a commit that referenced this pull request Apr 1, 2025
…ion (#57833)

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 topolarity removed the backport 1.10 Change should be backported to the 1.10 release label Apr 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix This change fixes an existing bug invalidations

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants