Skip to content

Commit

Permalink
Fix incorrect world age range expansion
Browse files Browse the repository at this point in the history
This is a bit of a tricky one, so let me start at the beginning. In
PR #25506, I pushed my WIP of a new inlining passes. However, one of
the things it temporarily doesn't do is turn call sites to :invoke,
causing significantly more jl_apply_generic calls to be codegened.
On CI for that PR (as well as locally), we non-deterministically see
errors like:
```
LoadError("sysimg.jl", 213, KeyError(LibGit2 [76f85450-5226-5b5a-8eaa-529ad045b433]))
```
Some investigation revealed that what happened here is that a `PkgId`
got placed in a dictionary, but was later failed to be retrieved
because its hash had supposedly changes. Further investigation reveals
that while the hash function used to place the item in the dictionary
was `hash(x::Union{String,SubString{String}}, h::UInt64)` in hashing2.jl,
the hash function being used to retrieve it is `hash(@nospecialize(x), h::UInt64)`
in hashing.jl. The former is loaded later than the latter and should thus invalidate
all specializations of the latter, making them ineligible for selection by
jl_apply_generic. However, looking at the appropriate age ranges showed that
`typeof(hash).name.mt.cache` had entries for both hash functions with overlapping
age ranges (which is obviously incorrect).

Tracking age range updates, it turns out the world age range for the specialization
of `hash(@nospecialize(x), h::UInt64)` was expanded by invalidate_backedges called
upon the definition of a hash function inside `LibGit2`. This seems wrong. I believe
invalidate_backedges should only ever truncate world age ranges rather than expanding
them. This patch simply does that.

The non-determinism of the original issue appears to have to do with which of the
specializations happen to be in the `jl_lookup_generic_` `call_cache` fast-path.

This issue is fairly hard to reproduce because it requires a very specific confluence
of circumstances. Since the range of the captured specialization only gets extended
to before the min_world age of the new definition, it is never visible in the latest
world (e.g. at the REPL). The included test case demonstrates the issue by capturing
the world age with a task. Commenting out the `f(x::Float64)` definition makes
the test pass, because it is that definition that causes the expansion of the original
specialization of `f`.

Ref #26514
(cherry picked from commit 90a2162)
  • Loading branch information
Keno authored and ararslan committed Apr 26, 2018
1 parent accb624 commit b1ed898
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 2 deletions.
4 changes: 2 additions & 2 deletions src/gf.c
Original file line number Diff line number Diff line change
Expand Up @@ -292,8 +292,8 @@ struct set_world {
static int set_max_world2(jl_typemap_entry_t *entry, void *closure0)
{
struct set_world *closure = (struct set_world*)closure0;
// entry->max_world should be <= closure->replaced->max_world and >= closure->world
if (entry->func.linfo == closure->replaced) {
// entry->max_world should be <= closure->replaced->max_world
if (entry->func.linfo == closure->replaced && entry->max_world > closure->world) {
entry->max_world = closure->world;
}
return 1;
Expand Down
27 changes: 27 additions & 0 deletions test/worlds.jl
Original file line number Diff line number Diff line change
Expand Up @@ -167,3 +167,30 @@ f_gen265(x::Type{Int}) = 3
@test g_gen265(0) == 1
@test f_gen265(Int) == 3
@test g_gen265b(0) == 3

# Test that old, invalidated specializations don't get revived for
# intermediate worlds by later additions to the method table that
# would have capped those specializations if they were still valid
f26506(@nospecialize(x)) = 1
g26506(x) = f26506(x[1])
z = Any["ABC"]
f26506(x::Int) = 2
g26506(z) # Places an entry for f26506(::String) in mt.name.cache
f26506(x::String) = 3
cache = typeof(f26506).name.mt.cache
# The entry we created above should have been truncated
@test cache.min_world == cache.max_world
c26506_1, c26506_2 = Condition(), Condition()
# Captures the world age
result26506 = Any[]
t = Task(()->begin
wait(c26506_1)
push!(result26506, g26506(z))
notify(c26506_2)
end)
yield(t)
f26506(x::Float64) = 4
cache = typeof(f26506).name.mt.cache
@test cache.min_world == cache.max_world
notify(c26506_1); wait(c26506_2);
@test result26506[1] == 3

0 comments on commit b1ed898

Please sign in to comment.