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

@report_opt reports runtime dispatch on abstract keyword arguments #269

Closed
dpinol opened this issue Oct 24, 2021 · 4 comments
Closed

@report_opt reports runtime dispatch on abstract keyword arguments #269

dpinol opened this issue Oct 24, 2021 · 4 comments

Comments

@dpinol
Copy link
Contributor

dpinol commented Oct 24, 2021

Hi,
the following base code causes a dispatch report.

struct S
    v::Vector
    S(;v) = new(v)
end
const s1=S(v=[3])

f(s::S) = S(v=s.v)
@report_opt f(s1)
═════ 1 possible error found ═════
┌ @ REPL[4]:1 NamedTuple{(:v,)}(%2)
│ runtime dispatch detected: NamedTuple{(:v,)}(%2::Tuple{Vector})
└─────────────

g() = f(s1)
@report_opt g()

However, variations with directly creating the S instance (V1) or doing it through 2 intermediate functions (V2) do not cause any report.

struct S
    v::Vector
    S(;v) = new(v)
end
const s1=S(v=[3])

f(s::S) = S(v=s.v)

@report_opt S(v=s1.v)
g() = f(s1)
@report_opt g()

With this variation V3 (normal argument instead of kwargs), I don't get any report either.

struct S
    v::Vector
    S(v) = new(v)
end
const s1=S([3])

f(s::S) = S(s.v)
@report_opt f(s1)

This final variation V4, identical to the first one, but running within a module I don't get any report either.

module Arg
using JET
struct S
    v::Vector
    S(;v) = new(v)
end
const s1=S(v=[3])

f(s::S) = S(v=s.v)
@report_opt f(s1)
end

Are all these results expected? If the dispatch reported by the original case is a true positive, is there a way to avoid it (apart from not using kwargs for the constructor and defining the field with a concrete type?)

thanks

@aviatesk
Copy link
Owner

aviatesk commented Oct 24, 2021

Are all these results expected?

It's not a false positive. In general, what report_opt reports should really reflect how Julia compiler fails to optimize your code.

The implementation of keyword argument scheme uses NamedTuple, which always tries to specialize inputs, and thus causes runtime dispatch if the input is abstract.

If you embed a global constant (like g() = f(s1)), Julia compiler may be able to constant fold it. This is why JET doesn't report the error for such cases.

@dpinol
Copy link
Contributor Author

dpinol commented Oct 25, 2021

thanks for your quick reply!

The implementation of keyword argument scheme uses NamedTuple, which always tries to specialize inputs, and thus causes runtime dispatch if the input is abstract.

This is unfortunate, as kw arguments make code much more robust and readable.

@aviatesk aviatesk changed the title @report_opt false positive on abstract type fields @report_opt reports runtime dispatch on abstract keyword arguments Oct 25, 2021
@aviatesk
Copy link
Owner

Yeah, it might make sense to allow @nospecialize to work with keyword arguments, like:

function kwIsType(; @nospecialize(a = nothing))
    isa(a, DataType) && a.name === Type.body.name
end

Maybe you want to file an issue at JuilaLang/julia ?

@dpinol
Copy link
Contributor Author

dpinol commented Oct 26, 2021

I will, thanks for the hint! 👍

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