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

Reduction of Allocations -> make FerriteViz go vroom #66

Open
koehlerson opened this issue Mar 9, 2023 · 9 comments · Fixed by #69
Open

Reduction of Allocations -> make FerriteViz go vroom #66

koehlerson opened this issue Mar 9, 2023 · 9 comments · Fixed by #69
Labels
help wanted Extra attention is needed refactor Code structure and transformations

Comments

@koehlerson
Copy link
Member

With #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.

return Makie.mesh!(CP, coords, plotter.triangles, color=solution, shading=CP[:shading], scale_plot=CP[:scale_plot], colormap=CP[:colormap], transparent=CP[:transparent])

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

@koehlerson koehlerson added help wanted Extra attention is needed refactor Code structure and transformations labels Mar 9, 2023
@termi-official
Copy link
Member

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
Copy link
Member

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

@koehlerson koehlerson linked a pull request Mar 16, 2023 that will close this issue
@koehlerson koehlerson reopened this Mar 23, 2023
@koehlerson
Copy link
Member Author

after #70 and MakieOrg/Makie.jl#2874 there isn't anything further to optimize if we don't have custom shaders

Its a little bit tricky with MakieOrg/Makie.jl#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 MakieOrg/Makie.jl#2874 is valid for our recipes too

@termi-official
Copy link
Member

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 Ferrite-FEM/Ferrite.jl#617 , which should be fixed in Ferrite, such that also everyone benefits here.

@koehlerson
Copy link
Member Author

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
Copy link
Member

termi-official commented Apr 21, 2023

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
Copy link
Member Author

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
Copy link
Member Author

I think this can be closed?

@termi-official
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed refactor Code structure and transformations
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants