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

Performance regression returning anonymous function from struct #40606

Closed
imLew opened this issue Apr 26, 2021 · 1 comment · Fixed by #51042
Closed

Performance regression returning anonymous function from struct #40606

imLew opened this issue Apr 26, 2021 · 1 comment · Fixed by #51042
Labels
performance Must go faster regression Regression in behavior compared to a previous version

Comments

@imLew
Copy link

imLew commented Apr 26, 2021

Sorry if the title isn't too clear, I'm not exactly sure what is going on. I experienced a huge performance decrease in a project I'm working on after upgrading to julia 1.6. I posted about it here, and following @KristofferC's advice tried reverting fd8f97e, which solved the problem.

A MWE that shows the problem is

using BenchmarkTools

struct Model
    ϕ  # feature maps
end

y(ϕ) = x -> ϕ(x)
y(model::Model) = x -> model.ϕ(x)
y(model::Model, x) =  model.ϕ(x)

ϕ = x -> x
M = Model(ϕ);
x = rand(100);

y(model::Model) = x -> model.ϕ(x)
@benchmark y($ϕ).($x) .* 1 
@benchmark y($M).($x) .* 1  # <---- This takes much longer with fd8f97e than without
@benchmark y($M, $x) .* 1  

This is an minimal as I could make it. The regression occurs when using the value of y($M).($x) (hence the .* 1). For julia installed from the Arch User Repository the results of the benchmark are:

julia> @benchmark y($M).($x) .* 1
BenchmarkTools.Trial:
  memory estimate:  277.09 KiB
  allocs estimate:  4286
  --------------
  minimum time:     3.082 ms (0.00% GC)
  median time:      3.914 ms (0.00% GC)
  mean time:        4.008 ms (0.42% GC)
  maximum time:     7.155 ms (41.82% GC)
  --------------
  samples:          1245
  evals/sample:     1

julia> versioninfo()
Julia Version 1.6.0
Commit f9720dc2eb* (2021-03-24 12:55 UTC)
Platform Info:
  OS: Linux (x86_64-pc-linux-gnu)
  CPU: Intel(R) Core(TM) i7-1065G7 CPU @ 1.30GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-11.1.0 (ORCJIT, icelake-client)

For julia master:

julia> @benchmark y($M).($x) .* 1
BenchmarkTools.Trial:
  memory estimate:  285.91 KiB
  allocs estimate:  4444
  --------------
  minimum time:     3.011 ms (0.00% GC)
  median time:      3.393 ms (0.00% GC)
  mean time:        3.549 ms (0.40% GC)
  maximum time:     6.644 ms (38.91% GC)
  --------------
  samples:          1407
  evals/sample:     1

julia> versioninfo()
Julia Version 1.7.0-DEV.1009
Commit 578d51857b* (2021-04-25 19:27 UTC)
Platform Info:
  OS: Linux (x86_64-pc-linux-gnu)
  CPU: Intel(R) Core(TM) i7-1065G7 CPU @ 1.30GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-11.0.1 (ORCJIT, icelake-client)

And finally for tag v1.6.0 with commit fd8f97e reverted:

julia> @benchmark y($M).($x) .* 1
BenchmarkTools.Trial:
  memory estimate:  4.38 KiB
  allocs estimate:  209
  --------------
  minimum time:     5.251 μs (0.00% GC)
  median time:      6.084 μs (0.00% GC)
  mean time:        6.344 μs (1.10% GC)
  maximum time:     150.130 μs (94.01% GC)
  --------------
  samples:          10000
  evals/sample:     6

julia> versioninfo()
Julia Version 1.6.1
Commit 3121d06bd6 (2021-04-25 18:53 UTC)
Platform Info:
  OS: Linux (x86_64-pc-linux-gnu)
  CPU: Intel(R) Core(TM) i7-1065G7 CPU @ 1.30GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-11.0.1 (ORCJIT, icelake-client)

I also tried reverting fd8f97e on master but there was a conflict in the affected code that I could not resolve.

@KristofferC KristofferC added performance Must go faster regression Regression in behavior compared to a previous version labels Apr 26, 2021
@vtjnash
Copy link
Member

vtjnash commented Aug 24, 2023

seems just as bad or worse now:

julia> @benchmark y($M).($x) .* 1  # <---- This takes much longer with fd8f97e than without
BenchmarkTools.Trial: 4185 samples with 1 evaluation.
 Range (min … max):  1.146 ms …  33.064 ms  ┊ GC (min … max): 0.00% … 96.04%
 Time  (median):     1.179 ms               ┊ GC (median):    0.00%
 Time  (mean ± σ):   1.192 ms ± 525.705 μs  ┊ GC (mean ± σ):  0.64% ±  1.48%

            ▁ ▅▄▄▄▃▃▂▂▅▃▃▄▃▃     ▁ ▁▅██▆▇▄▅▁                   
  ▁▁▂▃▄▅▆█▆█████████████████▇█▇▇██████████████▇▆▅▄▃▂▃▂▃▃▃▂▂▂▂ ▅
  1.15 ms         Histogram: frequency by time        1.22 ms <

 Memory estimate: 60.28 KiB, allocs estimate: 1424.

vtjnash added a commit that referenced this issue Aug 24, 2023
The `aft` parameter is a value already, so we should be checking it in
the value domain, not the type domain like `tt`. That check happens to
already be done (somewhat unnecessarily) earlier in the function.

Fixes #40606
aviatesk added a commit that referenced this issue Aug 27, 2023
The `aft` parameter is a value already, so we should be checking it in
the value domain, not the type domain like `tt`. That check happens to
already be done (somewhat unnecessarily) earlier in the function.

Fixes #40606

---------

Co-authored-by: Shuhei Kadowaki <[email protected]>
KristofferC pushed a commit that referenced this issue Sep 15, 2023
The `aft` parameter is a value already, so we should be checking it in
the value domain, not the type domain like `tt`. That check happens to
already be done (somewhat unnecessarily) earlier in the function.

Fixes #40606

---------

Co-authored-by: Shuhei Kadowaki <[email protected]>
(cherry picked from commit f24a93a)
nalimilan pushed a commit that referenced this issue Nov 5, 2023
The `aft` parameter is a value already, so we should be checking it in
the value domain, not the type domain like `tt`. That check happens to
already be done (somewhat unnecessarily) earlier in the function.

Fixes #40606

---------

Co-authored-by: Shuhei Kadowaki <[email protected]>
(cherry picked from commit f24a93a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster regression Regression in behavior compared to a previous version
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants