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

avoid invalidations when the overlap is ambiguous #35877

Closed
wants to merge 3 commits into from
Closed

avoid invalidations when the overlap is ambiguous #35877

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c1a0bb2
avoid invalidations when the overlap is ambiguous
JeffBezanson May 13, 2020
eca8e50
Merge branch 'master' into jb/ambiguousinvalidate
timholy Jun 3, 2020
f65a8a2
Merge branch 'master' into jb/ambiguousinvalidate
timholy Jun 4, 2020
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
45 changes: 32 additions & 13 deletions src/gf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1723,19 +1723,34 @@ JL_DLLEXPORT void jl_method_table_disable(jl_methtable_t *mt, jl_method_t *metho
JL_UNLOCK(&mt->writelock);
}

static int is_call_ambiguous(jl_methtable_t *mt, jl_value_t *types JL_PROPAGATES_ROOT, size_t world)
{
struct jl_typemap_assoc search = {(jl_value_t*)types, world, NULL, 0, ~(size_t)0};
jl_typemap_entry_t *entry = jl_typemap_assoc_by_type(mt->defs, &search, /*offs*/0, /*subtype*/1);
if (entry == NULL)
return 0; // we added a new definition to consider
jl_typemap_entry_t *m = jl_typemap_morespecific_by_type(entry, (jl_value_t*)types, NULL, world);
if (m == NULL) {
// TODO: this isn't quite right, since we just ask if the entry dominates
// over all subtypes of `types`, but wanted to ask the inverse question
return 1; // a new ambiguity doesn't change the result of method lookup
}
return 0; // existing and/or method already covered this intersection (TODO: which is it?)
}

JL_DLLEXPORT void jl_method_table_insert(jl_methtable_t *mt, jl_method_t *method, jl_tupletype_t *simpletype)
{
JL_TIMING(ADD_METHOD);
assert(jl_is_method(method));
assert(jl_is_mtable(mt));
jl_value_t *type = method->sig;
jl_value_t *oldvalue = NULL;
jl_value_t *oldvalue = NULL, *isect = NULL;
if (method->primary_world == 1)
method->primary_world = ++jl_world_counter;
size_t max_world = method->primary_world - 1;
int invalidated = 0;
jl_value_t *loctag = NULL; // debug info for invalidation
JL_GC_PUSH2(&oldvalue, &loctag);
JL_GC_PUSH3(&oldvalue, &loctag, &isect);
JL_LOCK(&mt->writelock);
// first delete the existing entry (we'll disable it later)
struct jl_typemap_assoc search = {(jl_value_t*)type, method->primary_world, NULL, 0, ~(size_t)0};
Expand All @@ -1760,7 +1775,8 @@ JL_DLLEXPORT void jl_method_table_insert(jl_methtable_t *mt, jl_method_t *method
size_t ins = 0;
for (i = 1; i < na; i += 2) {
jl_value_t *backedgetyp = backedges[i - 1];
if (!jl_has_empty_intersection(backedgetyp, (jl_value_t*)type)) {
isect = jl_type_intersection(backedgetyp, (jl_value_t*)type);
if (isect != jl_bottom_type && !is_call_ambiguous(mt, isect, max_world)) {
jl_method_instance_t *backedge = (jl_method_instance_t*)backedges[i];
invalidate_method_instance(backedge, max_world, 0);
invalidated = 1;
Expand Down Expand Up @@ -1810,18 +1826,21 @@ JL_DLLEXPORT void jl_method_table_insert(jl_methtable_t *mt, jl_method_t *method
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 (mi != NULL && !jl_has_empty_intersection(type, (jl_value_t*)mi->specTypes))
if (invalidate_backedges(mi, max_world)) {
invalidated = 1;
if (_jl_debug_method_invalidation) {
jl_array_ptr_1d_push(_jl_debug_method_invalidation, (jl_value_t*)mi);
if (!loctag) {
loctag = jl_cstr_to_string("jl_method_table_insert");
jl_gc_wb(_jl_debug_method_invalidation, loctag);
if (mi != NULL) {
isect = jl_type_intersection(type, (jl_value_t*)mi->specTypes);
if (isect != jl_bottom_type && !is_call_ambiguous(mt, isect, max_world))
if (invalidate_backedges(mi, max_world)) {
invalidated = 1;
if (_jl_debug_method_invalidation) {
jl_array_ptr_1d_push(_jl_debug_method_invalidation, (jl_value_t*)mi);
if (!loctag) {
loctag = jl_cstr_to_string("jl_method_table_insert");
jl_gc_wb(_jl_debug_method_invalidation, loctag);
Copy link
Member

Choose a reason for hiding this comment

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

This jl_gc_wb doesn't do anything (there's no memory being modified). Why is it here?

Copy link
Member

Choose a reason for hiding this comment

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

My fault, came from resolving conflict with #35768 and I resolved it into here.

Copy link
Member

Choose a reason for hiding this comment

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

None of those do anything either. What are they supposed to do?

Copy link
Member

Choose a reason for hiding this comment

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

What are "those"? You mean all the jl_gc_wb in #35768? I thought from https://docs.julialang.org/en/latest/manual/embedding/#Updating-fields-of-GC-managed-objects-1 that I'd need to protect them but I guess that's not necessary for array elements, only fields? I can get rid of them.

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't work for that anyways

}
jl_array_ptr_1d_push(_jl_debug_method_invalidation, loctag);
}
jl_array_ptr_1d_push(_jl_debug_method_invalidation, loctag);
}
}
}
}
}
}
Expand Down