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

[BUG] marker_z option is incredibly slow in latest Plots.jl releases #3000

Closed
juliohm opened this issue Sep 21, 2020 · 13 comments
Closed

[BUG] marker_z option is incredibly slow in latest Plots.jl releases #3000

juliohm opened this issue Sep 21, 2020 · 13 comments
Labels

Comments

@juliohm
Copy link

juliohm commented Sep 21, 2020

In the last 3 weeks some update in some indirect dependency of GeoStats.jl bumped the version of Plots.jl in the documentation build to 1.x and out of the sudden my documentation cannot be built:

] dev GeoStats
; cd .julia/dev/GeoStats/docs
] activate .
] instantiate
include("make.jl")

The issue seems to be related to SVG output, but I am not sure. Can you please help address this blocking issue? I was able to build the docs by removing all markdown pages that contained plot commands.The build was working just fine 4 weeks ago.

@juliohm juliohm added the bug label Sep 21, 2020
@BeastyBlacksmith
Copy link
Member

As a quick remedy you can pin the Plots version to the one that worked

@juliohm
Copy link
Author

juliohm commented Sep 21, 2020

I will end up doing this. Meanwhile, it seems that after hours waiting the documentation is being built. The update to Plots.jl apparently made the documentation build extremely slow, but I am waiting a few more hours here before I can conclude that.

@juliohm
Copy link
Author

juliohm commented Mar 1, 2021

@BeastyBlacksmith could you please help with this issue? I am still in a setup where the documentation with Plots.jl examples takes hours to finish. Pinning to an old version of Plots.jl is a suboptimal solution.

@juliohm
Copy link
Author

juliohm commented Mar 2, 2021

Anything that changed recently regarding the option marker_z in the GR backend that may have affected performance considerably with large point clouds? I think the plots I am having issue with are the ones where I try to do a colored scatter, but I am not sure. Still investigating this.

@juliohm
Copy link
Author

juliohm commented Mar 2, 2021

Ok, here is a MWE with the exact issue with marker_z incredibly slow: https://gist.github.com/juliohm/de0ceeb47b9076b626d9a0a793c658d6

@juliohm juliohm changed the title [BUG] Plots.jl is causing Documenter.jl build to hang [BUG] marker_z option is incredibly slow in latest Plots.jl releases Mar 2, 2021
@daschw
Copy link
Member

daschw commented Mar 2, 2021

I'm afraid I introduced this performance regression in #2940. The good news is that #3320 provides a fix.

@juliohm
Copy link
Author

juliohm commented Mar 2, 2021

Looking forward to seeing the PR merged.

cc: @jheinen

@jheinen
Copy link
Member

jheinen commented Mar 2, 2021

I'm not sure whether this will fix the performance regression. The get_* function in gr_draw_marker(...) cause too much load within the GR run-time.

    gr_set_bordercolor(get_markerstrokecolor(series, i));
    gr_set_markercolor(get_markercolor(series, clims, i));
    gr_set_transparency(get_markeralpha(series, i))

The problem is, that the colors are not indices into a pre-allocated color map.

I started implementing two additional GDP functions in GR which draw lines segments (with given line widths and colors) and markers (with given sizes and colors). For Plots this means, that there is only one ccall into a GR function.

Screen Shot 2021-03-02 at 18 11 17

I just finished the GKSTerm implementation. With this method, you can draw thousands of markers in milliseconds: 6.934 ms (6 allocations: 1.37 MiB)

using GR
using BenchmarkTools

setviewport(0, 1, 0, 1)
setcolormap(44)
setmarkertype(-5)

N = 60000
w = 3 .* round.(Int, rand(N) .* 100)
c = round.(Int, rand(N) .* 255) .+ 1000
for i in 1:N
    c[i] = inqcolor(c[i])
end
a = vec(hcat(w, c)')

function test()
    clearws()
    x = rand(N)
    y = rand(N)
    gdp(x, y, 3, a)
    updatews()
end

@btime test()

Screen Shot 2021-03-02 at 18 15 50

Please understand, that it will take some time to add this functionality to all GR/GKS drivers (Qt5, PDF, PS, SVG, Cairo, ...)

@juliohm
Copy link
Author

juliohm commented Mar 2, 2021

Thank you @jheinen for the amazing and quick response as usual. You mean that the new functionality implemented in GR will take time to make it into a release? What about the fix in the Plots.jl side, would it be quick after the GR functionality is released?

As a side comment, I really think that a project like Plots.jl needs more through testing and benchmarks. The current situation where a major regression like this is invisible to the maintainers is very concerning, specially nowadays with all the infrastructure around BenchmarkCI.jl, PkgBenchmark.jl etc. See Meshes.jl for an example of a working setup where we configured a label "run benchmark" that runs benchmarks in any PR of interest.

@jheinen
Copy link
Member

jheinen commented Mar 2, 2021

My plan is to publish a new release in 2-3 weeks. A colleague of mine is working on user-defined fonts. The above functionality will be finished then, too. There will probably only be few changes on the Plots.jl side. I think it will take 2-3 hours including testing.

For GR, we operate a complex CI/CD system on our servers with visual regression tests. But, you are right, we need even more tests and run benchmarks ...

@juliohm
Copy link
Author

juliohm commented Mar 2, 2021

Yes, GR is simply amazing. Super smooth and consistent experience.

@juliohm
Copy link
Author

juliohm commented Mar 11, 2021

@jheinen thank you for taking care of this issue. I bumped into it again today, and realized how hard my life would be without your continuous help. Hope the fixes are coming along as planned.

@BeastyBlacksmith
Copy link
Member

fixed in #3320

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants