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

widenconst of a Const(::Type) can be quite slow #44402

Closed
Keno opened this issue Mar 2, 2022 · 4 comments · Fixed by #44704
Closed

widenconst of a Const(::Type) can be quite slow #44402

Keno opened this issue Mar 2, 2022 · 4 comments · Fixed by #44704
Labels
performance Must go faster

Comments

@Keno
Copy link
Member

Keno commented Mar 2, 2022

Currently a lot of our code is written with the assumption that widenconst(::Const) is extremely fast. However, this is not true if the value inside the Const happens to be a Type. There, it can be very slow:

c1 = Core.Const(Base.RefValue)
c2 = Core.Const(2)
julia> @btime Core.Compiler.widenconst(c1)
  4.540 μs (0 allocations: 0 bytes)
Type{Base.RefValue}

julia> @btime Core.Compiler.widenconst(c2)
  40.162 ns (0 allocations: 0 bytes)
Int64
@oscardssmith oscardssmith added the performance Must go faster label Mar 2, 2022
@vtjnash
Copy link
Member

vtjnash commented Mar 2, 2022

Refs #42596?

@Keno
Copy link
Member Author

Keno commented Mar 2, 2022

Perhaps, but I think even a single type cache lookup can be quite problematic for these. I'm playing with just remembering the pointer in the types itself.

@Keno
Copy link
Member Author

Keno commented Mar 2, 2022

In fact, as it stands, this issue might make #42596 problematic, because it might force computation of the widenconst value even when it isn't otherwise required.

@vtjnash
Copy link
Member

vtjnash commented Mar 2, 2022

I think that change might see widenconst return the kind instead, with the type-level information consistently tracked elsewhere instead

Keno added a commit that referenced this issue Mar 4, 2022
Introduces a spearate lattice elements for tracking const Types
that also stores the corresponding Type{} information, ensuring
that this is only computed once and whenever possible is taken
from an existing instance of it instead of recomputing it.

Part of #44402
JeffBezanson added a commit that referenced this issue Mar 23, 2022
JeffBezanson added a commit that referenced this issue Apr 4, 2022
JeffBezanson added a commit that referenced this issue Apr 5, 2022
mostly fixes #44402

Co-authored-by: Jameson Nash <[email protected]>
JeffBezanson added a commit that referenced this issue Apr 5, 2022
mostly fixes #44402

Co-authored-by: Jameson Nash <[email protected]>
JeffBezanson added a commit that referenced this issue Apr 6, 2022
mostly fixes #44402

Co-authored-by: Jameson Nash <[email protected]>
JeffBezanson added a commit that referenced this issue Apr 8, 2022
mostly fixes #44402

Co-authored-by: Jameson Nash <[email protected]>
JeffBezanson added a commit that referenced this issue Apr 13, 2022
Keno added a commit that referenced this issue Sep 1, 2022
1. Force inference to union-split the `widenconst` dispatch rather than
   joing through dynamic dispatch.

2. Avoid widenconst(::Const) in a hot path when not necessary. This got
   faster (#44402), but is not quite fast yet.

Makes inference a few % faster.
Keno added a commit that referenced this issue Sep 1, 2022
1. Force inference to union-split the `widenconst` dispatch rather than
   joing through dynamic dispatch.

2. Avoid widenconst(::Const) in a hot path when not necessary. This got
   faster (#44402), but is not quite fast yet.

Makes inference a few % faster.
Keno added a commit that referenced this issue Sep 2, 2022
1. Force inference to union-split the `widenconst` dispatch rather than
   joing through dynamic dispatch.

2. Avoid widenconst(::Const) in a hot path when not necessary. This got
   faster (#44402), but is not quite fast yet.

Makes inference a few % faster.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants