Nospecialize close(c::Channel, excp::Exception) on excp.

[NHDaly]

Fixes #49507.

Avoids dynamic dispatch when closing a Channel with an Exception, and should avoid a call into the runtime for julia compilation when attempting to report an exception.

[NHDaly]

Avoids dynamic dispatch when closing a Channel with an Exception

Oh, jk i forgot that Channel is also a parametric type, so this will still have a dispatch to do.

... is there any reason to specialize on the Channel type? None of the Channel fields accessed in this function depend on the type of the parameter.. Would julia be able to tell that, and still be able to produce concrete code for a type without its type parameter if none of the accessed fields depend on the parameter? My guess is the answer is no, but it would be a neat feature.

@vchuravy or @pchintalapudi you are probably the best people I could get to answer that question - do you know?

[NHDaly]

@quinnj and @vtjnash: Without the above, i guess this will still trigger compilation, due to the dispatch on the Channel type.

Can I mark that as nospecialize too?

[vtjnash]

You may incur internal dispatch (for lock and unlock), but it can potentially still figure out memory layout and dispatch specializations

[vtjnash]

It looks like inference should know the type of Channel. Only the exception argument is type Any

[NHDaly]

You may incur internal dispatch (for lock and unlock), but it can potentially still figure out memory layout and dispatch specializations

Oh right. Good points..

.... Is there any reason I can't mark these as @nospecialize(c::Channel) as well???:

julia/base/channels.jl

Lines 525 to 528 in 36ace54

	lock(c::Channel) = lock(c.cond_take)
	lock(f, c::Channel) = lock(f, c.cond_take)
	unlock(c::Channel) = unlock(c.cond_take)
	trylock(c::Channel) = trylock(c.cond_take)

[NHDaly]

...I'm finding that I can't rely on Cthulhu to answer this question, since it seems to disregard the @nospecialize, and shows me a function that's been inferred specifically for the type i'm calling with, even though codegen will ultimately produce a single generic native function, right?

It looks like inference should know the type of Channel. Only the exception argument is type Any

@vtjnash: I'm not sure what you meant by this message. Can you expand on it? How does it apply to the situation where I mark both arguments nospecialize in close?

[NHDaly]

Ohhhhhh you're saying I don't need to mark it nospecialize in order to avoid a dispatch, since inference already knows the channel type, so i could still avoid the dispatch even if the exception type is unknown! 💡 great point. So it could still be an invoke, or even inlined 👍

Neat!

Oh, so then maybe Cthulhu was right after all, because it was devirtualizing my call! Aha, yes that was it. Neat!! :)
Julia is pretty cool you guys.

So then: the only fear would be that if this is the first exception we've ever called close() on for this channel type, we might still stackoverflow. But if we've done it for some other exceptions first, we'll be okay.
... that's probably still an improvement, and so we can probably do without marking it @nospecialize. (Unless we think it would still produce good code even in that case.)

[NHDaly]

Wow, incredible! Maybe it actually can produce good code, even if the Channel is nospecialize!

julia> struct Foo{T}
           a::Int
           b::T
       end

julia> function foo(@nospecialize(x::Foo))
           bar(x.a)
       end
foo (generic function with 1 method)

julia> bar(v) = v + 1
bar (generic function with 1 method)

julia> function run(v::Vector{Any})
           return foo(v[1])
       end
run (generic function with 1 method)

julia> using Cthulhu

julia> @descend run(Any[Foo(1, "hi")])
run(v::Vector{Any}) in Main at REPL[4]:1
1 function run(v::Vector{Any}::Vector{Any})::Int64
2     return foo(v::Vector{Any}[1]::Any)::Int64
3 end
Select a call to descend into or ↩ to ascend. [q]uit. [b]ookmark.
Toggles: [w]arn, [h]ide type-stable statements, [t]ype annotations, [s]yntax highlight for Source/LLVM/Native.
Show: [S]ource code, [A]ST, [T]yped code, [L]LVM IR, [N]ative code
Actions: [E]dit source code, [R]evise and redisplay
   2 v::Vector{Any}[1]
 • 2 foo(v::Vector{Any}[1]::Any)
   ↩
foo(x::Foo) in Main at REPL[2]:1
1 function foo(@nospecialize(x::Foo::Foo))::Int64
2     bar(x::Foo.a::Int64)::Int64
3 end
Select a call to descend into or ↩ to ascend. [q]uit. [b]ookmark.
Toggles: [w]arn, [h]ide type-stable statements, [t]ype annotations, [s]yntax highlight for Source/LLVM/Native.
Show: [S]ource code, [A]ST, [T]yped code, [L]LVM IR, [N]ative code
Actions: [E]dit source code, [R]evise and redisplay
   2 x::Foo.a
 • 2 bar(x::Foo.a::Int64)
   ↩
bar(v) in Main at REPL[3]:1
1 bar(v::Int64)::Int64 = v::Int64 + 1
Select a call to descend into or ↩ to ascend. [q]uit. [b]ookmark.
Toggles: [w]arn, [h]ide type-stable statements, [t]ype annotations, [s]yntax highlight for Source/LLVM/Native.
Show: [S]ource code, [A]ST, [T]yped code, [L]LVM IR, [N]ative code
Actions: [E]dit source code, [R]evise and redisplay
   1  v::Int64 + 1
 • ↩

julia>

Thanks Cthulhu: you're an amazing tool. (And thanks @vchuravy ❤️)

So I'm down to nospeicalize everything! Thoughts? I'll send up a commit like that too.

[NHDaly]

K, i put that into a separate PR, here, since I kind of went crazy and added a bunch of them everywhere it seemed to not depend on T:
#49509

I'll merge this one once tests are green, since we're happy with it, and we can discuss what to do next on #49509.

[NHDaly]

So then: the only fear would be that if this is the first exception we've ever called close() on for this channel type, we might still stackoverflow. But if we've done it for some other exceptions first, we'll be okay.

Oh, even this isn't a concern, since when it's devirtualized, it'll be inferred during its caller's inference! Wow, julia is pretty rad. 😎

[NHDaly]

BTW, yep, this does indeed resolve the issue by preventing dispatch+compilation when reporting the error, since it could already be compiled when compiling its caller.

Here's evidence: the reproducer from the original issue now succeeds:

julia> ch = let result = Channel()

       foo() = try
           foo()
       catch e;
           close(result, e)
       end

       foo()

       result
       end
Channel{Any}(0) (closed)

julia> take!(ch)
ERROR: StackOverflowError:
Stacktrace:
 [1] check_channel_state
   @ ./channels.jl:174 [inlined]
 [2] take_unbuffered(c::Channel{Any})
   @ Base ./channels.jl:472
 [3] take!(c::Channel{Any})
   @ Base ./channels.jl:451
 [4] top-level scope
   @ REPL[2]:1

[NHDaly]

Added a test, all tests passed, merging now. Thanks!

[NHDaly]

Since this is a bug-fix, could it be a backport candidate?

[vtjnash]

I forgot I meant to say this should go into the stack_overflow.jl file, since the handling of this error is quite bad and sometimes ends up breaking the process (as I found out today trying to debug something completely unrelated). Do you mind making a PR to move it?

[NHDaly]

Oh, interesting. Yeah, i'm happy to move it! 👍 PR incoming

[NHDaly]

PR here: #49702