avoid invalidations when the overlap is ambiguous #35877
Conversation
src/gf.c
Outdated
| for (size_t i = 0; i < jl_array_len(m->ambig); i++) { | ||
| jl_typemap_entry_t *mambig = (jl_typemap_entry_t*)jl_array_ptr_ref(m->ambig, i); | ||
| if (mambig->min_world <= jl_world_counter && jl_world_counter <= mambig->max_world) | ||
| if (jl_subtype((jl_value_t*)types, (jl_value_t*)mambig->sig)) |
There was a problem hiding this comment.
I'm not at all certain, but do you have to wait until this condition has been satisfied more than once?
|
IIUC, the failing test (in core) is indeed the counterfactual to this idea, unfortunately :/ |
src/gf.c
Outdated
| jl_methtable_t *mt = jl_first_argument_datatype(types)->name->mt; | ||
| struct jl_typemap_assoc search = {types, world, NULL, 0, ~(size_t)0}; | ||
| jl_typemap_entry_t *entry = jl_typemap_assoc_by_type(mt->defs, &search, 0, 1); | ||
| if (!entry) | ||
| return 0; | ||
| return jl_is_call_ambiguous(types, entry->func.method); |
There was a problem hiding this comment.
It seems like this code should be similar to jl_gf_invoke_lookup, but with one of the return values swapped:
| jl_methtable_t *mt = jl_first_argument_datatype(types)->name->mt; | |
| struct jl_typemap_assoc search = {types, world, NULL, 0, ~(size_t)0}; | |
| jl_typemap_entry_t *entry = jl_typemap_assoc_by_type(mt->defs, &search, 0, 1); | |
| if (!entry) | |
| return 0; | |
| return jl_is_call_ambiguous(types, entry->func.method); | |
| jl_methtable_t *mt = jl_method_table_for(types); // TODO: this should probably just be an argument to this function | |
| assert((jl_value_t*)mt != jl_nothing); | |
| 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?) |
Though I think the ml_matches_visitor has the more complete and correct version.
In these cases, the new method would not be called because the call would be an ambiguity error instead.
JeffBezanson
force-pushed
the
jb/ambiguousinvalidate
branch
from
f0e17a6
to
c1a0bb2
Compare
|
Ok, this should hopefully work now. It passes the ambiguity-related tests in #35855. |
JeffBezanson
changed the title
|
This helps a lot with FixedPointNumbers! It also fixes 2/ Being greedy, I should ask whether there anything that can be done about this? julia> using SnoopCompile
julia> abstract type SomeType end
julia> _invs = @snoopr Base.convert(::Type{T}, x) where T<:SomeType = nothing;
julia> length(unique(filter(x->isa(x, Core.MethodInstance), _invs)))
797For example, intersection julia> which(convert, (Type{T} where T<:Symbol,Any))
ERROR: no unique matching method found for the specified argument types
Stacktrace:
[1] error(::String) at ./error.jl:33
[2] which(::Any, ::Any) at ./reflection.jl:1138
[3] top-level scope at REPL[9]:1
julia> methods(convert, (Type{T} where T<:Symbol,Any))
# 3 methods for generic function "convert":
[1] convert(::Type{Union{}}, x) in Base at essentials.jl:169
[2] convert(::Type{T}, x::T) where T in Base at essentials.jl:171
[3] convert(::Type{T}, arg) where T<:VecElement in Base at baseext.jl:8This particular invalidation is not consequential (that julia> which(convert, (Type{T} where T<:Tuple,Tuple{}))
ERROR: no unique matching method found for the specified argument types
Stacktrace:
[1] error(::String) at ./error.jl:33
[2] which(::Any, ::Any) at ./reflection.jl:1138
[3] top-level scope at REPL[2]:1
julia> methods(convert, (Type{T} where T<:Tuple,Tuple{}))
# 6 methods for generic function "convert":
[1] convert(::Type{Tuple{}}, ::Tuple{}) in Base at essentials.jl:305
[2] convert(::Type{Union{}}, x) in Base at essentials.jl:169
[3] convert(::Type{Tuple{Vararg{V,N} where N}}, x::Tuple{Vararg{V,N} where N}) where V in Base at essentials.jl:314
[4] convert(T::Type{Tuple{Vararg{V,N} where N}}, x::Tuple) where V in Base at essentials.jl:315
[5] convert(::Type{T}, x::T) where T in Base at essentials.jl:171
[6] convert(::Type{T}, arg) where T<:VecElement in Base at baseext.jl:8I added a concrete test case for this in #35855. |
|
I fixed conflicts from #35768. This PR doesn't have tests, but they are coming in #35855, which I've slimmed down to currently-failing tests. Should we merge that and then this? Vice versa? The issue I point out in #35877 (comment) turns out to be mostly due to this method. I ran some tests locally that suggest we can delete it without needing other changes. So it's not obvious this needs to wait on other improvements. Thoughts, Jeff? |
|
@vtjnash might have some concerns --- is this at least consistent with the ambiguity detection we're capable of now, or does it make things worse? |
|
Worth waiting for. Maybe merge #35855 now, and change |
src/gf.c
Outdated
| 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); |
There was a problem hiding this comment.
This jl_gc_wb doesn't do anything (there's no memory being modified). Why is it here?
There was a problem hiding this comment.
My fault, came from resolving conflict with #35768 and I resolved it into here.
There was a problem hiding this comment.
None of those do anything either. What are they supposed to do?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Wouldn't work for that anyways
|
Hm, that actually may point to a simpler algorithm which could fix the problems with this one. Here, we only consider if the intersection was also partly covered by an ambiguity. But instead, what we really wanted to know is if the intersection was fully covered by an existing method that is not less specific. |
|
As I pointed out in #36005 (comment), this and one other (#35915) are basically the only things standing in the way of "democratizing" the hunt for invalidations. Would love to see this finished off. If no one else has time I could give it a whirl, but I have gotten the sense that @vtjnash knows what should be done. |
|
I can try implementing it. |
|
I'm sure this is laughable, but for me index e5870bdb9b..65dea8c3b6 100644
--- a/base/essentials.jl
+++ b/base/essentials.jl
@@ -166,7 +166,6 @@ true
"""
function convert end
-convert(::Type{Union{}}, x) = throw(MethodError(convert, (Union{}, x)))
convert(::Type{Any}, x) = x
convert(::Type{T}, x::T) where {T} = x
convert(::Type{Type}, x::Type) = x # the ssair optimizer is strongly dependent on this method existing to avoid over-specialization
diff --git a/src/gf.c b/src/gf.c
index f801cfc976..0de2f7008f 100644
--- a/src/gf.c
+++ b/src/gf.c
@@ -1725,11 +1725,13 @@ static int is_call_ambiguous(jl_methtable_t *mt, jl_value_t *types JL_PROPAGATES
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_value_t *isect = isect = jl_type_intersection(types, (jl_value_t*)m->sig);
+ size_t minworld = 0;
+ size_t maxworld = (size_t)world;
+ jl_value_t *mtchs = ml_matches(mt->defs, 0, (jl_tupletype_t*)isect, -1, 0, world, &minworld, &maxworld);
+ return jl_array_len(mtchs) == 0;
}
JL_DLLEXPORT void jl_method_table_insert(jl_methtable_t *mt, jl_method_t *method, jl_tupletype_t *simpletype)seems promising in fixing the issue. Would love to get this merged. Ambiguities are almost surely the single biggest "systematic" source of invalidations left. I can't tell for sure because SnoopCompile's analysis of this is crude and certainly wrong; if this is right, I can just trust it and move on with fixing specific invalidations. But the SNR is getting low enough that it's less satisfying to keep working on this without getting this case resolved first. |
|
I think that's also probably nearly right. One other observation I made while working on this code (61b1248a69d40c2fb931c35facf4e69e3a68fcd0) is that |
|
Hmm, even with this I am seeing that Line 230 in b15f6ad Line 556 in b15f6ad julia> typeintersect(Tuple, Array)
Union{}
julia> methods(Union{}, (Any,))
# 3 methods:
[1] (::Type{T})(x::Tuple) where T<:Tuple in Base at tuple.jl:225
[2] (::Type{T})(nt::NamedTuple) where T<:Tuple in Base at namedtuple.jl:128
[3] (::Type{T})(itr) where T<:Tuple in Base at tuple.jl:230But I guess julia> methods(Union{}, (AbstractArray,))
# 1 method:
[1] (::Type{T})(itr) where T<:Tuple in Base at tuple.jl:230is the only method, so maybe this is just something we need to look for explicitly? Not quite sure what the best way to implement this would be. |
|
This diff seems to exclude methods in diff --git a/src/gf.c b/src/gf.c
index eb293b77b3..d84f1c5b69 100644
--- a/src/gf.c
+++ b/src/gf.c
@@ -1774,10 +1774,17 @@ static int is_call_ambiguous(jl_methtable_t *mt, jl_value_t *types JL_PROPAGATES
jl_value_t *isect = isect = jl_type_intersection(types, (jl_value_t*)m->sig);
size_t minworld = 0;
size_t maxworld = (size_t)world;
- jl_value_t *mtchs = ml_matches(mt->defs, 0, (jl_tupletype_t*)isect, -1, 0, world, &minworld, &maxworld);
+ jl_value_t *mtchs = ml_matches(mt, 0, (jl_tupletype_t*)isect, -1, 0, world, &minworld, &maxworld, 0);
return jl_array_len(mtchs) == 0;
}
+jl_module_t* top_module(jl_module_t* mod)
+{
+ while (mod->parent != mod)
+ mod = mod->parent;
+ return mod;
+}
+
JL_DLLEXPORT void jl_method_table_insert(jl_methtable_t *mt, jl_method_t *method, jl_tupletype_t *simpletype)
{
JL_TIMING(ADD_METHOD);
@@ -1816,9 +1823,12 @@ 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];
+ jl_method_instance_t *backedge = (jl_method_instance_t*)backedges[i];
+ if (top_module(backedge->def.module) == jl_core_module &&
+ top_module(method->module) != jl_core_module)
+ continue;
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;
if (_jl_debug_method_invalidation)(First changed line of the diff is from rebasing.) |
Codecov Report
@@ Coverage Diff @@
## master #35877 +/- ##
========================================
Coverage 86.11% 86.12%
========================================
Files 348 349 +1
Lines 64975 65401 +426
========================================
+ Hits 55955 56324 +369
- Misses 9020 9077 +57
Continue to review full report at Codecov.
|
|
master:
julia> @time using Plots
6.092184 seconds (13.64 M allocations: 797.681 MiB, 4.29% gc time)
julia> @time display(plot(rand(10)))
6.243096 seconds (9.41 M allocations: 526.851 MiB, 3.82% gc time)This branch, together with the patches provided above: julia> @time using Plots
4.939772 seconds (9.16 M allocations: 570.798 MiB, 4.28% gc time)
julia> @time display(plot(rand(10)))
6.210409 seconds (9.45 M allocations: 528.374 MiB, 1.86% gc time)(Caveat: also with #36427 but I doubt that changes anything.) The savings are quite reproducible. Let's do this! Not only is it useful on its own, but it will clear out a bunch of useless clutter from the diagnostics of how to keep fixing other invalidations. |
|
Invalidations of Core.Compiler are not generally an issue, since they might affect some of the The main reason I asked to stall this, during this stage of the release cycle, is I'm testing a fairly significant overhaul of this system in #35983, which is nearing possible completion, and might need a slightly different approach. |
I wondered about that. I'd seen a bit of re-inference via
Oh, I didn't know you'd asked to stall it. I was puzzled by the lack of movement, thanks for explaining, it makes a difference. Good luck with #35983, that does seem like an interesting experiment. I will close #36374 as it was basically lobbying for finishing this 😄. |
|
I don't believe the module-based logic is technically valid, since Core and Base share the |
|
Sounds like the Core.Compiler stuff isn't that much of an issue anyway...I just wasn't sure, and it looked noteworthy. |
|
xref JuliaLang/www.julialang.org#794 (comment), JuMP is highly recommended as a test case for measuring the effect of this PR. (It bundles StaticArrays, which would be the other one I'd recommend, so JuMP is 2 for the price of 1.)
Now that #35983 has been merged, are there other components of that work that this is still waiting on? |
In these cases, the new method would not be called because the call would be an ambiguity error instead.
At least, that's what I'm trying to do. I'm not sure it is complete and correct, but so far it does reduce the output from
jl_debug_method_invalidationwhen loading FixedPointNumbers from ~600 lines to ~200.