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.

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

fix new type instability from getindex(::String, r::UnitRange{Int}) #55625

Merged
merged 1 commit into from
Aug 31, 2024

Conversation

aviatesk
Copy link
Member

@aviatesk aviatesk commented Aug 29, 2024

After investigating the cause of the invalidation reported in #55583, it was found that the issue arises only when r is propagated as an extended lattice element, such as PartialStruct(UnitRange{Int}, ...), for the method of getindex(::String, r::UnitRange{Int}). Specifically, the path at

if uncached
# our attempt to speculate into a constant call lead to an undesired self-cycle
# that cannot be converged: poison our call-stack (up to the discovered duplicate frame)
# with the limited flag and abort (set return type to Any) now
poison_callstack!(parent, frame)
return true
end
is hit, so the direct cause was the recursion limit for constant inference (inference with extended lattice elements, more correctly).

To explain in more detail, within the slow path of nextind which is called inside getindex(::String, ::UnitRange{Int}), the 1-argument @assert is used

@assert l >= 0xc0
. The code related to print associated with this @assert further uses getindex(::String, ::UnitRange{Int}), causing the recursion limit. This recursion limit is only for constant inference, which is why we saw this regression only for the PartialStruct case. Moreover, since this recursion limit occurs within the @assert-related code, this issue did not arise until now (i.e. until #49260 was merged).

As a solution, I considered improving the recursion limit itself, but decided that keeping the current code for the recursion limit of constant inference is safer. Ideally, this should be addressed on the compiler side, but there is certainly deep recursion in this case. As an easier solution, this commit resolves the issue by changing the 1-arg @assert to the 2-arg version.

@aviatesk
Copy link
Member Author

We can add this regression test:

let m = only(methods(Base._string_n, (Integer,)))
    mi = Core.Compiler.specialize_method(m, Tuple{typeof(Base._string_n),Integer}, Core.svec())
    m′ = only(methods(Base.getindex, (String,UnitRange{Int})))
    mi′ = Core.Compiler.specialize_method(m′, Tuple{typeof(Base.getindex),String,UnitRange{Int}}, Core.svec())
    @test mi′  mi.backedges
end

but I have doubts about whether it will remain a robust test case in the future. It's probably safer to use tools like JET for testing this sort of things.

@nsajko
Copy link
Contributor

nsajko commented Aug 29, 2024

In my PR this reproducer is provided:

using SnoopCompileCore
length(@snoop_invalidations begin
    struct I <: Integer end
    Csize_t
    Csize_t(::I) = Csize_t(0)
end)

Is there a way to count invalidations that could be used from within the test suite? Could we add a test something like:

@test length(invalidations) < 50

?

@aviatesk
Copy link
Member Author

The number of invalidations isn't that robust and SnoopCompile isn't available for Julia base test suite at this moment.

After investigating the cause of the invalidation reported in
#55583, it was found that the issue arises only when `r`
is propagated as an extended lattice element, such as
`PartialStruct(UnitRange{Int}, ...)`, for the method of
`getindex(::String, r::UnitRange{Int})`. Specifically, the path at
https://github.com/JuliaLang/julia/blob/cebfd7bc66153b82c56715cb1cb52dac7df8eac8/base/compiler/typeinfer.jl#L809-L815
is hit, so the direct cause was the recursion limit for constant
inference.

To explain in more detail, within the slow path of `nextind` which is
called inside `getindex(::String, ::UnitRange{Int})`, the 1-argument
`@assert` is used https://github.com/JuliaLang/julia/blob/cebfd7bc66153b82c56715cb1cb52dac7df8eac8/base/strings/string.jl#L211.
The code related to `print` associated with this `@assert` further uses
`getindex(::String, ::UnitRange{Int})`, causing the recursion limit.
This recursion limit is only for constant inference, which is why we
saw this regression only for the `PartialStruct` case.
Moreover, since this recursion limit occurs within the
`@assert`-related code, this issue did not arise until now (i.e. until
#49260 was merged).

As a solution, I considered improving the recursion limit itself, but
decided that keeping the current code for the recursion limit of
constant inference is safer. Ideally, this should be addressed on the
compiler side, but there is certainly deep recursion in this case. As an
easier solution, this commit resolves the issue by changing the 1-arg
`@assert` to the 2-arg version.

- replaces #55583
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants