-
Notifications
You must be signed in to change notification settings - Fork 148
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
Add a work-around on Julia 1.0 for #857 #859
Conversation
`StaticVecOrMatLike` is a very large union, and this caused Julia 1.0.x to use a large amount of memory when compiling StaticArrays in the presence of other packages that also extend `LinearAlgebra.mul!` in a similar fashion. As an example, this led to JuMP using more than 4 GB of RAM when running `using JuMP`, causing its CI to crash. Computing the eltypes within the function, rather than specifying them in the type arguments greatly improves the issue, with `using JuMP` going from: julia> @time using JuMP [ Info: Precompiling JuMP [4076af6c-e467-56ae-b986-b466b2749572] Killed to: julia> @time using JuMP [ Info: Precompiling JuMP [4076af6c-e467-56ae-b986-b466b2749572] 97.868305 seconds (12.75 M allocations: 805.413 MiB, 0.30% gc time)
I have no idea how did you manage to figure this out but it's a really clean solution 👍 . |
A hunch based on bisecting (jump-dev/JuMP.jl#2390 (comment)) this diff: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, it looks good. Could you bump the version to 1.0.1?
@c42f could you tag a new version when this PR is merged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A clever workaround! I'm fine to just merge this, though if the type parameters are the problem, I wondered if we can get away with something even simpler.
Sure. By the way @mateuszbaran please feel free to push out low risk patch releases if you feel comfortable doing it. It's always good to get them done promptly. Anyway, let's see if we can get away with the one-line version of this workaround. If not, I think it's fine to merge what's here. |
OK, thanks. I didn't even know I have the right permissions. |
I think anyone with write privileges can do it? Just mention |
Error while trying to register: Action not recognized: as |
Haha oops! Apparently JuliaRegistrator is always listening, even if you quote its name in a code block 😆 |
OK, I'll try tagging 1.0.1 then when this is finished. |
Bumped the version and simplified the code to remove the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me. In fact, I'd argue that this is an improvement even for Julia > 1.0.
Generally I prefer to remove type parameters where possible (apart from the hack which is https://docs.julialang.org/en/v1/manual/performance-tips/#Be-aware-of-when-Julia-avoids-specializing which I wish would go away :) ).
At this point we could probably trim down the big comment block to a few lines, as the code doesn't actually look like a hack which needs explaining; it just looks nice.
Now there seems to be a problem with memory consumption in StaticArrays on Julia 1.3/Linux 😕 : https://github.com/JuliaArrays/StaticArrays.jl/runs/1503538242?check_suite_focus=true :
|
That was probably something random, I've re-run the tests and now they all pass. |
Thanks for the fast response. Looks like we're back up and running: jump-dev/JuMP.jl#2396 |
This PR is an attempt at fixing #857.
Briefly,
StaticVecOrMatLike
is a very large union, and this caused Julia 1.0.xto use a large amount of memory when compiling StaticArrays in the presence of
other packages that also extend
LinearAlgebra.mul!
in a similar fashion. As anexample, this led to JuMP using more than 4 GB of RAM when running
using JuMP
,causing its CI to crash.
Computing the eltypes within the function, rather than specifying them in the
type arguments greatly improves the issue, with
using JuMP
going from:to:
Here's a work-flow of after/before:
I also checked that allocations were still 0:
Closes #857