Skip to content
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

Improve performance around Axis(3) limits #2115

Merged
merged 13 commits into from
Jul 20, 2022

Conversation

jkrumbiegel
Copy link
Member

Seems like performance slows quite a bit if many plots are added in sequence to an Axis. Because each plot triggers a resize_limits!, first of all the limits have to be recomputed for all previous plots when a new one is added, and this also triggers tick changes, which trigger layout changes, etc. So all in all this can be quite slow. This PR adds an option to disable the reset_limits! call, which should mainly be interesting for workflows where all plots are added in one go, without a person looking at the progress. So for example this should probably be used in AlgebraOfGraphics @piever .

Also, I noticed dynamic dispatch in the code path that computes the limits in the first place and tried to fix that. I'm not sure if the logic is perfectly right with regards to NaNs, that behavior can be a bit tricky to pin down. For example, what if there's a NaN only in one dimension of a point, do we skip that whole point or use the remaining components. This PR does the latter.

Here's some examples how the PR improves performance:

Many plots in sequence

Before: 4.999897 seconds (206.07 M allocations: 6.235 GiB, 14.02% gc time)
After: 0.255500 seconds (2.88 M allocations: 151.533 MiB)

f = Figure()
ax = Axis(f[1, 1])
ax.reset_limits_on_plot = false # new feature
@time for i in 1:500
    lines!(ax, randn(1000) .+ 5i)
end
ax.reset_limits_on_plot = true # new feature
reset_limits!(ax)
f

Single plot

Before: 0.176131 seconds (10.00 M allocations: 305.179 MiB)
After: 0.012864 seconds (92 allocations: 4.203 KiB)

f = Figure()
ax = Axis(f[1, 1])
lines!(cumsum(randn(1_000_000)))
@time reset_limits!(ax)
f

Type of change

Delete options that do not apply:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • Added an entry in NEWS.md (for new features and breaking changes)
  • Added or changed relevant sections in the documentation
  • Added unit tests for new algorithms, conversion methods, etc.
  • Added reference image tests for new plotting functions, recipes, visual options, etc.

@MakieBot
Copy link
Collaborator

MakieBot commented Jul 5, 2022

Compile Times benchmark

Note, that these numbers may fluctuate on the CI servers, so take them with a grain of salt. All benchmark results are based on the mean time and negative percent mean faster than the base branch. Note, that GLMakie + WGLMakie run on an emulated GPU, so the runtime benchmark is much slower. Results are from running:

using_time = @ctime using Backend
# Compile time
create_time = @ctime fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @ctime Makie.colorbuffer(display(fig))
# Runtime
create_time = @benchmark fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @benchmark Makie.colorbuffer(display(fig))
using create display create display
GLMakie 13.84s (13.39, 15.26) 0.64+- 18.09s (17.62, 19.86) 0.79+- 16.13s (15.51, 17.51) 0.72+- 15.43ms (14.64, 16.58) 0.82+- 82.65ms (81.32, 85.09) 1.38+-
master 14.02s (13.49, 15.03) 0.57+- 18.37s (17.79, 20.01) 0.83+- 16.24s (15.42, 17.73) 0.87+- 18.40ms (17.53, 19.38) 0.79+- 82.87ms (80.65, 85.31) 1.93+-
evaluation -1.36%, -0.19s invariant (-0.31d, 0.57p, 0.61std) -1.56%, -0.28s invariant (-0.35d, 0.53p, 0.81std) -0.71%, -0.11s invariant (-0.14d, 0.79p, 0.79std) -19.26%, -2.97ms faster✅ (-3.70d, 0.00p, 0.80std) -0.26%, -0.21ms invariant (-0.13d, 0.82p, 1.66std)
CairoMakie 15.30s (14.13, 16.29) 0.83+- 23.86s (22.43, 25.38) 1.22+- 3.08s (2.85, 3.36) 0.22+- 30.31ms (28.60, 31.64) 1.14+- 28.56ms (27.29, 31.10) 1.55+-
master 14.51s (13.61, 16.06) 1.01+- 23.73s (22.02, 26.23) 1.71+- 3.00s (2.72, 3.38) 0.28+- 32.71ms (31.19, 34.01) 1.23+- 29.30ms (27.67, 31.21) 1.36+-
evaluation +5.14%, 0.79s noisy🤷‍♀️ (0.85d, 0.14p, 0.92std) +0.51%, 0.12s invariant (0.08d, 0.88p, 1.47std) +2.62%, 0.08s invariant (0.33d, 0.55p, 0.25std) -7.91%, -2.4ms faster✅ (-2.02d, 0.00p, 1.18std) -2.61%, -0.74ms invariant (-0.51d, 0.36p, 1.45std)
WGLMakie 19.28s (18.98, 19.76) 0.26+- 37.52s (36.86, 38.60) 0.65+- 52.45s (51.43, 53.84) 0.91+- 26.13ms (25.25, 27.19) 0.72+- 1.89s (1.84, 1.94) 0.03+-
master 19.21s (18.93, 19.66) 0.27+- 37.18s (36.54, 38.08) 0.62+- 52.08s (51.50, 53.40) 0.67+- 30.20ms (28.84, 31.26) 0.84+- 1.86s (1.82, 1.91) 0.03+-
evaluation +0.37%, 0.07s invariant (0.27d, 0.62p, 0.26std) +0.90%, 0.34s invariant (0.53d, 0.34p, 0.64std) +0.71%, 0.37s invariant (0.46d, 0.40p, 0.79std) -15.56%, -4.07ms faster✅ (-5.19d, 0.00p, 0.78std) +1.74%, 0.03s invariant (0.99d, 0.09p, 0.03std)

@SimonDanisch
Copy link
Member

I still think we should only update limits on display, before we add the workaround of disabling limit calculation to all kind of 3rd party libraries... This could be quite the easy change, no?

@jkrumbiegel
Copy link
Member Author

How does the Axis know it's being displayed?

@SimonDanisch
Copy link
Member

Adding something like

update_state_before_display!(figlike) # or some better name

Here: https://github.com/JuliaPlots/Makie.jl/blob/master/src/display.jl#L62=
And overload that function appropriately for figure, scene, axis etc, so that it gets called for all axes in a figure.

@SimonDanisch
Copy link
Member

Or better in backend_display + backend_show actually...

@jkrumbiegel
Copy link
Member Author

But is it better that display becomes a state mutating thing then? I'm not sure

@Moelf
Copy link
Contributor

Moelf commented Jul 5, 2022

I think compute on display is reasonable, display is already non-pure (I/O).

One problem I can think of is: do people use reflection (to read axis limits) before display in order to do something? I hope the answer is no

@jkrumbiegel
Copy link
Member Author

jkrumbiegel commented Jul 5, 2022

In that case they could always call reset_limits! themselves I guess?

And yes it's not pure, I meant that the Figure is not modified by being displayed currently. And this way you'd add limit reset overhead to every display call.

@ffreyer
Copy link
Collaborator

ffreyer commented Jul 5, 2022

Wouldn't update on display be an issue when plotting in the repl? I.e. starting with fig = Figure() in the REPL you'd display immediately and need another display or reset_limit! at the end to update limits.

Another option to throttle these kind of updates would be to generalize screen.render_tick from GLMakie to all backends and use it as a trigger. That would reduce things to one update per frame but it would require some bookkeeping to decide what needs to be updates.

@SimonDanisch
Copy link
Member

Another option to throttle these kind of updates would be to generalize screen.render_tick from GLMakie to all backends and use it as a trigger. That would reduce things to one update per frame but it would require some bookkeeping to decide what needs to be updates.

I don't know how that should work, since no other backend has the notion of frames.

And this way you'd add limit reset overhead to every display call.

Well, I'd say its more like removing the overhead of calling reset_limits! for every other operation ;) display seems to be the best place to call it one time, and I guess we can still leave an option to disable it completely, for people that absolutely cant afford an overhead like that?

@jkrumbiegel
Copy link
Member Author

jkrumbiegel commented Jul 7, 2022

Could we do it such that in the plot! call where reset_limits! is currently called every time we check if any parent Scene of the Axis is currently displayed? If yes, we reset limits. If not, we don't (most programmatic cases).

And additionally the update_state_before_display! thing that Simon suggested.

This leaves the case where someone displays a window and wants to quickly add 1000 lines to it. I still think there should be some way to disable that, no?

@SimonDanisch
Copy link
Member

The only situation I can think of why one would need to calculate limits before display, is if someone was querying them to use it in other plots. This could trigger a reset_limits! , but otherwise no limits need to get updated before we display anything.
As Julius points out, we have a couple of other situations after display. We could:

  1. manual call update after we add new plots after display
  2. always update limits after display (which we could do here: https://github.com/JuliaPlots/Makie.jl/blob/master/src/scenes.jl#L408=)
  3. do block for batching multiple calls with only one update

I guess we've been defaulting lately to slower but more user friendly solutions, so I guess we should go for 2 + 3.

@piever
Copy link
Contributor

piever commented Jul 7, 2022

As a reference point, AlgebraOfGraphics added a do block method to batch layout updates when creating a facet plot (see here). I think that's a much nicer interface than manually calling updating functions.

@jkrumbiegel
Copy link
Member Author

I've changed the implementation to update_state_before_display! and reset if the Axis scene or any of its parents are open.

However, there's still the problem of all the functions that can save a scene. save, record, colorbuffer, VideoStream, are there more? Do all of them need a separate update_state_before_display! inserted?

@SimonDanisch
Copy link
Member

Most of them should use backend_display / backend_show under the hood... I can take a look if you want

@jkrumbiegel
Copy link
Member Author

Problem is that that's already Scene, and we don't have the back link to Axis

@jkrumbiegel
Copy link
Member Author

Tests pass now, question is if all the places to call update_state_before_display! make sense.

@SimonDanisch
Copy link
Member

Guess it would be good to check the docs before merging this ;)

@SimonDanisch SimonDanisch merged commit 0c1c305 into master Jul 20, 2022
@SimonDanisch SimonDanisch deleted the jk/axis-limits-improvements branch July 20, 2022 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants