Time-to-first-plot improvements #1278

non-Jedi · 2019-04-19T02:47:05Z

These two changes give close to a 20% speedup in time-to-first-plot in
my informal bench-marking. Unfortunately, this produces different
output on some of the regression suite (thus the WIP); things aren't
getting drawn in the same order as before (see below). A total of 17
images are different, and more svgs than that are structured
differently without actually having visible differences. Don't worry;
I'll figure it out.

auto-enumerate
colored_smoot_lm
density2d
hair
histogram_errorbar
issue975
layer_leak
layer_order
multi_geom_layer
nan_skipping
smooth_lm
stat_binmean
subplot_grid
subplot_grid_free_axis
subplot_grid_free_y_2
subplot_layers
unitful_basic

The changes here are just my first fumbling attempts to address the
profiling in #1275. I honestly expected the first commit in this pack
to have a bigger effect than it did, and I expected the nothing
comparisons to be negligible when in reality they were more important
than getting rid of the splatting. Pushing this now as WIP since I
don't know when I'll be able to get back to this: could be tomorrow,
or it could be weeks.

Contributor checklist:

I've updated the documentation to reflect these changes
I've added an entry to NEWS.md
I've added and/or updated the unit tests
I've run the regression tests
I've squash'ed or fixup'ed junk commits with git-rebase
I've built the docs and confirmed these changes don't cause new errors

Proposed changes

non-Jedi · 2019-04-19T03:14:46Z

Just noticed some other miscellaneous refactoring accidentally made it into the first commit other than its stated purpose of eliminating splatting. I think they're all changes for the good, but I can rebase them out if you'd prefer a cleaner review here.

codecov-io · 2019-04-19T03:19:06Z

Codecov Report

Merging #1278 into master will increase coverage by 0.03%.
The diff coverage is 96.35%.

@@            Coverage Diff             @@
##           master    #1278      +/-   ##
==========================================
+ Coverage   85.83%   85.86%   +0.03%     
==========================================
  Files          36       36              
  Lines        4158     4161       +3     
==========================================
+ Hits         3569     3573       +4     
+ Misses        589      588       -1

Impacted Files	Coverage Δ
src/geometry.jl	`75% <ø> (ø)`	⬆️
src/data.jl	`0% <0%> (ø)`	⬆️
src/misc.jl	`62.58% <0%> (+0.16%)`	⬆️
src/varset.jl	`97.77% <100%> (ø)`	⬆️
src/geom/point.jl	`100% <100%> (ø)`	⬆️
src/dataframes.jl	`85.71% <100%> (ø)`	⬆️
src/mapping.jl	`83.01% <100%> (ø)`	⬆️
src/geom/bar.jl	`91.85% <100%> (ø)`	⬆️
src/coord.jl	`84.53% <100%> (ø)`	⬆️
src/geom/hvabline.jl	`93.84% <100%> (ø)`	⬆️
... and 13 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 9f5f621...bf5bd2e. Read the comment docs.

Mattriks · 2019-04-19T03:20:48Z

src/geom/point.jl

@@ -67,7 +67,7 @@ function render(geom::PointGeometry, theme::Gadfly.Theme, aes::Gadfly.Aesthetics
    for (x, y, color, size, shape, alpha) in Compose.cyclezip(aes.x, aes.y, aes.color, aes.size, aes.shape, aes_alpha)
        shapefun = typeof(shape) <: Function ? shape : theme.point_shapes[shape]
        sizeval = typeof(size) <: Int ? interpolate_size(size) : size
-        strokecolor = aes.color_key_continuous != nothing && aes.color_key_continuous ?
+        strokecolor = issomething(aes.color_key_continuous) && aes.color_key_continuous ?


Note PR #1258 which rewrites render for PointGeometry.

How close is that PR to merging? I can either rebase this on top of it, you can rebase on top of this, or we can wait and see whichever gets merged first. I did notice you used != nothing a few times in your rewrite; whatever we do, we should make sure that those are changed to issomething before they make it to master.

bjarthur · 2019-04-19T11:01:55Z

thanks for you work on this dire problem! i'd been assuming it was hopeless from a pkg dev point of view and that only changes to julia base could fix it.

non-Jedi · 2019-04-19T18:34:58Z

Well, I'm down to 7 that are different now (all but one entirely new from the previous list):

and to be honest for some of them I'm not seeing the difference. Any thoughts? I'll keep working. Examples:

colorful_hist (fixed)

No idea what's going on here.

master

devel

diff

hexbin_dark (fixed)

something different about the colors maybe?

master

devel

diff

stacked_continuous_histogram (fixed)

no idea

master

devel

diff

subplot_grid_free_y_2

Scales are off on this one. Weird that it's the only one that seems to have this problem.

master

devel

diff

non-Jedi · 2019-04-20T01:41:53Z

All regression tests now pass. I'm gonna run some stuff to make pretty graphs showing the improvement as is only proper when working on Gadfly.

non-Jedi · 2019-04-20T16:42:35Z

So the results weren't as dramatic for these particular changes running on my desktop versus running on my laptop. Still a nice little ~4% bump though. I threw the data from running through all the testscripts up on Github if anyone is curious enough to take a closer look.

Or perhaps more telling (I think the cluster around 10 seconds is because I had forgotten to make CSV or some other test dependency available):

So to get below 10 seconds or so for most plots, I only need to make similar improvements another (0.95^28 * 40 seconds = 9.5 seconds)... 27 times... Hopefully some of my other ideas will be a bit more fruitful.

bjarthur · 2019-04-21T12:52:51Z

lgtm. 4% is better than nothing.

where is issomething defined? i can't find it!

you might consider a PR to the "performance tips" of the julia docs. there is no mention of isnothing being faster and splats being slow.

@Mattriks you ready to merge #1258 ?

non-Jedi · 2019-04-22T12:49:56Z

Both isnothing and issomething are actually defined in Gadfly's misc.jl which is necessary to support 1.0 since isnothing wasn't introduced to Base until 1.1. Which reminds me: the Base implementation uses type dispatch instead of ===. I should change misc.jl to match to see if we can get a tiny bit of additional speedup there.

non-Jedi · 2019-04-22T12:57:44Z

Force pushed with changes mentioned above to isnothing and a rebase on master to resolve conflicts with #1276.

src/misc.jl

This gives a marginal improvement on speed and memory use during first plot, but the biggest benefit is that it makes the code easier to read and the profiling significantly easier to grok.

non-Jedi · 2019-04-22T23:20:20Z

CI failure is unrelated. I'll try to PR a version bump of Documenter in the next few days to fix doc building failure.

bjarthur · 2019-04-23T22:01:38Z

perhaps CI failure for docs is due to forgetting to fold docs/Project.toml into ./Project.toml in #1277 ?

src/geom/rectbin.jl

Comparing nothing by value (==, =!) rather than by identity (===, !==) or by type (isnothing) can have negative consequences on code speed. This commit changes all nothing comparisons to use the isnothing utility added in Julia 1.1 and Compat 2.1.

non-Jedi · 2019-04-23T23:24:12Z

docs/Project.toml doesn't have a way to be folded into ./Project.toml unfortunately. The problem is that Documenter 0.20 has a hard version cap on DocumentStringExtensions of 0.2 which conflicts with current requirements in ./Project.toml. Should be fixable by bumping Documenter to 0.22.

src/coord.jl

ararslan · 2019-05-05T05:22:26Z

src/Gadfly.jl

@@ -388,14 +388,14 @@ function render_prepare(plot::Plot)
    # they are missing.
    datas = Array{Data}(undef, length(plot.layers))
    for (i, layer) in enumerate(plot.layers)
-        if layer.data_source === nothing && isempty(layer.mapping)
+        if isnothing(layer.data_source) && isempty(layer.mapping)


I'd be willing to bet === nothing is more efficient. The compiler loves ===.

Simply changing occurrences of == nothing to use === (and likewise for the negative) should provide a nice improvement.

For reference, this issue applies: JuliaLang/julia#27681

Good to know. I'll update with straight === and see if it makes a difference.

That would also resolve the isnothing name qualification discussion.

I've pushed a commit changing from isnothing to === nothing. Still need to get back to the computer I've benchmarked on before to see if it makes a difference.

@ararslan

Per comment from @ararslan, the Julia compiler may prefer === versus isnothing; improvements to be confirmed with benchmarking.

Mattriks reviewed Apr 19, 2019

View reviewed changes

non-Jedi force-pushed the benchmark2 branch from 844afc5 to e9a991a Compare April 19, 2019 18:14

non-Jedi force-pushed the benchmark2 branch 3 times, most recently from da4a328 to c5a6136 Compare April 20, 2019 01:39

non-Jedi changed the title ~~WIP: Time-to-first-plot improvements~~ Time-to-first-plot improvements Apr 20, 2019

non-Jedi force-pushed the benchmark2 branch from c5a6136 to e12089f Compare April 22, 2019 12:56

bjarthur reviewed Apr 22, 2019

View reviewed changes

src/misc.jl Outdated Show resolved Hide resolved

Remove unnecessary splatting from internal interfaces

0f11ad0

This gives a marginal improvement on speed and memory use during first plot, but the biggest benefit is that it makes the code easier to read and the profiling significantly easier to grok.

non-Jedi force-pushed the benchmark2 branch 6 times, most recently from 45f7b14 to 3470acc Compare April 22, 2019 22:14

non-Jedi mentioned this pull request Apr 23, 2019

Use snoopcompile to more aggressively precompile Gadfly #1280

Open

6 tasks

bjarthur reviewed Apr 23, 2019

View reviewed changes

src/geom/rectbin.jl Outdated Show resolved Hide resolved

non-Jedi force-pushed the benchmark2 branch from 3470acc to bf5bd2e Compare April 23, 2019 23:20

bjarthur reviewed Apr 27, 2019

View reviewed changes

src/coord.jl Outdated Show resolved Hide resolved

ararslan reviewed May 5, 2019

View reviewed changes

Compare nothing values with ===

80ae3df

Per comment from @ararslan, the Julia compiler may prefer === versus isnothing; improvements to be confirmed with benchmarking.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Time-to-first-plot improvements #1278

Time-to-first-plot improvements #1278

non-Jedi commented Apr 19, 2019 •

edited

Loading

non-Jedi commented Apr 19, 2019 •

edited

Loading

codecov-io commented Apr 19, 2019 •

edited

Loading

Mattriks Apr 19, 2019 •

edited

Loading

non-Jedi Apr 19, 2019

bjarthur commented Apr 19, 2019

non-Jedi commented Apr 19, 2019 •

edited

Loading

non-Jedi commented Apr 20, 2019

non-Jedi commented Apr 20, 2019 •

edited

Loading

bjarthur commented Apr 21, 2019

non-Jedi commented Apr 22, 2019

non-Jedi commented Apr 22, 2019

non-Jedi commented Apr 22, 2019

bjarthur commented Apr 23, 2019 •

edited

Loading

non-Jedi commented Apr 23, 2019

ararslan May 5, 2019

ararslan May 5, 2019

ararslan May 5, 2019

non-Jedi May 6, 2019

ararslan May 6, 2019

non-Jedi May 8, 2019

Time-to-first-plot improvements #1278

Are you sure you want to change the base?

Time-to-first-plot improvements #1278

Conversation

non-Jedi commented Apr 19, 2019 • edited Loading

Contributor checklist:

Proposed changes

non-Jedi commented Apr 19, 2019 • edited Loading

codecov-io commented Apr 19, 2019 • edited Loading

Codecov Report

Mattriks Apr 19, 2019 • edited Loading

Choose a reason for hiding this comment

non-Jedi Apr 19, 2019

Choose a reason for hiding this comment

bjarthur commented Apr 19, 2019

non-Jedi commented Apr 19, 2019 • edited Loading

colorful_hist (fixed)

master

devel

diff

hexbin_dark (fixed)

master

devel

diff

stacked_continuous_histogram (fixed)

master

devel

diff

subplot_grid_free_y_2

master

devel

diff

non-Jedi commented Apr 20, 2019

non-Jedi commented Apr 20, 2019 • edited Loading

bjarthur commented Apr 21, 2019

non-Jedi commented Apr 22, 2019

non-Jedi commented Apr 22, 2019

non-Jedi commented Apr 22, 2019

bjarthur commented Apr 23, 2019 • edited Loading

non-Jedi commented Apr 23, 2019

ararslan May 5, 2019

Choose a reason for hiding this comment

ararslan May 5, 2019

Choose a reason for hiding this comment

ararslan May 5, 2019

Choose a reason for hiding this comment

non-Jedi May 6, 2019

Choose a reason for hiding this comment

ararslan May 6, 2019

Choose a reason for hiding this comment

non-Jedi May 8, 2019

Choose a reason for hiding this comment

non-Jedi commented Apr 19, 2019 •

edited

Loading

non-Jedi commented Apr 19, 2019 •

edited

Loading

codecov-io commented Apr 19, 2019 •

edited

Loading

Mattriks Apr 19, 2019 •

edited

Loading

non-Jedi commented Apr 19, 2019 •

edited

Loading

non-Jedi commented Apr 20, 2019 •

edited

Loading

bjarthur commented Apr 23, 2019 •

edited

Loading