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

Optionally disallow defining new methods and drop backedges #198

Merged
merged 5 commits into from
Nov 21, 2024
Merged

Optionally disallow defining new methods and drop backedges #198

Show file tree
Hide file tree
Changes from all 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 Nov 14, 2024
a79cee8
Make `allow_new_worlds` atomic
Drvi Nov 14, 2024
917f872
Fixup: also respect `allow_new_world` in `jl_method_table_add_backedge`
Drvi Nov 14, 2024
2ab1309
make `allow_new_worlds` a `bool`
Drvi Nov 20, 2024
775d60e
.
Drvi Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions src/gf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
. static parameter inference
. method specialization and caching, invoking type inference
*/
#include <stdbool.h>
#include <stdlib.h>
#include <string.h>
#include "julia.h"
Expand All @@ -24,6 +25,7 @@
extern "C" {
#endif

static _Atomic(bool) allow_new_worlds = true;
JL_DLLEXPORT _Atomic(size_t) jl_world_counter = 1; // uses atomic acquire/release
JL_DLLEXPORT size_t jl_get_world_counter(void) JL_NOTSAFEPOINT
{
Expand Down Expand Up @@ -1718,6 +1720,8 @@ static void invalidate_backedges(void (*f)(jl_code_instance_t*), jl_method_insta
// add a backedge from callee to caller
JL_DLLEXPORT void jl_method_instance_add_backedge(jl_method_instance_t *callee, jl_value_t *invokesig, jl_method_instance_t *caller)
{
if (!jl_atomic_load_acquire(&allow_new_worlds))
return;
JL_LOCK(&callee->def.method->writelock);
if (invokesig == jl_nothing)
invokesig = NULL; // julia uses `nothing` but C uses NULL (#undef)
Expand Down Expand Up @@ -1753,6 +1757,8 @@ JL_DLLEXPORT void jl_method_instance_add_backedge(jl_method_instance_t *callee,
// add a backedge from a non-existent signature to caller
JL_DLLEXPORT void jl_method_table_add_backedge(jl_methtable_t *mt, jl_value_t *typ, jl_value_t *caller)
{
if (!jl_atomic_load_acquire(&allow_new_worlds))
return;
JL_LOCK(&mt->writelock);
if (!mt->backedges) {
// lazy-init the backedges array
Expand Down Expand Up @@ -1915,8 +1921,48 @@ static void jl_method_table_invalidate(jl_methtable_t *mt, jl_typemap_entry_t *m
}
}

static int erase_method_backedges(jl_typemap_entry_t *def, void *closure)
{
jl_method_t *method = def->func.method;
jl_value_t *specializations = jl_atomic_load_relaxed(&method->specializations);
if (jl_is_svec(specializations)) {
size_t i, l = jl_svec_len(specializations);
for (i = 0; i < l; i++) {
jl_method_instance_t *mi = (jl_method_instance_t*)jl_svecref(specializations, i);
if ((jl_value_t*)mi != jl_nothing) {
mi->backedges = NULL;
}
}
}
else {
jl_method_instance_t *mi = (jl_method_instance_t*)specializations;
mi->backedges = NULL;
}
return 1;
}

static int erase_all_backedges(jl_methtable_t *mt, void *env)
{
// removes all method caches
// this might not be entirely safe (GC or MT), thus we only do it very early in bootstrapping
mt->backedges = NULL;
jl_typemap_visitor(jl_atomic_load_relaxed(&mt->defs), erase_method_backedges, env);
return 1;
}
Comment on lines +1924 to +1951
Copy link
Member

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?

Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

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

backedges are jl_vector_ts 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 this

Copy link
Member

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?

Copy link
Member

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?

Copy link
Member

@adnan-alhomssi adnan-alhomssi Nov 21, 2024 •
edited
Loading

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); ?

Copy link
Member

Choose a reason for hiding this comment

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

From documentation,

If the object being stored is a `jl_value_t`, the Julia garbage collector must be notified also:

It is NULL so it looks fine the way it currently is.

Copy link
Member

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.

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.

It is NULL so it looks fine the way it currently is.

Yes, you don't need a write barrier here because you are storing NULL.


JL_DLLEXPORT void jl_disable_new_worlds(void)
{
if (jl_generating_output())
jl_error("Disabling Method changes is not possible when generating output.");
jl_atomic_store_release(&allow_new_worlds, false);
jl_foreach_reachable_mtable(erase_all_backedges, (void*)NULL);
}


JL_DLLEXPORT void jl_method_table_disable(jl_methtable_t *mt, jl_method_t *method)
{
if (!jl_atomic_load_acquire(&allow_new_worlds))
jl_error("Method changes have been disabled via a call to jl_disable_new_worlds.");
jl_typemap_entry_t *methodentry = do_typemap_search(mt, method);
JL_LOCK(&mt->writelock);
// Narrow the world age on the method to make it uncallable
Expand Down Expand Up @@ -1986,6 +2032,8 @@ static int is_replacing(char ambig, jl_value_t *type, jl_method_t *m, jl_method_

JL_DLLEXPORT void jl_method_table_insert(jl_methtable_t *mt, jl_method_t *method, jl_tupletype_t *simpletype)
{
if (!jl_atomic_load_acquire(&allow_new_worlds))
jl_error("Method changes have been disabled via a call to jl_disable_new_worlds.");
JL_TIMING(ADD_METHOD, ADD_METHOD);
assert(jl_is_method(method));
assert(jl_is_mtable(mt));
Expand Down
4 changes: 2 additions & 2 deletions src/staticdata.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

what difference does this move make?

Copy link
Member Author

Choose a reason for hiding this comment

The 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)
Expand Down