Ferrite-FEM / FerriteViz.jl

Plot your Ferrite.jl data
https://ferrite-fem.github.io/FerriteViz.jl/
MIT License
28 stars 9 forks source link

Reduction of Allocations -> make FerriteViz go vroom #66

Open koehlerson opened 1 year ago

koehlerson commented 1 year ago

With https://github.com/Ferrite-FEM/FerriteViz.jl/pull/63 we exploit something conceptually, however, the current implementation allocates rather much, because we run into

https://github.com/MakieOrg/Makie.jl/blob/adc9d9ae3523f746f9c32652c833a59bfc0167b2/src/conversions.jl#L581-L595

by e.g.

https://github.com/Ferrite-FEM/FerriteViz.jl/blob/59285b4e5ebeaf08f5b0dee25b181bf4d5ac96d3/src/makieplotting.jl#L95

where instead we could directly provide a GLNormalMesh which shouldn't allocate further according to Simon. If that's the case it is a bug in Makie and we could circumvent it by directly overloading recipe internals, such as draw_atomic. Besides that we could directly use Makie.Buffer to construct the GLNormalMesh in the spirit of https://docs.makie.org/stable/examples/plotting_functions/mesh/index.html#using_geometrybasicsmesh_and_buffersampler_type

termi-official commented 1 year ago

We should also propagete function barriers and try to make a non-allocating version of e.g. function_value. Also reinit seems to allocate for some reason. We also saw with Jet some possible instabilities in Ferrite.

termi-official commented 1 year ago

TODO: We also need to investigate further the tradeoff between allocating extra arrays and maintaining performant cache access patterns.

koehlerson commented 1 year ago

after https://github.com/Ferrite-FEM/FerriteViz.jl/pull/70 and https://github.com/MakieOrg/Makie.jl/pull/2874 there isn't anything further to optimize if we don't have custom shaders

Its a little bit tricky with https://github.com/MakieOrg/Makie.jl/pull/2874 because this only dispatches on Makie.mesh and not on our custom recipes as e.g. solutionplot, cellplot etc. So the only thing which comes to my mind is to do namespace pollution in FerriteViz and tell explicitly that data_limits from https://github.com/MakieOrg/Makie.jl/pull/2874 is valid for our recipes too

termi-official commented 1 year ago

I also think that the remaining "performance issue" should be fixed in Makie itself, so everyone can benefit from it. However, for the surface plots I think the speed is fine enough for now, even for large plots. The other major bottleneck for "time to first plot" is https://github.com/Ferrite-FEM/Ferrite.jl/issues/617 , which should be fixed in Ferrite, such that also everyone benefits here.

koehlerson commented 1 year ago

Yes but my question is how we can forward the data_limits dispatches that I'm trying to add in Makie. In Makie I can only dispatch on the type of the basic mesh recipe

termi-official commented 1 year ago

You mean e.g. for scatter? Or am I misunderstanding that this line https://github.com/Ferrite-FEM/FerriteViz.jl/blob/master/src/makieplotting.jl#L314 reaches the data_limits dispatch?

koehlerson commented 1 year ago

No it doesnt, since SF::FerriteViz.Surface and this line https://github.com/Ferrite-FEM/FerriteViz.jl/blob/master/src/makieplotting.jl#L69 reaches the data_limits dispatch with SP::FerriteViz.SolutionPlot and not with Makie.Mesh

koehlerson commented 10 months ago

I think this can be closed?

termi-official commented 10 months ago

Not sure. I think we should add some kind of benchmark here to monitor perf before marking this as resolved. What do you think?