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

work around a splatting penalty in twiceprecision #29060

Merged
merged 2 commits into from
Sep 6, 2018
Merged

Conversation

KristofferC
Copy link
Member

@KristofferC KristofferC commented Sep 5, 2018

Before:

julia> @btime 0:$(286.493442):360
  1.648 μs (8 allocations: 352 bytes)

After:

julia> @btime 0:$(286.493442):360
  170.405 ns (0 allocations: 0 bytes)

Ref https://discourse.julialang.org/t/a-possible-regression-in-0-7/14597

The typed code before this PR looks like

julia> f() = Base.steprangelen_hp(Float64, 2.0, 25.0, 5, 10, 2)
f (generic function with 1 method)

julia> @code_typed f()
CodeInfo(
1 1 ─ %1  = (Core._apply)(TwicePrecision{Float64}, 2.0)::TwicePrecision{Float64}                                                                                                                                   │╻     steprangelen_hp
  │   %2  = (Core._apply)(TwicePrecision{Float64}, 25.0)::TwicePrecision{Float64}  
...           

so not sure why that is not devirtualized (if this is the correct term here).

@KristofferC KristofferC added the performance Must go faster label Sep 5, 2018
@mbauman
Copy link
Member

mbauman commented Sep 5, 2018

Oh interesting. We have special support for splatting tuples, but this is sometimes splatting numbers to just pass one argument, and there we hit the pessimistic case:

julia> f(x) = println(x...)
f (generic function with 1 method)

julia> @code_typed f(1)
CodeInfo(
1 1%1 = (Core._apply)(Main.println, x)::Const(nothing, false)           │
  └──      return %1                                                       │
) => Nothing

julia> @code_typed f((1,2))
CodeInfo(
1 1%1 = (getfield)(x, 1)::Int64                                         │
  │   %2 = (getfield)(x, 2)::Int64                                         │
  │   %3 = invoke Main.println(%1::Int64, %2::Int64)::Const(nothing, false)│
  └──      return %3                                                       │
) => Nothing

Maybe add an @allocated test?

@vtjnash
Copy link
Member

vtjnash commented Sep 5, 2018

but this is sometimes splatting numbers to just pass one argument

I think Jarrett was working on this some in #28955

@martinholters
Copy link
Member

Ref. #27434 (comment) re. elision of _apply here.

@KristofferC KristofferC added the potential benchmark Could make a good benchmark in BaseBenchmarks label Sep 6, 2018
Copy link
Member

@Keno Keno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, though we should fix the underlying compiler issue also.

@KristofferC
Copy link
Member Author

When that happens I'll happily revert :)

@KristofferC KristofferC merged commit 88d536a into master Sep 6, 2018
@mbauman mbauman deleted the kc/perf branch September 6, 2018 18:07
KristofferC added a commit that referenced this pull request Sep 6, 2018
* work around a splatting penalty in twiceprecision

* add allocation test

(cherry picked from commit 88d536a)
@KristofferC KristofferC mentioned this pull request Sep 6, 2018
KristofferC added a commit that referenced this pull request Sep 8, 2018
* work around a splatting penalty in twiceprecision

* add allocation test

(cherry picked from commit 88d536a)
KristofferC added a commit that referenced this pull request Sep 8, 2018
* work around a splatting penalty in twiceprecision

* add allocation test

(cherry picked from commit 88d536a)
KristofferC added a commit that referenced this pull request Feb 11, 2019
* work around a splatting penalty in twiceprecision

* add allocation test

(cherry picked from commit 88d536a)
Keno added a commit that referenced this pull request Jul 19, 2020
But keep the test. This workaround is no longer required, because the
compiler can now understand this pattern.

This reverts commit 88d536a.
Keno added a commit that referenced this pull request Jul 19, 2020
…36728)

But keep the test. This workaround is no longer required, because the
compiler can now understand this pattern.

This reverts commit 88d536a.
simeonschaub pushed a commit to simeonschaub/julia that referenced this pull request Aug 11, 2020
…29060)" (JuliaLang#36728)

But keep the test. This workaround is no longer required, because the
compiler can now understand this pattern.

This reverts commit 88d536a.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster potential benchmark Could make a good benchmark in BaseBenchmarks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants