Add some precompiles and improve inference #4

timholy · 2020-09-22T23:17:42Z

codecov-commenter · 2020-09-22T23:19:57Z

Codecov Report

Merging #4 into master will decrease coverage by 1.52%.
The diff coverage is 78.62%.

@@            Coverage Diff             @@
##           master       #4      +/-   ##
==========================================
- Coverage   87.22%   85.69%   -1.53%     
==========================================
  Files           9       10       +1     
  Lines         939      972      +33     
==========================================
+ Hits          819      833      +14     
- Misses        120      139      +19

Impacted Files	Coverage Δ
src/GridLayoutBase.jl	`100.00% <ø> (ø)`
src/types.jl	`100.00% <ø> (ø)`
src/layoutobservables.jl	`60.13% <38.88%> (-1.46%)`	⬇️
src/precompile.jl	`93.10% <93.10%> (ø)`
src/gridlayout.jl	`93.01% <93.33%> (-1.56%)`	⬇️
src/geometry_integration.jl	`100.00% <100.00%> (ø)`
src/gridlayoutspec.jl	`100.00% <100.00%> (ø)`
src/helpers.jl	`18.75% <100.00%> (+11.60%)`	⬆️
src/layout_engine.jl	`80.34% <0.00%> (-4.28%)`	⬇️
... and 2 more

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 8b4eedb...f132bcb. Read the comment docs.

jkrumbiegel · 2020-09-25T06:20:28Z

src/gridlayout.jl


    on(computedbboxobservable(gl)) do cbb
+        # 0.7s inference time for this anonymous function. TODO? precompile
        align_to_bbox!(gl, cbb)


@timholy That is crazy, why would such a small function take so long to infer?

It's probably really the inference time for laign_to_bbox! itself. If inference was good throughout, it would be precompiled when GridLayout(Int, Int) gets precompiled. So I marked this rather than fixing it manually. I'll take a poke at Cthulhu.descend(GridLayout, (Int, Int)) and see if I can just fix the inference problem.

Correction: it's due to the fact that on doesn't force specialization of its first argument. That's fine and how it should be.

I just pushed a couple more precompiles.

jkrumbiegel · 2020-09-25T06:22:15Z

src/precompile.jl

@@ -0,0 +1,4 @@
+function _precompile_()
+    ccall(:jl_generating_output, Cint, ()) == 1 || return nothing
+    precompile(GridLayout, (Int, Int))


Is precompile(GridLayout, (Int, Int)) the only thing you thought was worth doing, or would it make sense to precompile all functionality that the tests go over?

Nah, just a conversation-starter 😄.

This adds a number of other precompiles too

This is a breaking change, but it makes other stuff easier. Alternately, one could force-specialize on the type (which would not be breaking). But this is generally the recommended style.

timholy · 2020-09-25T14:33:46Z

OK, I think this is to a good stopping point (I'm stopping, anyway). There is still more inference you could get rid of (probably mostly better setindex! precompiles), but big chunks of what remains rely on TestDebug and obviously that's not a type you'll use in practice. As is, this shaves a bit more than 10% off the test time. Once you start looking at this in the context of Makie you will very likely want to add a few more precompiles for yet more benefits.

A few explanations are in order:

this package was already in pretty good shape, inference-wise
in the end, you do want to profile (e.g., profilng include("runtests.jl")). That was how my attention was directed to various concatenation lines as costing a fair amount in inference time.
a pretty common strategy was to split out anonymous functions so their work could be precompiled

It's also worth emphasizing that this contains a breaking change: see the modification to the LayoutObservables constructor. You can alterately use ::Type{T} rather than T::Type as an argument, and get the same benefit with a non-breaking change, but I do think the LayoutObservables{T} style is recommended in modern Julia.

Given my other duties, this is probably the last Makie-related package that I'll directly work on for a while. But if you guys want to make further progress I am happy to answer questions.

Worth emphasizing that you want to do all this work on Julia 1.6 since the invalidation picture is so much better---you'll get murky results on 1.5 because all your improvements will be invalidated.

SimonDanisch · 2020-09-25T14:46:10Z

Project.toml


 [deps]
 GeometryBasics = "5c1252a2-5f33-56bf-86c9-59e7332b4326"
+InteractiveUtils = "b77e0a4c-d291-57a0-90e8-8db25a27a240"


I'm guessing, that this was unintentional?

No, I used subtypes in precompile.jl.

Oh :D fair enough ;)

timholy · 2020-10-06T17:02:31Z

Anything blocking this from being merged? It does contain a breaking change (see above), but [compat] bounds should take care of that nicely. Update: total inference time is now cut in half (1.3s vs 2.6s), and a bunch of that is due to DebugRect or inference in Test itself.

codecov-io · 2020-10-09T08:19:28Z

Codecov Report

Merging #4 into master will decrease coverage by 1.82%.
The diff coverage is 72.83%.

@@            Coverage Diff             @@
##           master       #4      +/-   ##
==========================================
- Coverage   87.22%   85.39%   -1.83%     
==========================================
  Files           9       10       +1     
  Lines         939      993      +54     
==========================================
+ Hits          819      848      +29     
- Misses        120      145      +25

Impacted Files	Coverage Δ
src/GridLayoutBase.jl	`100.00% <ø> (ø)`
src/types.jl	`100.00% <ø> (ø)`
src/layoutobservables.jl	`58.22% <33.87%> (-3.37%)`	⬇️
src/gridlayout.jl	`92.84% <93.33%> (-1.73%)`	⬇️
src/precompile.jl	`95.45% <95.45%> (ø)`
src/geometry_integration.jl	`100.00% <100.00%> (ø)`
src/gridlayoutspec.jl	`100.00% <100.00%> (ø)`
src/helpers.jl	`23.52% <100.00%> (+16.38%)`	⬆️
src/layout_engine.jl	`80.34% <0.00%> (-4.28%)`	⬇️
... and 2 more

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 8b4eedb...bf4d275. Read the comment docs.

jkrumbiegel · 2020-10-10T08:47:16Z

thank you again for your effort Tim, I just didn't have the time to go over everything until now. I can't say that I grasp all changes completely, because there are so many all across the project, but as the tests run and I see you mostly changed "stylistic" aspects that allow easier inference, and nothing works substantially differently, I'm going to merge this

timholy · 2020-10-10T09:02:55Z

You're most welcome. Like I've said, I'd be happy to host an interactive session to show you folks my workflow for changes like these. Once you see how they are discovered, analyzed, and resolved it's all pretty straightforward.

timholy · 2020-10-13T11:43:31Z

I ended up using my cumsum/concatenation inference work here as the basis of a brief "tutorial" on precompilation, see https://discourse.julialang.org/t/understanding-precompilation-and-its-limitations-reducing-latency/48294.

Add a couple of precompiles

4edf1bb

jkrumbiegel reviewed Sep 25, 2020

View reviewed changes

timholy added 4 commits September 25, 2020 05:36

Name some anonymous functions and then precompile them

0f212a1

This adds a number of other precompiles too

Rewrite various bits to make them friendlier to inference

c62428f

Eliminate type-as-argument in LayoutObservables constructor

bd43074

This is a breaking change, but it makes other stuff easier. Alternately, one could force-specialize on the type (which would not be breaking). But this is generally the recommended style.

Enhance the precompiles

f132bcb

SimonDanisch reviewed Sep 25, 2020

View reviewed changes

timholy added 2 commits October 7, 2020 06:23

Yet more precompiles

7b495cc

Inference improvements in box alignment

bf4d275

timholy changed the title ~~Add a couple of precompiles~~ Add some precompiles and improve inference Oct 9, 2020

jkrumbiegel merged commit 4cb23ac into jkrumbiegel:master Oct 10, 2020

timholy deleted the teh/ttfp branch October 10, 2020 09:01

Add some precompiles and improve inference #4

Add some precompiles and improve inference #4

Uh oh!

Conversation

timholy commented Sep 22, 2020

Uh oh!

codecov-commenter commented Sep 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timholy commented Sep 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timholy commented Oct 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Oct 9, 2020

Codecov Report

Uh oh!

jkrumbiegel commented Oct 10, 2020

Uh oh!

timholy commented Oct 10, 2020

Uh oh!

timholy commented Oct 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Sep 22, 2020 •

edited

Loading

timholy commented Sep 25, 2020 •

edited

Loading

timholy commented Oct 6, 2020 •

edited

Loading