Skip to content

aot: move jl_insert_backedges to Julia side #56499

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
Merged
merged 3 commits into from
Jan 15, 2025
Merged

aot: move jl_insert_backedges to Julia side #56499

merged 3 commits into from
Jan 15, 2025

Conversation

@aviatesk
Copy link
Member

@aviatesk aviatesk commented Nov 8, 2024

With #56447, the dependency between jl_insert_backedges and method insertion has been eliminated, allowing jl_insert_backedges to be performed after loading. As a result, it is now possible to move jl_insert_backedges to the Julia side.

Currently this commit simply moves the implementation without adding any new features.

@aviatesk aviatesk requested a review from vtjnash November 8, 2024 08:47
@aviatesk aviatesk force-pushed the avi/move-staticdata_utils-julia branch from 4959411 to 2eafa61 Compare November 8, 2024 10:49
@gbaraldi
Copy link
Member

gbaraldi commented Nov 8, 2024

Does this have any performance diff?

@aviatesk
Copy link
Member Author

aviatesk commented Nov 8, 2024 •
edited
Loading

Moving the implementation to Julia seems to result in a slight increase in allocations, but considering the potential for future improvements, it doesn’t seem like a significant issue

julia> @time using Debugger
  0.233097 seconds (626.92 k allocations: 33.375 MiB, 26.87% compilation time: 100% of which was recompilation) # master
  0.228869 seconds (627.05 k allocations: 33.421 MiB, 27.19% compilation time: 100% of which was recompilation)

@aviatesk aviatesk force-pushed the avi/move-staticdata_utils-julia branch from 2eafa61 to 718aea8 Compare November 9, 2024 11:02
@aviatesk aviatesk force-pushed the avi/move-staticdata_utils-julia branch 4 times, most recently from 907d058 to 6b69260 Compare November 22, 2024 08:56
@aviatesk aviatesk force-pushed the avi/move-staticdata_utils-julia branch 2 times, most recently from 7b54cf5 to 87836bf Compare January 6, 2025 17:37
vtjnash
vtjnash reviewed Jan 6, 2025
View reviewed changes
vtjnash
vtjnash reviewed Jan 6, 2025
View reviewed changes
vtjnash
vtjnash reviewed Jan 6, 2025
View reviewed changes
vtjnash
vtjnash reviewed Jan 6, 2025
View reviewed changes
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

LGTM

@aviatesk aviatesk force-pushed the avi/move-staticdata_utils-julia branch from 109748f to 929d53c Compare January 7, 2025 06:48
aviatesk added a commit that referenced this pull request Jan 8, 2025
@aviatesk
staticdata: remove reinit_ccallable
cf6b5ac 
If #56499 is merged, the functionality of
`jl_reinit_ccallable` will become incomplete. However, it is
questionable whether `jl_reinit_ccallable` is truly necessary.
It seems to have been added in #44527, but all test cases
appear to pass without it. Therefore, it might be safe to remove it
altogether.
@aviatesk aviatesk mentioned this pull request Jan 8, 2025
Merged
aviatesk added a commit that referenced this pull request Jan 8, 2025
@aviatesk
staticdata: remove reinit_ccallable
400292a 
If #56499 is merged, the functionality of
`jl_reinit_ccallable` will become incomplete. However, it is
questionable whether `jl_reinit_ccallable` is truly necessary.
It seems to have been added in #44527, but all test cases
appear to pass without it. Therefore, it might be safe to remove it
altogether.
@aviatesk aviatesk force-pushed the avi/move-staticdata_utils-julia branch from 929d53c to 8ed7f7f Compare January 8, 2025 06:49
Keno added a commit that referenced this pull request Jan 9, 2025
@Keno
Add edge kind for access to non-explicit partitioned bindings
1254f33 
Our binding partion invalidation code scans the original source
for any GlobalRefs that need to be invalidated. However, this
is not the only source of access to binding partitions. Various
intrinsics (in particular the `*global` ones) also operate on
bindings. Since these are not manifest in the source, we instead
use the existing `edges` mechanism to give them forward edges.

This PR only includes the basic info type, and validation in the
replacement case. There's a bit more work to do there, but I'm
waiting on #56499 for that part, precompilation in particular.
@Keno Keno mentioned this pull request Jan 9, 2025
Merged
Keno added a commit that referenced this pull request Jan 9, 2025
@Keno
Add edge kind for access to non-explicit partitioned bindings
42080a8 
Our binding partion invalidation code scans the original source
for any GlobalRefs that need to be invalidated. However, this
is not the only source of access to binding partitions. Various
intrinsics (in particular the `*global` ones) also operate on
bindings. Since these are not manifest in the source, we instead
use the existing `edges` mechanism to give them forward edges.

This PR only includes the basic info type, and validation in the
replacement case. There's a bit more work to do there, but I'm
waiting on #56499 for that part, precompilation in particular.
Keno added a commit that referenced this pull request Jan 10, 2025
@Keno
Add edge kind for access to non-explicit partitioned bindings
6448ad8 
Our binding partion invalidation code scans the original source
for any GlobalRefs that need to be invalidated. However, this
is not the only source of access to binding partitions. Various
intrinsics (in particular the `*global` ones) also operate on
bindings. Since these are not manifest in the source, we instead
use the existing `edges` mechanism to give them forward edges.

This PR only includes the basic info type, and validation in the
replacement case. There's a bit more work to do there, but I'm
waiting on #56499 for that part, precompilation in particular.
Keno added a commit that referenced this pull request Jan 10, 2025
@Keno
Add edge kind for access to non-explicit partitioned bindings (#57009)
9211a93 
Our binding partion invalidation code scans the original source for any
GlobalRefs that need to be invalidated. However, this is not the only
source of access to binding partitions. Various intrinsics (in
particular the `*global` ones) also operate on bindings. Since these are
not manifest in the source, we instead use the existing `edges`
mechanism to give them forward edges.

This PR only includes the basic info type, and validation in the
replacement case. There's a bit more work to do there, but I'm waiting
on #56499 for that part, precompilation in particular.
@aviatesk aviatesk force-pushed the avi/move-staticdata_utils-julia branch 2 times, most recently from e1c3ce6 to 9340251 Compare January 13, 2025 10:51
@aviatesk aviatesk changed the base branch from master to avi/rm-reinit_ccallable January 13, 2025 14:16
@aviatesk aviatesk force-pushed the avi/move-staticdata_utils-julia branch from 9340251 to b5d0227 Compare January 13, 2025 14:30
@aviatesk
Copy link
Member Author

This should be ready to go.

@aviatesk aviatesk force-pushed the avi/move-staticdata_utils-julia branch from b5d0227 to d9d83a6 Compare January 14, 2025 06:33
@aviatesk aviatesk changed the base branch from avi/rm-reinit_ccallable to master January 14, 2025 06:33
@aviatesk
Copy link
Member Author

This PR is currently depending on and thus blocked by #56987. However, I believe it is not strictly necessary to merge #56987 before this PR. For the sake of the ongoing development of partitioned bindings, I have made this PR standalone and ready for quick merging.

@aviatesk aviatesk added the merge me PR is reviewed. Merge when all tests are passing label Jan 14, 2025
@aviatesk aviatesk force-pushed the avi/move-staticdata_utils-julia branch 2 times, most recently from 0b2575d to 2e7b0b2 Compare January 14, 2025 10:53
vtjnash
vtjnash reviewed Jan 15, 2025
View reviewed changes
vtjnash
vtjnash reviewed Jan 15, 2025
View reviewed changes
vtjnash
vtjnash reviewed Jan 15, 2025
View reviewed changes
base/staticdata.jl
@@ -0,0 +1,296 @@
# This file is a part of Julia. License is MIT: https://julialang.org/license

module StaticData
Copy link
Member

Choose a reason for hiding this comment

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

This eventually should be moved inside the Compiler module and locked to the Compiler world age, since broadly speaking this algorithm is just a slightly different re-implementation of that entire code. I guess that isn't blocking though for now, since this is a private module name

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's move it to Compiler in a separate PR.

aviatesk and others added 3 commits January 15, 2025 13:45
@aviatesk
aot: move jl_insert_backedges to Julia side
204356e 
With #56447, the dependency between `jl_insert_backedges`
and method insertion has been eliminated, allowing `jl_insert_backedges`
to be performed after loading. As a result, it is now possible to move
`jl_insert_backedges` to the Julia side.

Currently this commit simply moves the implementation without adding
any new features.
@aviatesk @vtjnash
Update base/staticdata.jl
5295c83 
Co-authored-by: Jameson Nash <[email protected]>
@aviatesk
@atomic :monotonic stores
6e8bd6b
@aviatesk aviatesk force-pushed the avi/move-staticdata_utils-julia branch from d427164 to 6e8bd6b Compare January 15, 2025 04:47
@aviatesk aviatesk merged commit ef04055 into master Jan 15, 2025
5 of 7 checks passed
@aviatesk aviatesk deleted the avi/move-staticdata_utils-julia branch January 15, 2025 09:13
@inkydragon inkydragon removed the merge me PR is reviewed. Merge when all tests are passing label Jan 15, 2025
@vtjnash vtjnash mentioned this pull request Feb 18, 2025
Closed
vtjnash added a commit that referenced this pull request Mar 14, 2025
@vtjnash
loading: fix ccall signature in _include_from_serialized
5c1b4fe 
This still sort of errored correctly anyways (after breakage added from
PR #56499), but that behavior should not be relied upon optimizations
not breaking it when this is providing incorrect info to the compiler.

Fix #57459
@vtjnash vtjnash mentioned this pull request Mar 14, 2025
Merged
topolarity pushed a commit that referenced this pull request Mar 15, 2025
@vtjnash
loading: fix ccall signature in _include_from_serialized (#57776)
a5abb6f 
This still sort of errored correctly anyways (after breakage added from
PR #56499), but that behavior should not be relied upon optimizations
not breaking it when this is providing incorrect info to the compiler.

Fix #57459
serenity4 pushed a commit to serenity4/julia that referenced this pull request May 1, 2025
@Keno
Add edge kind for access to non-explicit partitioned bindings (JuliaL…
92998f2 
…ang#57009)

Our binding partion invalidation code scans the original source for any
GlobalRefs that need to be invalidated. However, this is not the only
source of access to binding partitions. Various intrinsics (in
particular the `*global` ones) also operate on bindings. Since these are
not manifest in the source, we instead use the existing `edges`
mechanism to give them forward edges.

This PR only includes the basic info type, and validation in the
replacement case. There's a bit more work to do there, but I'm waiting
on JuliaLang#56499 for that part, precompilation in particular.
serenity4 pushed a commit to serenity4/julia that referenced this pull request May 1, 2025
@aviatesk @vtjnash
aot: move jl_insert_backedges to Julia side (JuliaLang#56499)
3e14e22 
With JuliaLang#56447, the dependency between `jl_insert_backedges`
and method insertion has been eliminated, allowing `jl_insert_backedges`
to be performed after loading. As a result, it is now possible to move
`jl_insert_backedges` to the Julia side.

Currently this commit simply moves the implementation without adding any
new features.

---------

Co-authored-by: Jameson Nash <[email protected]>
@vchuravy vchuravy mentioned this pull request Aug 18, 2025
Closed
KristofferC pushed a commit that referenced this pull request Aug 19, 2025
@vtjnash @KristofferC
loading: fix ccall signature in _include_from_serialized (#57776)
3b8a100 
This still sort of errored correctly anyways (after breakage added from
PR #56499), but that behavior should not be relied upon optimizations
not breaking it when this is providing incorrect info to the compiler.

Fix #57459

(cherry picked from commit a5abb6f)
KristofferC pushed a commit that referenced this pull request Sep 2, 2025
@vtjnash @KristofferC
loading: fix ccall signature in _include_from_serialized (#57776)
be5bc79 
This still sort of errored correctly anyways (after breakage added from
PR #56499), but that behavior should not be relied upon optimizations
not breaking it when this is providing incorrect info to the compiler.

Fix #57459

(cherry picked from commit a5abb6f)
KristofferC pushed a commit that referenced this pull request Sep 15, 2025
@vtjnash
loading: fix ccall signature in _include_from_serialized (#57776)
8dbbfa5 
This still sort of errored correctly anyways (after breakage added from
PR #56499), but that behavior should not be relied upon optimizations
not breaking it when this is providing incorrect info to the compiler.

Fix #57459

(cherry picked from commit a5abb6f)
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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@aviatesk @gbaraldi @vtjnash @inkydragon