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

[release-1.6] First time printing is slow #39349

Closed
mforets opened this issue Jan 21, 2021 · 9 comments · Fixed by #39406
Closed

[release-1.6] First time printing is slow #39349

mforets opened this issue Jan 21, 2021 · 9 comments · Fixed by #39406
Labels
compiler:inference Type inference compiler:latency Compiler latency regression Regression in behavior compared to a previous version

Comments

@mforets
Copy link
Contributor

mforets commented Jan 21, 2021

This may be a Julia issue, reported in:

[mforets@localhost bin]$ ./julia
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.6.0-beta1 (2021-01-08)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release
|__/                   |

julia> @time using Polyhedra
  2.419120 seconds (6.69 M allocations: 486.807 MiB, 3.52% gc time, 0.41% compilation time)

julia> function foo()
           A = [1. 1; 1 -1; -1 0]
           b = [1, 0, 0]
           rep = hrep(A, b)
           plib = default_library(2, Float64)
           p = polyhedron(rep, plib)
       end
foo (generic function with 1 method)

julia> @time p = foo();
  0.022039 seconds (46.42 k allocations: 2.957 MiB, 99.94% compilation time)

julia> p # long time
Polyhedron DefaultPolyhedron{Float64, MixedMatHRep{Float64, Matrix{Float64}}, MixedMatVRep{Float64, Matrix{Float64}}}:
3-element iterator of HalfSpace{Float64, Vector{Float64}}:
 HalfSpace([1.0, 1.0], 1.0)
 HalfSpace([1.0, -1.0], 0.0)
 HalfSpace([-1.0, 0.0], 0.0)

There was a similar issue in Julia v1.01 (JuliaPolyhedra/Polyhedra.jl#128) and it was fixed in #30010, but the problem seems to have come back.

CC: @blegat, @schillic

@JeffBezanson
Copy link
Member

You're not kidding, that is a REALLY long time. I waited several minutes and it didn't finish. Finishes instantly with --compile=min though.

@JeffBezanson JeffBezanson added the regression Regression in behavior compared to a previous version label Jan 21, 2021
@JeffBezanson
Copy link
Member

Ok, 6 minutes in 1.5, 29 minutes on master...

@JeffBezanson
Copy link
Member

Got some interesting information on this. The regression happened in roughly 2 steps. One big one is #27843, which is not surprising since it widens types more slowly.

The other one was caused, believe it or not, by changing VERSION to 1.7.0-DEV. I imagine that would be due to version checks in packages enabling or disabling some code. All the time there is in METHOD_MATCH, so perhaps some extra method signatures are putting us on a bad path. The same regression can be triggered prior to 1.7.0-DEV by upgrading from

  [d8a4904e] MutableArithmetics v0.1.0
  [67491407] Polyhedra v0.5.8

to

  [d8a4904e] MutableArithmetics v0.2.14
  [67491407] Polyhedra v0.6.12

so perhaps that involves similar code changes.

@vtjnash
Copy link
Member

vtjnash commented Jan 23, 2021

Should we revert #27843, or is it more probable we can fix an underlying issue and get this to be much faster?

@JeffBezanson
Copy link
Member

Yeah, I'm inclined to revert it, but I might try a couple variations of that logic as well.

vtjnash pushed a commit that referenced this issue Jan 27, 2021
This reverts commit 6c1824d.

Fixes #39349. The extra inference precision is nice, but seems to carry too much risk of blowup.
@mforets
Copy link
Contributor Author

mforets commented Jan 27, 2021

thanks a lot for promptly addressing this issue!

@blegat
Copy link
Contributor

blegat commented Jan 27, 2021

Thanks a lot ! I have just tried this on master and

julia> p
Polyhedron DefaultPolyhedron{Float64, MixedMatHRep{Float64, Matrix{Float64}}, MixedMatVRep{Float64, Matrix{Float64}}}:
3-element iterator of HalfSpace{Float64, Vector{Float64}}:
 HalfSpace([1.0, 1.0], 1.0)
 HalfSpace([1.0, -1.0], 0.0)
 HalfSpace([-1.0, 0.0], 0.0)

only took a few seconds. I thought it would only remove one of the regressions from the 6 minutes to the 29 minutes of master so I was expecting the time to be between 6 and 29 minutes. But instead, it seems to not only have solved the regression but also the issue (that 6 minutes was already too slow) with Julia v1.5 since it only took a few seconds. Is that expected or am I not understanding this correctly ?

@KristofferC KristofferC reopened this Jan 27, 2021
@KristofferC
Copy link
Member

Reopening since #39406 couldn't have fixed this since the commit it reverts is not on the 1.6 beta.

@vtjnash vtjnash changed the title First time printing is slow [release-1.6] First time printing is slow Jan 27, 2021
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this issue May 4, 2021
…ang#39406)

This reverts commit 6c1824d.

Fixes JuliaLang#39349. The extra inference precision is nice, but seems to carry too much risk of blowup.
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this issue May 9, 2021
…ang#39406)

This reverts commit 6c1824d.

Fixes JuliaLang#39349. The extra inference precision is nice, but seems to carry too much risk of blowup.
@ViralBShah
Copy link
Member

Works in a couple of seconds now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:inference Type inference compiler:latency Compiler latency regression Regression in behavior compared to a previous version
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants