Skip to content

Commit 32b458b

Browse files
committed
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
1 parent b664693 commit 32b458b

File tree

1 file changed

+27
-8
lines changed

1 file changed

+27
-8
lines changed

src/gf.c

Lines changed: 27 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1795,6 +1795,22 @@ static int invalidate_mt_cache(jl_typemap_entry_t *oldentry, void *closure0)
17951795
break;
17961796
}
17971797
}
1798+
if (intersects && (jl_value_t*)oldentry->sig != mi->specTypes) {
1799+
// the entry may point to a widened MethodInstance, in which case it is worthwhile to check if the new method
1800+
// actually has any meaningful intersection with the old one
1801+
intersects = !jl_has_empty_intersection((jl_value_t*)oldentry->sig, (jl_value_t*)env->newentry->sig);
1802+
}
1803+
if (intersects && oldentry->guardsigs != jl_emptysvec) {
1804+
// similarly, if it already matches an existing guardsigs, this is already safe to keep
1805+
size_t i, l;
1806+
for (i = 0, l = jl_svec_len(oldentry->guardsigs); i < l; i++) {
1807+
// see corresponding code in jl_typemap_entry_assoc_exact
1808+
if (jl_subtype((jl_value_t*)env->newentry->sig, jl_svecref(oldentry->guardsigs, i))) {
1809+
intersects = 0;
1810+
break;
1811+
}
1812+
}
1813+
}
17981814
if (intersects) {
17991815
if (_jl_debug_method_invalidation) {
18001816
jl_array_ptr_1d_push(_jl_debug_method_invalidation, (jl_value_t*)mi);
@@ -1937,8 +1953,7 @@ enum morespec_options {
19371953
};
19381954

19391955
// check if `type` is replacing `m` with an ambiguity here, given other methods in `d` that already match it
1940-
// precondition: type is not more specific than `m`
1941-
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)
1956+
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)
19421957
{
19431958
size_t k;
19441959
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,
19511966
if (morespec[k] == (char)morespec_is)
19521967
// not actually shadowing this--m2 will still be better
19531968
return 0;
1969+
// if type is not more specific than m (thus now dominating it)
1970+
// then there is a new ambiguity here,
19541971
// since m2 was also a previous match over isect,
1955-
// see if m was also previously dominant over all m2
1956-
if (!jl_type_morespecific(m->sig, m2->sig))
1957-
// m and m2 were previously ambiguous over the full intersection of mi with type, and will still be ambiguous with type
1972+
// see if m was previously dominant over all m2
1973+
// or if this was already ambiguous before
1974+
if (ambig != morespec_is && !jl_type_morespecific(m->sig, m2->sig)) {
1975+
// m and m2 were previously ambiguous over the full intersection of mi with type, and will still be ambiguous with addition of type
19581976
return 0;
1977+
}
19591978
}
19601979
return 1;
19611980
}
@@ -2096,7 +2115,7 @@ JL_DLLEXPORT void jl_method_table_insert(jl_methtable_t *mt, jl_method_t *method
20962115
ambig = jl_type_morespecific(type, m->sig) ? morespec_is : morespec_isnot;
20972116
// replacing a method--see if this really was the selected method previously
20982117
// over the intersection (not ambiguous) and the new method will be selected now (morespec_is)
2099-
int replaced_dispatch = ambig == morespec_is || is_replacing(type, m, d, n, isect, isect2, morespec);
2118+
int replaced_dispatch = is_replacing(ambig, type, m, d, n, isect, isect2, morespec);
21002119
// found that this specialization dispatch got replaced by m
21012120
// call invalidate_backedges(&do_nothing_with_codeinst, mi, max_world, "jl_method_table_insert");
21022121
// but ignore invoke-type edges
@@ -2110,7 +2129,7 @@ JL_DLLEXPORT void jl_method_table_insert(jl_methtable_t *mt, jl_method_t *method
21102129
int replaced_edge;
21112130
if (invokeTypes) {
21122131
// 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
2113-
replaced_edge = jl_subtype(invokeTypes, type) && (ambig == morespec_is || is_replacing(type, m, d, n, invokeTypes, NULL, morespec));
2132+
replaced_edge = jl_subtype(invokeTypes, type) && is_replacing(ambig, type, m, d, n, invokeTypes, NULL, morespec);
21142133
}
21152134
else {
21162135
replaced_edge = replaced_dispatch;
@@ -2137,7 +2156,7 @@ JL_DLLEXPORT void jl_method_table_insert(jl_methtable_t *mt, jl_method_t *method
21372156
}
21382157
if (jl_array_len(oldmi)) {
21392158
// search mt->cache and leafcache and drop anything that might overlap with the new method
2140-
// TODO: keep track of just the `mi` for which shadowing was true (to avoid recomputing that here)
2159+
// this is very cheap, so we don't mind being fairly conservative at over-approximating this
21412160
struct invalidate_mt_env mt_cache_env;
21422161
mt_cache_env.max_world = max_world;
21432162
mt_cache_env.shadowed = oldmi;

0 commit comments

Comments
 (0)