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

Example where getfield does not constant fold in JET #254

Closed
jakobnissen opened this issue Sep 15, 2021 · 5 comments
Closed

Example where getfield does not constant fold in JET #254

jakobnissen opened this issue Sep 15, 2021 · 5 comments

Comments

@jakobnissen
Copy link
Contributor

It's a little tricky to replicate, I'll see if I can make a smaller example. I'll try to describe what's going on so you might not need to replicate it fully.

For now, you can replicate it by checking out TranscodingStreams.jl commit 0dedb79 (from my repo: https://github.com/jakobnissen/TranscodingStreams.jl/tree/0dedb795faceef48ce8aa5385808d4ab91d02c30), and doing the following:

julia> using TranscodingStreams, CodecZlib, JET

julia> f(s) = open(
           i -> first(read(i, String), 10),
           GzipDecompressorStream  NoopStream  GzipCompressorStream  NoopStream,
           s
       )
f (generic function with 1 method)

julia> @report_call f("Project.toml")

Here is the output:

═════ 1 possible error found ═════
┌ @ REPL[2]:1 Main.open(#1, Main.∘(Main.∘(Main.∘(Main.GzipDecompressorStream, Main.NoopStream), Main.GzipCompressorStream), Main.NoopStream), s)
│┌ @ /Users/jakobnissen/code/TranscodingStreams.jl/src/stream.jl:183 f(io)
││┌ @ REPL[2]:2 Main.read(i, Main.String)
│││┌ @ io.jl:969 Base.read(s)
││││┌ @ io.jl:964 #self#(s, Base.typemax(Base.Int))
│││││┌ @ io.jl:965 Base.readbytes!(s, b, nb)
││││││┌ @ /Users/jakobnissen/code/TranscodingStreams.jl/src/stream.jl:417 TranscodingStreams.eof(stream)
│││││││┌ @ /Users/jakobnissen/code/TranscodingStreams.jl/src/stream.jl:234 TranscodingStreams.fillbuffer(stream)
││││││││┌ @ /Users/jakobnissen/code/TranscodingStreams.jl/src/stream.jl:610 TranscodingStreams.#fillbuffer#11(false, #self#, stream)
│││││││││┌ @ /Users/jakobnissen/code/TranscodingStreams.jl/src/stream.jl:622 TranscodingStreams.readdata!(Base.getproperty(stream, :stream), buffer2)
││││││││││┌ @ /Users/jakobnissen/code/TranscodingStreams.jl/src/noop.jl:164 Base.setproperty!(buffer, :transcoded, TranscodingStreams.+(Base.getproperty(buffer, :transcoded), nfilled))
│││││││││││┌ @ Base.jl:43 Base.convert(Base.fieldtype(Base.typeof(x), f), v)
││││││││││││ for 1 of union split cases, no matching method found for call signatures (Tuple{typeof(convert), Type{Vector{UInt8}}, Int64})): Base.convert(Base.fieldtype(Base.typeof(x::TranscodingStreams.Buffer)::Type{TranscodingStreams.Buffer}, f::Symbol)::Union{Type{Vector{UInt8}}, Type{Int64}}, v::Int64)
│││││││││││└──────────────
(String, 1)

The weird thing is at the end. Line TranscodingStreams.jl/src/noop.jl:164 contains:

buffer.transcoded += nfilled

As can be seen from the final line of JET's output, buffer is inferred to be Buffer, which has the following layout:

Buffer <: Any
  data::Vector{UInt8}
  markpos::Int64
  bufferpos::Int64
  marginpos::Int64
  transcoded::Int64

And buffer.transcoded is inferred to be Union{Int64, Vector{UInt8}} - but this is despite JET realizing that the symbol :transcoded is constant. In other words, JET realizes it fetches getfield(::Buffer, :transcoded), but it does not realize that this is a Int64?

@jakobnissen jakobnissen changed the title Example where constant getproperty does not constant fold Example where getfield does not constant fold in JET Sep 15, 2021
@aviatesk
Copy link
Owner

It might be better to use the latest version of Cthulhu.jl to understand how constant-prop' heuristics work on your code:
JuliaDebug/Cthulhu.jl#225

And I guess you're on v1.7 ?
Actually v1.7 will fail to constant prop' for setproperty! calls when the return type is already constant (but constant-prop' is still useful to optimize the call, because setproperty! usually contains convert and such as in your program).
If that applies to this case, I "fixed" the heuristics on the latest master (JuliaLang/julia#41882), so you may not see it on v1.8 :p

Having said that, we may want JET to constant-prop' far more aggressively so that we won't see this sort of error even on v1.7 ?

@jakobnissen
Copy link
Contributor Author

Unfortunately I can't load JET on Julia master:

julia> using JET
[ Info: Precompiling JET [c3a54625-cd67-489e-a8e7-0a5a0ff4e31b]
ERROR: LoadError: MethodError: no method matching Core.Compiler.OptimizationParams(; inlining=true, inline_cost_threshold=100, inline_nonleaf_penalty=1000, inline_tupleret_bonus=250, inline_error_path_cost=20, max_methods=3, tuple_splat=32, union_splitting=4, unoptimize_throw_blocks=true)
[ stacktrace elided ]

So yeah, I'm on Julia 1.7. I'll try to use Cthulhu to dig in now. It turned out to be quite difficult to make a minimal example, probably because the fillbuffer! function can call itself recursively with different types

@aviatesk
Copy link
Owner

Oh, please checkout to the latest master branch of JET. v0.4.6 isn't be compatible with the latest master. The latest master branch should work with it, and should analyze your code faster.

It might be tricky to debug constant prop' stuff, so please ask me any trouble you have.

@jakobnissen
Copy link
Contributor Author

I blundered around in the recursive calls in Chtulhu for a while and did find one of the calls where the getfield inferred to ::Any (weird, since JET infers it to a Union) but after 20 descends I have no idea where in the callchain I ended up, so not sure it's useful.

Anyway, I tried with latest JET on Julia's master branch, and the issue was gone! Thanks - and closing as solved.

@aviatesk
Copy link
Owner

Okay glad to know that. I may try to debug your code tomorrow if I have time.

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

No branches or pull requests

2 participants