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

Added kwargs to invokelatest. #22646

Merged
merged 3 commits into from
Jul 27, 2017
Merged

Conversation

rofinn
Copy link
Contributor

@rofinn rofinn commented Jul 1, 2017

Closes #22642

@ararslan ararslan requested a review from yuyichao July 1, 2017 04:05
@rofinn
Copy link
Contributor Author

rofinn commented Jul 1, 2017

Not sure what's causing the xcode build issue.

@martinholters
Copy link
Member

The first Travis OSX error is

/Users/travis/build/JuliaLang/julia/src/codegen.cpp:1:1: error: unknown type name 'wnloadQueue'
wnloadQueue</string>

Maybe the codegen.cpp file got corrupted somehow? Also, for the other errors, the reported line numbers seem to be off, so maybe the beginning of that file was garbled? I'll restart the build, let's see whether that helps...

@stevengj
Copy link
Member

stevengj commented Jul 6, 2017

We should double-check performance here, as @JeffBezanson noted in #21543 that invokelatest is performance-critical for remote calls. (I don't know whether nanosoldier has benchmark coverage of this?)

@rofinn
Copy link
Contributor Author

rofinn commented Jul 6, 2017

I'm not sure if these benchmark tests are cheating but:

Regular Args

Base performance:

# Test Function
julia> issue19774(x::Int)::Int = 0
issue19774 (generic function with 1 method)

# 0.6.0-rc3.0
julia> @benchmark issue19774(0)
BenchmarkTools.Trial:
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     1.503 ns (0.00% GC)
  median time:      1.507 ns (0.00% GC)
  mean time:        1.666 ns (0.00% GC)
  maximum time:     149.130 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     1000

# PR
julia> @benchmark issue19774(0)
BenchmarkTools.Trial:
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     1.495 ns (0.00% GC)
  median time:      1.505 ns (0.00% GC)
  mean time:        1.601 ns (0.00% GC)
  maximum time:     51.402 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     1000

Okay, almost identical.

With invokelatest

function foo()
    eval(:(issue19774(x::Int)::Int = 2))
    return Base.invokelatest(issue19774, 0)
end

# 0.6.0-rc3.0
julia> @benchmark foo()
BenchmarkTools.Trial:
  memory estimate:  17.85 KiB
  allocs estimate:  285
  --------------
  minimum time:     2.261 ms (0.00% GC)
  median time:      2.649 ms (0.00% GC)
  mean time:        2.787 ms (0.09% GC)
  maximum time:     7.857 ms (59.68% GC)
  --------------
  samples:          1783
  evals/sample:     1

# PR
julia> @benchmark foo()
BenchmarkTools.Trial:
  memory estimate:  18.95 KiB
  allocs estimate:  312
  --------------
  minimum time:     2.362 ms (0.00% GC)
  median time:      2.855 ms (0.00% GC)
  mean time:        2.962 ms (0.25% GC)
  maximum time:     15.707 ms (78.52% GC)
  --------------
  samples:          1678
  evals/sample:     1

Okay, almost identical again, so it looks like the PR doesn't hurt invokelatest performance with regular args.

Kwargs.

# Test Function
julia> kwargs19774(x::Int, y::Int; z::Int=0)::Int = 1
kwargs19774 (generic function with 1 method)

# Base performance.
julia> @benchmark kwargs19774(2, 3; z=1)
BenchmarkTools.Trial:
  memory estimate:  96 bytes
  allocs estimate:  1
  --------------
  minimum time:     39.195 ns (0.00% GC)
  median time:      41.929 ns (0.00% GC)
  mean time:        56.237 ns (20.11% GC)
  maximum time:     38.655 μs (99.86% GC)
  --------------
  samples:          10000
  evals/sample:     992

# Overwriting kwargs19774
julia> function foo_kwargs()
          eval(:(kwargs19774(x::Int, y::Int; z::Int=3)::Int = z))
          return kwargs19774(2, 3; z=1)
       end

julia> @benchmark foo_kwargs()
BenchmarkTools.Trial:
  memory estimate:  18.17 KiB
  allocs estimate:  258
  --------------
  minimum time:     1.189 ms (0.00% GC)
  median time:      1.273 ms (0.00% GC)
  mean time:        1.395 ms (0.35% GC)
  maximum time:     18.418 ms (92.97% GC)
  --------------
  samples:          3533
  evals/sample:     1

# With invokelatest
julia> function foo_kwargs()
          eval(:(kwargs19774(x::Int, y::Int; z::Int=3)::Int = z))
          return Base.invokelatest(kwargs19774, 2, 3; z=1)
       end
foo_kwargs (generic function with 1 method)

julia> @benchmark foo_kwargs()
BenchmarkTools.Trial:
  memory estimate:  139.40 KiB
  allocs estimate:  2544
  --------------
  minimum time:     14.964 ms (0.00% GC)
  median time:      15.985 ms (0.00% GC)
  mean time:        16.381 ms (0.26% GC)
  maximum time:     28.723 ms (44.39% GC)
  --------------
  samples:          305
  evals/sample:     1

So, keyword performance isn't great, but it doesn't seem to hurt regular arg performance.

* Replaced the `eval` with a module to help with readability.
* Added a test case to demonstrate the failure condition that invokelatest fixes.
Copy link
Member

@vchuravy vchuravy left a comment

Choose a reason for hiding this comment

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

You are benchmarking the eval overhead more than the actual cost of invokelatest

issue19774(x::Int)::Int = 0
kwargs19774(x::Int, y::Int; z::Int=0)::Int = 1

function newinvokelatest(f, args...; kwargs...)
    inner(f, args...) = f(args...; kwargs...)
    Core._apply_latest(inner, (f, args...))
end

function foo()
    return Base.invokelatest(issue19774, 0)
end
function foo_new()
    return newinvokelatest(issue19774, 0)
end
function foo_kwargs()
    return newinvokelatest(kwargs19774, 2, 3; z=1)
end

Results

# Current implementation
julia> @benchmark foo()
BenchmarkTools.Trial: 
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     54.827 ns (0.00% GC)
  median time:      55.091 ns (0.00% GC)
  mean time:        55.673 ns (0.00% GC)
  maximum time:     130.690 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     985

# Using inner func
julia> @benchmark foo_new()
BenchmarkTools.Trial: 
  memory estimate:  112 bytes
  allocs estimate:  3
  --------------
  minimum time:     830.962 ns (0.00% GC)
  median time:      846.474 ns (0.00% GC)
  mean time:        877.734 ns (0.82% GC)
  maximum time:     21.935 μs (91.31% GC)
  --------------
  samples:          10000
  evals/sample:     78

# Kwargs in the mix
julia> @benchmark foo_kwargs()
BenchmarkTools.Trial: 
  memory estimate:  592 bytes
  allocs estimate:  14
  --------------
  minimum time:     4.265 μs (0.00% GC)
  median time:      4.325 μs (0.00% GC)
  mean time:        4.510 μs (2.55% GC)
  maximum time:     599.276 μs (96.54% GC)
  --------------
  samples:          10000
  evals/sample:     7

The following implementation performs better at least in my tests

function newinvokelatest2(f, args...; kwargs...)
    inner() = f(args...; kwargs...)
    Core._apply_latest(inner)
end
# foo_new with newinvokelatest2
@benchmark foo_new()
BenchmarkTools.Trial: 
  memory estimate:  112 bytes
  allocs estimate:  2
  --------------
  minimum time:     61.380 ns (0.00% GC)
  median time:      64.960 ns (0.00% GC)
  mean time:        77.797 ns (12.66% GC)
  maximum time:     2.548 μs (94.48% GC)
  --------------
  samples:          10000
  evals/sample:     982

tkelman referenced this pull request in JuliaEarth/GeoStats.jl Jul 13, 2017
@rofinn
Copy link
Contributor Author

rofinn commented Jul 17, 2017

@vchuravy Yeah, those tests seem a lot more reasonable, thanks! I've also updated the function to avoid passing args to the closure as per your suggestion. @yuyichao Would you be able to review when you get the chance?

tanmaykm added a commit to JuliaWeb/JuliaWebAPI.jl that referenced this pull request Jul 24, 2017
Call `Core._applu_latest` to support `open` mode in `APIResponder` on Julia 0.6.

We can call `Compat.invoke_latest` instead after JuliaLang/julia#22646 is merged and assimilated in Compat.jl suitably.
tanmaykm added a commit to JuliaWeb/JuliaWebAPI.jl that referenced this pull request Jul 24, 2017
Call `Core._apply_latest` to support `open` mode in `APIResponder` on Julia 0.6.

We can call `Compat.invokelatest` instead after JuliaLang/julia#22646 is merged and assimilated in Compat.jl suitably.
# We use a closure (`inner`) to handle kwargs.
inner() = f(args...; kwargs...)
Core._apply_latest(inner)
end
Copy link
Member

Choose a reason for hiding this comment

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

It seems weird to me to construct a new closure for each call. Wouldn't it be better to do:

_invoker(f, args, kwargs) = f(args..., kwargs...)
invokelatest(f, args...; kwargs...) = Core._apply_latest(_invoker, args, kwargs)

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't you need to do Core._apply_latest(_invoker, (f, args, kwargs))? In that case, the no-kwargs conditions would be slower.

julia> using BenchmarkTools

julia> issue19774(x::Int)::Int = 0
issue19774 (generic function with 1 method)

julia> kwargs19774(x::Int, y::Int; z::Int=0)::Int = 1
kwargs19774 (generic function with 1 method)

julia> function newinvokelatest(f, args...; kwargs...)
           inner() = f(args...; kwargs...)
           Core._apply_latest(inner)
       end
newinvokelatest (generic function with 1 method)

julia> newinvokelatest2(f, args...; kwargs...) = Core._apply_latest(_newinvoker, (f, args, kwargs))
newinvokelatest2 (generic function with 1 method)

julia> _newinvoker(f, args, kwargs) = f(args...; kwargs...)
_newinvoker (generic function with 1 method)

julia> function foo()
           return Base.invokelatest(issue19774, 0)
       end
foo (generic function with 1 method)

julia> function foo_new()
           return newinvokelatest(issue19774, 0)
       end
foo_new (generic function with 1 method)

julia> function foo_new2()
           return newinvokelatest2(issue19774, 0)
       end
foo_new2 (generic function with 1 method)

julia> function foo_kwargs()
           return newinvokelatest(kwargs19774, 2, 3; z=1)
       end
foo_kwargs (generic function with 1 method)

julia> function foo_kwargs2()
           return newinvokelatest2(kwargs19774, 2, 3; z=1)
       end
foo_kwargs2 (generic function with 1 method)

julia> @benchmark foo()
BenchmarkTools.Trial:
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     55.914 ns (0.00% GC)
  median time:      56.209 ns (0.00% GC)
  mean time:        59.357 ns (0.00% GC)
  maximum time:     193.226 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     984

julia> @benchmark foo_new()
BenchmarkTools.Trial:
  memory estimate:  112 bytes
  allocs estimate:  2
  --------------
  minimum time:     69.090 ns (0.00% GC)
  median time:      71.658 ns (0.00% GC)
  mean time:        84.819 ns (9.79% GC)
  maximum time:     2.234 μs (93.51% GC)
  --------------
  samples:          10000
  evals/sample:     976

julia> @benchmark foo_new2()
BenchmarkTools.Trial:
  memory estimate:  128 bytes
  allocs estimate:  3
  --------------
  minimum time:     231.818 ns (0.00% GC)
  median time:      235.975 ns (0.00% GC)
  mean time:        266.367 ns (4.67% GC)
  maximum time:     9.024 μs (92.26% GC)
  --------------
  samples:          10000
  evals/sample:     451

julia> @benchmark foo_kwargs()
BenchmarkTools.Trial:
  memory estimate:  1008 bytes
  allocs estimate:  20
  --------------
  minimum time:     3.734 μs (0.00% GC)
  median time:      3.801 μs (0.00% GC)
  mean time:        4.224 μs (3.51% GC)
  maximum time:     638.504 μs (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     8

julia> @benchmark foo_kwargs2()
BenchmarkTools.Trial:
  memory estimate:  560 bytes
  allocs estimate:  12
  --------------
  minimum time:     3.399 μs (0.00% GC)
  median time:      3.485 μs (0.00% GC)
  mean time:        3.902 μs (2.53% GC)
  maximum time:     517.470 μs (95.49% GC)
  --------------
  samples:          10000
  evals/sample:     8

Copy link
Member

Choose a reason for hiding this comment

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

Huh, okay.

tanmaykm added a commit to JuliaWeb/JuliaWebAPI.jl that referenced this pull request Jul 24, 2017
Call `Core._apply_latest` to support `open` mode in `APIResponder` on Julia 0.6.

We can call `Compat.invokelatest` instead after JuliaLang/julia#22646 is merged and assimilated in Compat.jl suitably.
@omus
Copy link
Member

omus commented Jul 26, 2017

I'll merge this tomorrow unless there are objections.

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

Successfully merging this pull request may close these issues.

5 participants