remove gridcontent type parameterization #30

jkrumbiegel · 2022-01-16T10:40:20Z

No description provided.

codecov-commenter · 2022-01-16T10:42:07Z

Codecov Report

Merging #30 (a07b954) into master (d3700e2) will increase coverage by 4.86%.
The diff coverage is 94.88%.

@@            Coverage Diff             @@
##           master      #30      +/-   ##
==========================================
+ Coverage   86.89%   91.76%   +4.86%     
==========================================
  Files           7        6       -1     
  Lines        1351     1312      -39     
==========================================
+ Hits         1174     1204      +30     
+ Misses        177      108      -69

Impacted Files	Coverage Δ
src/GridLayoutBase.jl	`100.00% <ø> (ø)`
src/types.jl	`96.55% <0.00%> (-3.45%)`	⬇️
src/gridlayout.jl	`93.52% <94.87%> (+1.79%)`	⬆️
src/gridlayoutspec.jl	`100.00% <100.00%> (ø)`
src/helpers.jl	`80.51% <100.00%> (+4.90%)`	⬆️
src/layoutobservables.jl	`86.13% <100.00%> (+0.28%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d3700e2...a07b954. Read the comment docs.

jkrumbiegel · 2022-01-23T22:09:18Z

@timholy I've been trying to incorporate most of the changes from your older PR (I made some other changes in the meantime that I needed to work around, also I really wanted to understand everything that was going on), but now I'm a bit surprised at the output of this snippet:

using SnoopCompile
using GridLayoutBase

tinf = @snoopi_deep begin
    gl = GridLayout()
    gl2 = GridLayout()
    gl[1, 1] = gl2
    nothing
end

It seems that there are large flames that are not red, and I thought I'd specifically addressed them in the precompilation file. Why are these lines still being inferred again, is there a trick to it that I'm not getting?

Somehow all my "improvements" didn't seem to have any effect on timings. I have a little benchmark package which just compares runs across commits/branches and I compared master to my branch for this code:

# using GridLayoutBase
@timed using GridLayoutBase

# GridLayout constructor
@timed GridLayout()

# Big GridLayout
@timed let
    gl = GridLayout()
    for i in 1:10, j in 1:10, k in 1:10
        gl[i, j] = GridLayout()
    end
end

Here's the output, the optimizations branch is not really faster (here it might look like 10ms for using, but this measurement also fluctuates quite a bit so it might not be super reliable to time using it seems. I expected a clear difference given all the changes I made).

6×11 DataFrame
 Row │ version        repetition  name                    juliaversion  commit     allocations  time      gctime     file                               commit_date          date               ⋯
     │ String         Int64       String                  String        SubStrin…  Int64        Float64   Float64    String                             DateTime             DateTime           ⋯
─────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   1 │ optimizations           1  using GridLayoutBase    1.7.1         a07b954      261358885  1.28611   0.0        /Users/juliuskrumbiegel/.julia/d…  2022-01-23T22:58:33  2022-01-23T23:04:2 ⋯
   2 │ optimizations           1  GridLayout constructor  1.7.1         a07b954       56776749  0.661936  0.0435153  /Users/juliuskrumbiegel/.julia/d…  2022-01-23T22:58:33  2022-01-23T23:04:2
   3 │ optimizations           1  Big GridLayout          1.7.1         a07b954     4690200580  8.39461   0.807336   /Users/juliuskrumbiegel/.julia/d…  2022-01-23T22:58:33  2022-01-23T23:04:2
   4 │ master                  1  using GridLayoutBase    1.7.1         d3700e2      261395013  1.37096   0.0        /Users/juliuskrumbiegel/.julia/d…  2022-01-16T11:32:20  2022-01-23T23:04:2
   5 │ master                  1  GridLayout constructor  1.7.1         d3700e2       56776749  0.64803   0.0442939  /Users/juliuskrumbiegel/.julia/d…  2022-01-16T11:32:20  2022-01-23T23:04:2 ⋯
   6 │ master                  1  Big GridLayout          1.7.1         d3700e2     4690200580  8.44522   0.815214   /Users/juliuskrumbiegel/.julia/d…  2022-01-16T11:32:20  2022-01-23T23:04:2

If you have time, I'd be glad for any enlightening thoughts you might have on this :)

This reverts commit 412e16d.

This reverts commit 8a1393a.

timholy · 2022-02-01T15:51:55Z

Thanks for picking this up! To explain the flamegraphs...unless the external (Base) code is inlined into your functions, you can't save the inference results---Julia only saves inference results for module-owned methods.

On JuliaLang/julia#43990, here's what the flamegraph looks like:

😄 On that PR I get

julia> tinf
InferenceTimingNode: 1.951009/1.951067 on Core.Compiler.Timings.ROOT() with 1 direct children

whereas on Julia 1.6 I get

InferenceTimingNode: 2.413862/2.990975 on Core.Compiler.Timings.ROOT() with 71 direct children

jkrumbiegel · 2022-02-01T19:42:27Z

Ok cool, so in your new branch basically no additional inference has to run (everything is precompiled and saved)?

I still don't understand though, why something like GridLayoutBase.compute_rowcols can't be saved, that's not a Base method. Or which Base code do you mean? In the precompilation file I explicitly call GridLayoutBase.compute_rowcols(gl, GridLayoutBase.suggestedbboxobservable(gl)[]). I thought everything with backedges to module-owned functions could be saved, but maybe I misunderstood this.

timholy · 2022-02-01T20:28:53Z

so in your new branch basically no additional inference has to run (everything is precompiled and saved)?

Yep.

why something like GridLayoutBase.compute_rowcols can't be saved

It immediately demands some Base methods, and those bars are stacked right on top of compute_rowcols. (Avoiding broadcasting might work around this?)

But you're right that the presence of compute_rowcols is weird. I've recently discovered that there's a process of stripping the inferred code under some circumstances (I haven't fully investigated which circumstances), from JuliaLang/julia#16907. Check this out:

julia> using GridLayoutBase, MethodAnalysis

julia> mis = methodinstances(GridLayoutBase.compute_rowcols)
1-element Vector{Core.MethodInstance}:
 MethodInstance for compute_rowcols(::GridLayout, ::GeometryBasics.HyperRectangle{2, Float32})

julia> mi = first(mis)
MethodInstance for compute_rowcols(::GridLayout, ::GeometryBasics.HyperRectangle{2, Float32})

julia> mi.backedges
1-element Vector{Any}:
 MethodInstance for align_to_bbox!(::GridLayout, ::GeometryBasics.HyperRectangle{2, Float32})

julia> ci = mi.cache
Core.CodeInstance(MethodInstance for compute_rowcols(::GridLayout, ::GeometryBasics.HyperRectangle{2, Float32}), #undef, 0x00000000000073d6, 0xffffffffffffffff, Tuple{GridLayoutBase.RowCols{Vector{Float32}}, GridLayoutBase.RowCols{Vector{Float32}}}, #undef, nothing, false, false, Ptr{Nothing} @0x0000000000000000, Ptr{Nothing} @0x0000000000000000)

julia> ci.inferred    # where did the inferred code go?

julia> ci.invoke      # because we loaded this from a *.ji file, we also don't have a useful pointer for the native code 
Ptr{Nothing} @0x0000000000000000

julia> begin
           gl = GridLayout()
           gl2 = GridLayout()
           gl[1, 1] = gl2
           nothing
       end

julia> ci.inferred       # stripped again!

julia> ci.invoke         # but we at least have the native code
Ptr{Nothing} @0x00007fa6e6840d00

One of the changes in my PR is to stop deleting the inferred code if you're doing precompilation, see the change in my PR to codegen.cpp.

jkrumbiegel · 2022-02-04T09:12:02Z

Interesting, I didn't know about this behavior! But looks like inference-wise this package will be in good shape after your changes landed in Julia. So I don't have to spend more time on that, although some algorithmic improvements might decrease general compilation duration. What is the current best-practice to generate a list of compilation time per function? If I understood it correctly, @snoopi_deep only separates inference from all else, but then I don't know if compilation is heavy or if there's just an algorithm computing a lot of stuff.

timholy · 2022-02-04T10:44:46Z

There's also @snoopl to measure the LLVM time. I haven't used it much myself, it was added by Nathan Daly to SnoopCompile.

timholy · 2022-02-04T10:48:07Z

Thanks for tackling this!

Lower priority, but it still might be worth making Side and GridDir enums rather than types---less specialization means shorter latencies (at least until native-code caching lands) and shorter build times (probably forever). Do you want me to resurrect that from #21? Admittedly there are much bigger fish to fry in Makie-land, so if you don't feel like this is the right time I'd understand 100%.

jkrumbiegel · 2022-02-04T12:27:12Z

I had tried those changes out and they didn't seem to have much effect in timings. Because those were breaking I reversed them then, figuring it was not worth the effort

timholy · 2022-02-04T14:44:07Z

That's reasonable!

remove gridcontent type parameterization

b5555ce

jkrumbiegel added 11 commits January 16, 2022 20:27

remove needs_update observable

f55ba48

remove unnecessary field

00e0768

make aligns convert correctly

ad9caec

simplify setproperty

f9de74f

change tellheight to Observable{Bool}

87e80ea

fix width/height types

270e5cd

make GridDir an enum

412e16d

change Side to enum

8a1393a

simplify precompilation

4631818

snoopcompile precompile script

449c54a

use simple precompilation again

a07b954

jkrumbiegel added 9 commits January 24, 2022 15:12

fix a couple of type instabilities

ff7eaa5

make size categories closed unions

45ed2fc

schematic of gridlayout updates

09ab162

Revert "make GridDir an enum"

cb575b9

This reverts commit 412e16d.

Revert "change Side to enum"

ac2fbd4

This reverts commit 8a1393a.

switch back to old types completely

eec2536

simplify filterenum

a3c8735

fix more type instabilities

912d7b3

make alignmode closed union

8286f01

jkrumbiegel merged commit cf512cb into master Feb 4, 2022

timholy mentioned this pull request Feb 8, 2022

More latency/inferrability work #21

Closed

jkrumbiegel deleted the optimizations branch February 26, 2022 09:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove gridcontent type parameterization #30

remove gridcontent type parameterization #30

jkrumbiegel commented Jan 16, 2022

codecov-commenter commented Jan 16, 2022 •

edited

Loading

jkrumbiegel commented Jan 23, 2022 •

edited

Loading

timholy commented Feb 1, 2022

jkrumbiegel commented Feb 1, 2022 •

edited

Loading

timholy commented Feb 1, 2022 •

edited

Loading

jkrumbiegel commented Feb 4, 2022

timholy commented Feb 4, 2022

timholy commented Feb 4, 2022

jkrumbiegel commented Feb 4, 2022

timholy commented Feb 4, 2022

remove gridcontent type parameterization #30

remove gridcontent type parameterization #30

Conversation

jkrumbiegel commented Jan 16, 2022

codecov-commenter commented Jan 16, 2022 • edited Loading

Codecov Report

jkrumbiegel commented Jan 23, 2022 • edited Loading

timholy commented Feb 1, 2022

jkrumbiegel commented Feb 1, 2022 • edited Loading

timholy commented Feb 1, 2022 • edited Loading

jkrumbiegel commented Feb 4, 2022

timholy commented Feb 4, 2022

timholy commented Feb 4, 2022

jkrumbiegel commented Feb 4, 2022

timholy commented Feb 4, 2022

codecov-commenter commented Jan 16, 2022 •

edited

Loading

jkrumbiegel commented Jan 23, 2022 •

edited

Loading

jkrumbiegel commented Feb 1, 2022 •

edited

Loading

timholy commented Feb 1, 2022 •

edited

Loading