forked from JuliaLang/julia
-
Notifications
You must be signed in to change notification settings - Fork 0
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
Optionally disallow defining new methods and drop backedges #198
Merged
Merged
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
4e03fea
Optionally disallow defining new methods and drop backedges
Drvi a79cee8
Make `allow_new_worlds` atomic
Drvi 917f872
Fixup: also respect `allow_new_world` in `jl_method_table_add_backedge`
Drvi 2ab1309
make `allow_new_worlds` a `bool`
Drvi 775d60e
.
Drvi File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3447,11 +3447,11 @@ static jl_value_t *jl_restore_package_image_from_stream(void* pkgimage_handle, i | |
jl_restore_system_image_from_stream_(f, image, depmods, checksum, (jl_array_t**)&restored, &init_order, &extext_methods, &new_specializations, &method_roots_list, &ext_targets, &edges, &base, &ccallable_list, &cachesizes); | ||
JL_SIGATOMIC_END(); | ||
|
||
// Insert method extensions | ||
jl_insert_methods(extext_methods); | ||
// No special processing of `new_specializations` is required because recaching handled it | ||
// Add roots to methods | ||
jl_copy_roots(method_roots_list, jl_worklist_key((jl_array_t*)restored)); | ||
// Insert method extensions | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what difference does this move make? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know, but it was in the diff we were given, so I included it. |
||
jl_insert_methods(extext_methods); | ||
// Handle edges | ||
size_t world = jl_atomic_load_acquire(&jl_world_counter); | ||
jl_insert_backedges((jl_array_t*)edges, (jl_array_t*)ext_targets, (jl_array_t*)new_specializations, world); // restore external backedges (needs to be last) | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting that we just NULL the backedges field, and don't erase/free anything.
Are these GC-managed objects that we were pointing to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I am also curious to know how are these objects managed and freed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
backedges
arejl_vector_t
s which are managed by the GC unless they are explicitly constructed to opt-out (owned=0), IIUC. Them being GC tracked is relied on even elsewhere e.g.invalidate_method_instance
also does thisThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe @d-netto can weigh in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Drvi I think here it becomes clear how GC is made ware of it (notice
jl_gc_wb
).Let's merge it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or, do we need
jl_gc_wb(mt, mt->backedges);
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From documentation,
It is NULL so it looks fine the way it currently is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be fine. If the reference we erased happened to be the last one pointing to the given object, then the mark phase will detect that this object is unreachable and free it during sweeping.
Yes, you don't need a write barrier here because you are storing
NULL
.