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

Compute primitive variables once before computing the volume integral etc. #54

Open
ranocha opened this issue Aug 18, 2020 · 6 comments

Comments

@ranocha
Copy link
Member

ranocha commented Aug 18, 2020

In GitLab by @ranocha on May 18, 2020, 14:37

This could be a possible optimization to speed up the code.

@ranocha
Copy link
Member Author

ranocha commented Aug 18, 2020

In GitLab by @sloede on May 20, 2020, 09:48

My initial feeling is that we should only do this if it brings a significant speedup. I don't like that it will increase complexity, in that it will add another piece of implicit knowledge that everyone has to keep in mind (i.e., which quantities are stored in primitive and which in conservative state).

I might be biased, though, since I have used a large code which used both primitive and conservative variables side-by-side before, and it was a nightmare in terms of comprehensibility (especially for new users). Once they introduced primitive variables at a global scope, people started to use primitive variables also in other areas, such as stateful BCs, intermediate computations etc. Thus when working with another piece of code that was not written by yourself, you always had to make sure that you first check if they are doing their algorithms in primitive or conservative state.

@ranocha
Copy link
Member Author

ranocha commented Nov 3, 2021

Some micro-benchmarks of computing flux differencing volume terms suggest that the benefits will be between 20% and 50% for the compressible Euler equations.

image

Left: flux_shima_etal
Right: flux_ranocha

@gregorgassner
Copy link
Contributor

Hm, I wonder if there is a feasible way to have an optimized (more complicated) RHS side-by-side to the standard RHS we have right now.

50% seems to be a lot, so it is not easy to ignore. 20% I would say is maybe not worth all the hassle.

I personally am thinking about globally storing the primitive variables as well (as I guess is done e.g. in FLASH). Of course, this has some impact on the storage, although the impact is relative smaller for curvilinear grids (where the metric terms need also a lot of memory) compared to the Cartesian solvers. Then one could decide to either use the prim, the cons or a mixture of both whenever appropropriate. The collocation methods we are using are ideally suited for such an approach.

However, I need to think more about it...

@ranocha
Copy link
Member Author

ranocha commented Nov 3, 2021

Yes, we should definitely move this up on our priority list. For cheap fluxes such as flux_shima_etal, the performance gain is ca. 50% to 40%. For EX fluxes, it seems to be around 25%.

@ranocha
Copy link
Member Author

ranocha commented Nov 3, 2021

When we do that, it would also be great to think about more general ways to handle coefficients, see #358 (comment).

@ranocha
Copy link
Member Author

ranocha commented Nov 3, 2021

For the specific case of flux differencing volume terms, we could also just store primitive variables per element and invoke the split_form_kernel! afterwards. That's what I did for these preliminary performance micro-benchmarks.

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

No branches or pull requests

2 participants