From 32b458bd6eee028a91040f49235909d74a3013a1 Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Wed, 19 Apr 2023 12:02:30 -0400 Subject: [PATCH] avoid some more invalidations that are not necessary Even if we have a new method that is more specific than the method it is replacing, there still might exist an existing method that is more specific than both which already covers their intersection. An example of this pattern is adding Base.IteratorSize(::Type{<:NewType}) causing invalidations on Base.IteratorSize(::Type) for specializations such as Base.IteratorSize(::Type{<:AbstractString}) even though the intersection of these is fully covered already by Base.IteratorSize(::Type{Union{}}) so our new method would never be selected there. This won't detect ambiguities that already cover this intersection, but that is why we are looking to move away from that pattern towards explicit methods for detection in closer to O(n) instead of O(n^2): #49349. Similarly, for this method, we were unnecessarily dropping it from the MethodTable cache. This is not a significant latency problem (the cache is cheap to rebuild), but it is also easy to avoid in the first place. Refs #49350 --- src/gf.c | 35 +++++++++++++++++++++++++++-------- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/src/gf.c b/src/gf.c index 704471a29b4ad..2c3485823202b 100644 --- a/src/gf.c +++ b/src/gf.c @@ -1795,6 +1795,22 @@ static int invalidate_mt_cache(jl_typemap_entry_t *oldentry, void *closure0) break; } } + if (intersects && (jl_value_t*)oldentry->sig != mi->specTypes) { + // the entry may point to a widened MethodInstance, in which case it is worthwhile to check if the new method + // actually has any meaningful intersection with the old one + intersects = !jl_has_empty_intersection((jl_value_t*)oldentry->sig, (jl_value_t*)env->newentry->sig); + } + if (intersects && oldentry->guardsigs != jl_emptysvec) { + // similarly, if it already matches an existing guardsigs, this is already safe to keep + size_t i, l; + for (i = 0, l = jl_svec_len(oldentry->guardsigs); i < l; i++) { + // see corresponding code in jl_typemap_entry_assoc_exact + if (jl_subtype((jl_value_t*)env->newentry->sig, jl_svecref(oldentry->guardsigs, i))) { + intersects = 0; + break; + } + } + } if (intersects) { if (_jl_debug_method_invalidation) { jl_array_ptr_1d_push(_jl_debug_method_invalidation, (jl_value_t*)mi); @@ -1937,8 +1953,7 @@ enum morespec_options { }; // check if `type` is replacing `m` with an ambiguity here, given other methods in `d` that already match it -// precondition: type is not more specific than `m` -static int is_replacing(jl_value_t *type, jl_method_t *m, jl_method_t *const *d, size_t n, jl_value_t *isect, jl_value_t *isect2, char *morespec) +static int is_replacing(char ambig, jl_value_t *type, jl_method_t *m, jl_method_t *const *d, size_t n, jl_value_t *isect, jl_value_t *isect2, char *morespec) { size_t k; for (k = 0; k < n; k++) { @@ -1951,11 +1966,15 @@ static int is_replacing(jl_value_t *type, jl_method_t *m, jl_method_t *const *d, if (morespec[k] == (char)morespec_is) // not actually shadowing this--m2 will still be better return 0; + // if type is not more specific than m (thus now dominating it) + // then there is a new ambiguity here, // since m2 was also a previous match over isect, - // see if m was also previously dominant over all m2 - if (!jl_type_morespecific(m->sig, m2->sig)) - // m and m2 were previously ambiguous over the full intersection of mi with type, and will still be ambiguous with type + // see if m was previously dominant over all m2 + // or if this was already ambiguous before + if (ambig != morespec_is && !jl_type_morespecific(m->sig, m2->sig)) { + // m and m2 were previously ambiguous over the full intersection of mi with type, and will still be ambiguous with addition of type return 0; + } } return 1; } @@ -2096,7 +2115,7 @@ JL_DLLEXPORT void jl_method_table_insert(jl_methtable_t *mt, jl_method_t *method ambig = jl_type_morespecific(type, m->sig) ? morespec_is : morespec_isnot; // replacing a method--see if this really was the selected method previously // over the intersection (not ambiguous) and the new method will be selected now (morespec_is) - int replaced_dispatch = ambig == morespec_is || is_replacing(type, m, d, n, isect, isect2, morespec); + int replaced_dispatch = is_replacing(ambig, type, m, d, n, isect, isect2, morespec); // found that this specialization dispatch got replaced by m // call invalidate_backedges(&do_nothing_with_codeinst, mi, max_world, "jl_method_table_insert"); // but ignore invoke-type edges @@ -2110,7 +2129,7 @@ JL_DLLEXPORT void jl_method_table_insert(jl_methtable_t *mt, jl_method_t *method int replaced_edge; if (invokeTypes) { // n.b. normally we must have mi.specTypes <: invokeTypes <: m.sig (though it might not strictly hold), so we only need to check the other subtypes - replaced_edge = jl_subtype(invokeTypes, type) && (ambig == morespec_is || is_replacing(type, m, d, n, invokeTypes, NULL, morespec)); + replaced_edge = jl_subtype(invokeTypes, type) && is_replacing(ambig, type, m, d, n, invokeTypes, NULL, morespec); } else { replaced_edge = replaced_dispatch; @@ -2137,7 +2156,7 @@ JL_DLLEXPORT void jl_method_table_insert(jl_methtable_t *mt, jl_method_t *method } if (jl_array_len(oldmi)) { // search mt->cache and leafcache and drop anything that might overlap with the new method - // TODO: keep track of just the `mi` for which shadowing was true (to avoid recomputing that here) + // this is very cheap, so we don't mind being fairly conservative at over-approximating this struct invalidate_mt_env mt_cache_env; mt_cache_env.max_world = max_world; mt_cache_env.shadowed = oldmi;