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 makes function unusable on 1.10.0-beta1 #50859

Closed
nathanrboyer opened this issue Aug 9, 2023 · 13 comments · Fixed by #51154
Closed

Performance regression makes function unusable on 1.10.0-beta1 #50859

nathanrboyer opened this issue Aug 9, 2023 · 13 comments · Fixed by #51154
Assignees
Labels
performance Must go faster regression Regression in behavior compared to a previous version
Milestone

Comments

@nathanrboyer
Copy link
Contributor

Sample code for testing is at the following repository: https://github.com/nathanrboyer/JuliaNightlyTestCode

polyfit function takes 5 minutes on 1.9 and over 1 day on 1.10. I get similar performance to 1.10.0-beta1 using the Nightly build from August 8th, 2023.

@nathanrboyer
Copy link
Contributor Author

nathanrboyer commented Aug 9, 2023

My timings using the trimmed data are below.

julia> versioninfo()
Julia Version 1.9.2
Commit e4ee485e90 (2023-07-05 09:39 UTC)
Platform Info:
  OS: Windows (x86_64-w64-mingw32)
  CPU: 8 × Intel(R) Core(TM) i7-9700 CPU @ 3.00GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-14.0.6 (ORCJIT, skylake)
  Threads: 1 on 8 virtual cores
Environment:
  JULIA_EDITOR = code.cmd
  JULIA_NUM_THREADS =

julia> include("test.jl")
  2.359726 seconds (6.76 M allocations: 3.041 GiB, 16.36% gc time, 72.71% compilation time)
  0.601131 seconds (2.38 M allocations: 2.750 GiB, 32.01% gc time)
  2.536775 seconds (8.09 M allocations: 5.714 GiB, 17.44% gc time, 54.94% compilation time)

julia> include("test.jl")
  0.613573 seconds (2.38 M allocations: 2.750 GiB, 33.90% gc time)
  0.572166 seconds (2.38 M allocations: 2.750 GiB, 29.21% gc time)
  1.119766 seconds (4.84 M allocations: 5.502 GiB, 30.87% gc time)
julia> versioninfo()
Julia Version 1.11.0-DEV.239
Commit c40ecd3f03 (2023-08-08 03:06 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Windows (x86_64-w64-mingw32)
  CPU: 8 × Intel(R) Core(TM) i7-9700 CPU @ 3.00GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-15.0.7 (ORCJIT, skylake)
  Threads: 1 on 8 virtual cores

julia> include("test.jl")
  1.996888 seconds (6.10 M allocations: 1.759 GiB, 11.57% gc time, 74.28% compilation time)
  0.489526 seconds (2.06 M allocations: 1.490 GiB, 37.59% gc time)
1119.263458 seconds (665.26 M allocations: 50.574 GiB, 0.50% gc time, 0.09% compilation time)

julia> include("test.jl")
  0.533068 seconds (2.13 M allocations: 1.494 GiB, 25.27% gc time, 18.15% compilation time)
  0.427734 seconds (2.06 M allocations: 1.490 GiB, 22.92% gc time)
1096.128067 seconds (663.10 M allocations: 50.433 GiB, 0.38% gc time, 0.00% compilation time)

Looks like 1.10 is allocating too much.

@gbaraldi
Copy link
Member

gbaraldi commented Aug 9, 2023

So this seems to have regressed a couple of times actually.
I'm doing the bisec right now, but one big regression happened in
35e4a1f @vtjnash

@Seelengrab Seelengrab added performance Must go faster regression Regression in behavior compared to a previous version labels Aug 9, 2023
@Seelengrab
Copy link
Contributor

That's a 288x regression 😮

@gbaraldi
Copy link
Member

gbaraldi commented Aug 9, 2023

It actually has exponential behaviour here which is very fun.

@Seelengrab
Copy link
Contributor

Seems like a good candidate for GC benchmarks then!

@gbaraldi
Copy link
Member

gbaraldi commented Aug 9, 2023

It's not a GC regression at all, 35e4a1f made it super bad and fbbe9ed made it superbad 2x

@brenhinkeller
Copy link
Contributor

Is there an intuitive explanation for why making Tuple{Union{}} "unconstructable" caused this?

@gbaraldi
Copy link
Member

I believe it's the change to the broadcasting code that caused this.

@gbaraldi
Copy link
Member

gbaraldi commented Aug 10, 2023

So what I think is going on here is that this code is somewhat unstable but return_type moved from a compile time thing to a runtime one. I'm not sure why however.

@brenhinkeller
Copy link
Contributor

ahh, interesting

@nathanrboyer
Copy link
Contributor Author

I'm open to just making my code more type-stable if you have suggestions for that and think it unlikely that others will run into this issue. I thought that I had defined types well enough for the compiler but apparently not.

@brenhinkeller
Copy link
Contributor

That's always good too (sometimes easier said than done, though I find Cthulhu.jl is nice for tracking down where the instabilities start) -- but I think this is also something we'll want to try to fix! Looks like it's been assigned to the 1.10 milestone, meaning 1.10 won't be released until it's addressed at least in some way

@vtjnash
Copy link
Member

vtjnash commented Aug 31, 2023

Sounds probably the same as #51129

vtjnash added a commit that referenced this issue Aug 31, 2023
Inference seems to have trouble with the anonymous function version, so
go back to the recursive version.

Fixes #51129
Probably also fixes #50859
vtjnash added a commit that referenced this issue Sep 1, 2023
Inference seems to have trouble with the anonymous function version, so
go back to the recursive version.

Fixes #51129
Probably also fixes #50859
N5N3 pushed a commit that referenced this issue Sep 3, 2023
Inference seems to have trouble with the anonymous function version, so
go back to the recursive version.

Fixes #51129
Probably also fixes #50859
KristofferC pushed a commit that referenced this issue Sep 15, 2023
Inference seems to have trouble with the anonymous function version, so
go back to the recursive version.

Fixes #51129
Probably also fixes #50859

(cherry picked from commit d949bb4)
nalimilan pushed a commit that referenced this issue Nov 5, 2023
Inference seems to have trouble with the anonymous function version, so
go back to the recursive version.

Fixes #51129
Probably also fixes #50859

(cherry picked from commit d949bb4)
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.

6 participants