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

Some runtime reflection calls use the wrong world age for their queries. #53105

Open
gbaraldi opened this issue Jan 29, 2024 · 4 comments
Open
Labels
compiler:inference Type inference speculative Whether the change will be implemented is speculative

Comments

@gbaraldi
Copy link
Member

gbaraldi commented Jan 29, 2024

Base.return_types, Base.infer_return_type and friends use get_world_counter() to query the world, this means that they will reflect on worlds further in the future than what they are actually executing.

foo() = 1

function bar()
    while true
        @show Base.infer_return_type(foo, Tuple{})
        sleep(2)
    end
end

function baz()
    while true
        @show foo()
        sleep(2)
    end
end

Threads.@spawn bar()
Threads.@spawn baz()

foo() = 1
Base.infer_return_type(foo, Tuple{}) = Int64
foo() = 1
Base.infer_return_type(foo, Tuple{}) = Int64
foo() = 1
Base.infer_return_type(foo, Tuple{}) = Int64
foo() = 1

foo() = throw(error("should not affect running task"))

foo() = 1
Base.infer_return_type(foo, Tuple{}) = Union{}
foo() = 1
Base.infer_return_type(foo, Tuple{}) = Union{}

Maybe they should use ccall(:jl_get_tls_world_age, UInt, ()) instead.

@vtjnash vtjnash added speculative Whether the change will be implemented is speculative compiler:inference Type inference labels Jan 29, 2024
@vtjnash
Copy link
Member

vtjnash commented Jan 29, 2024

I will leave it up to @aviatesk to say whether these are supposed to query the tls world or the max world. I think his intent was to query in the max world, independent of what worker task might be doing the query in some fixed world.

@aviatesk
Copy link
Member

Indeed. Functions like Core.Compiler.return_type and hasmethod utilize TLS world age to legitimize their optimizations by the compiler, whereas the infer_xxx reflection series do not. So it might be acceptable to always perform inferences using the max world. Nonetheless, unifying them to consistently use TLS world age could also be beneficial. For scenarios like Language Server implementation, where it might be necessary to always use the latest world age, we can use invokelatest to get the same effect.

@vchuravy
Copy link
Member

I think this is also an API decision. Right now folks use Core.Compiler.return_type but Core.Compiler is very much defined as unstable and private.

So we should steer them towards a public API like Base.infer_return_type, but that is currently not usable for the purpose.

Note that #53088 exposes for the first time tls_world_age as Julia function.

@staticfloat
Copy link
Member

I tried to use Base.return_types() as an introspection tool to make things a bit more ergonomic. Example:

struct CachePool{T}
    pool::Vector{T}
    alloc::Function

    function CachePool(alloc::Function)
        T = only(Base.return_types(alloc))
        return new{T}(T[], alloc)
    end
end

Typically, I would require the user to provide a T manually to CachePool, however I saw using Base.return_types() as providing the following advantages:

  • It reduces the possibility that the user passes a T that doesn't actually match the return type of the function, such as passing T = Int64 but then returning [1] from alloc() and having things fail on a 32-bit system.
  • It allows me to implement some restrictions on T such as disallowing Any or Union{} types, to force the user to provide an allocation function that is type-stable and minimally allocating, if I wish.
  • It reduces unnecessary duplication of code.

Is it the intention of Base.return_types() to support this kind of usecase?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:inference Type inference speculative Whether the change will be implemented is speculative
Projects
None yet
Development

No branches or pull requests

5 participants