More latency/inferrability work #21
Conversation
Codecov Report
@@ Coverage Diff @@
## master #21 +/- ##
===========================================
- Coverage 87.00% 16.56% -70.45%
===========================================
Files 10 9 -1
Lines 1170 1014 -156
===========================================
- Hits 1018 168 -850
- Misses 152 846 +694
Continue to review full report at Codecov.
|
|
CC @SimonDanisch, @IanButterworth. This package was already in pretty good shape, latency-wise, but it illustrates the style I generally find useful more broadly. |
|
@jkrumbiegel, let me know if you're at all interested in this PR. If so I might be able to help get the module in Makie updated for this (and work on latency there too, which I suspect will be a more impactful change). If not, feel free to close. |
| Base.getindex(rowcols::RowCols, side::Side) = side == Left ? rowcols.lefts : | ||
| side == Right ? rowcols.rights : | ||
| side == Top ? rowcols.tops : | ||
| side == Bottom ? rowcols.bottoms : throw_side(side) |
There was a problem hiding this comment.
why is this style better than the dispatch version? aren't four methods compiled anyway?
There was a problem hiding this comment.
ah I see now that you've changed it to an enum
There was a problem hiding this comment.
so this style with a couple of types should be avoided in general?
There was a problem hiding this comment.
It depends: if Side is effectively "closed" then an enum has a big advantage, but a big weakness if your goal is external extensibility. More detail: recall that Julia specializes methods for each specific argument type:
julia> abstract type StaticNumber <: Number end
julia> struct One <: StaticNumber end
julia> struct Two <: StaticNumber end
julia> f(n::StaticNumber) = nothing
f (generic function with 1 method)
julia> f(One())
julia> f(Two())
julia> using MethodAnalysis
julia> methodinstances(f)
2-element Vector{Core.MethodInstance}:
MethodInstance for f(::One)
MethodInstance for f(::Two)That means f has to be compiled twice. That's not so bad for such simple f, but if it has n arguments and each comes in 2 flavors then pretty soon you're at 2^n combinations, which is a big big problem. But with this version:
julia> @enum StaticNumber One Two
julia> f(n::StaticNumber) = nothing
f (generic function with 1 method)
julia> f(One)
julia> f(Two)
julia> using MethodAnalysis
julia> methodinstances(f)
1-element Vector{Core.MethodInstance}:
MethodInstance for f(::StaticNumber)it only has to be compiled once. If you have several such arguments to a multi-argument function, the resulting savings/costs can be quite large. This will presumably be more impactful in Makie itself which adds a lot more types.
The downside is that you can no longer use dispatch to extend Side unless you do it in this package. As long as you're OK with that, the advantages here are surely worth having.
| valign = :center, | ||
| default_rowgap = DEFAULT_ROWGAP_GETTER[](), | ||
| default_colgap = DEFAULT_COLGAP_GETTER[](), | ||
| halign::AlignAttribute = :center, |
There was a problem hiding this comment.
In principle, an align is just a float. The symbols are just because this is directly user facing, and it's nicer to write :right rather than 1. But I already feared that these type instabilities that propagate relatively far into the code aren't good.
| observablify(x, type=Any) = Observable{type}(x) | ||
| observablify(x) = Observable(x) | ||
| struct Observablify{T} end | ||
| Observablify{T}(x) where T = convert(Observable{T}, x)::Observable{T} |
There was a problem hiding this comment.
what is the purpose of this struct? isn't it weird to have a constructor Observablify that creates an Observable?
There was a problem hiding this comment.
Yeah, I'll change this. I can't remember right now why a regular function call didn't seem adequate.
| # These don't work completely but they have partial success | ||
| @warnpcfail precompile(repr, (MIME{Symbol("text/plain")}, GridLayout)) | ||
| let | ||
| while true |
There was a problem hiding this comment.
what does this while true do?
There was a problem hiding this comment.
Forces it to be compiled. Code at toplevel is often run in the interpreter, but we have a heuristic that says anything with a loop in it should be compiled because we know the interpreter is really slow and there's no way to know in general how many iterations the loop will have.
I'll add a comment about this.
There was a problem hiding this comment.
Thanks for your work, Tim! Sorry for the delay, I'm again a bit short on time these days, and this looked like a lot. Although now it seems there are mostly a few things that cause many changed lines, but are not so complex in themselves. I've commented on some things that I didn't understand when reading through.
|
No worries! Thanks for the review, mostly I just wanted to know whether you appreciated this or whether it wasn't helpful. I notice Makie is still on v0.5.5 for this package. Would it be premature to start upgrading it for this PR? I'm guessing the big hurdle is upgrading Makie to GeometryBasics 0.4? How bad is that likely to be? |
|
The good parts of this were done better in #30. Thanks again, @jkrumbiegel! |
This is a fairly extensive/intrusive change aimed at further reducing GridLayoutBase's contribution to the overall Makie latency. It shaves about 15% off the time to run the test suite.
The biggest changes are to the types:
SideandGridDirenums rather than types, to reduce the amount of compiler specializationIt also changes how the precompile file works. It may be slightly less comprehensive but far more portable.
You'll have to decide whether you think this is worthwhile, and if so on what schedule to adopt it.
As is presumably obvious, this is a breaking change.