compare meshes, unify signatures, and improve performance #550

ranocha · 2021-04-16T15:38:19Z

I used the following code to compare the performance of our different mesh types (running Julia v1.6 with julia --check-bounds=no --threads=1).

julia> using Trixi, BenchmarkTools

julia> begin
           polydeg = 3
           tspan = (0.0, 1.0e-10)

           for elixir in ["benchmark/elixir_2d_euler_vortex_tree.jl",
                          "benchmark/elixir_2d_euler_vortex_structured.jl",
                          "benchmark/elixir_2d_euler_vortex_unstructured.jl",
                          #"benchmark/elixir_3d_euler_source_terms_tree.jl",
                          #"benchmark/elixir_3d_euler_source_terms_structured.jl",
                          ]
               println("#"^80)
               println(elixir)
               redirect_stdout(devnull) do
                   trixi_include(elixir; polydeg, tspan)
               end
               du_ode = similar(ode.u0); du = Trixi.wrap_array(du_ode, semi)
               u_ode = copy(ode.u0); u = Trixi.wrap_array(u_ode, semi)
               display(@benchmark Trixi.rhs!($(du_ode), $(u_ode), $(semi), $(first(tspan))))
               display(@benchmark Trixi.calc_volume_integral!($du, $u, $mesh,
                           $(Trixi.have_nonconservative_terms(equations)),
                           $equations, $(solver.volume_integral), $solver,  $(semi.cache)))
               if mesh isa CurvedMesh
                   display(@benchmark Trixi.calc_interface_flux!($(semi.cache), $u, $mesh, $equations, $solver))
               else
                   display(@benchmark Trixi.prolong2interfaces!($(semi.cache), $u, $mesh, $equations, $solver))
                   display(@benchmark Trixi.calc_interface_flux!($(semi.cache.elements.surface_flux_values),
                               $mesh, $(Trixi.have_nonconservative_terms(equations)),
                               $equations, $solver, $(semi.cache)))
               end
           end
       end

Using the changes made in this PR, I get (showing only the benchmarks for rhs!)

################################################################################
benchmark/elixir_euler_vortex_tree.jl
BenchmarkTools.Trial: 
  memory estimate:  544 bytes
  allocs estimate:  8
  --------------
  minimum time:     97.368 μs (0.00% GC)
  median time:      99.162 μs (0.00% GC)
  mean time:        100.917 μs (0.00% GC)
  maximum time:     250.850 μs (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     1
################################################################################
benchmark/elixir_euler_vortex_structured.jl
BenchmarkTools.Trial: 
  memory estimate:  544 bytes
  allocs estimate:  8
  --------------
  minimum time:     99.542 μs (0.00% GC)
  median time:      100.742 μs (0.00% GC)
  mean time:        102.777 μs (0.00% GC)
  maximum time:     262.146 μs (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     1
################################################################################
benchmark/elixir_euler_vortex_unstructured.jl
BenchmarkTools.Trial: 
  memory estimate:  768 bytes
  allocs estimate:  9
  --------------
  minimum time:     130.149 μs (0.00% GC)
  median time:      132.214 μs (0.00% GC)
  mean time:        135.632 μs (0.00% GC)
  maximum time:     307.331 μs (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     1

On main without the change from FluxRotated(flux_lax_friedrichs) to flux_lax_friedrichs with a specialization of flux to normal::AbstractVector instead of orientation::Integer, the CurvedMesh has an additional overhead of ca. 25 μs, which is roughly 25% of the total time for rhs!.

The UnstructuredMesh has the variant of FluxRotated hard-coded right now, so it didn't benefit immediately. Hence, we should switch to the same version used for the CurvedMesh.

Results from Rocinante using one thread at `ccafced`

ID	time ratio	memory ratio
`["2d", "elixir_euler_nonperiodic_curved.jl", "p3_rhs!"]`	0.87 (5%) ✅	1.00 (1%)
`["2d", "elixir_euler_nonperiodic_curved.jl", "p7_rhs!"]`	0.92 (5%) ✅	1.00 (1%)
`["3d", "elixir_euler_nonperiodic_curved.jl", "p3_rhs!"]`	0.66 (5%) ✅	1.00 (1%)
`["3d", "elixir_euler_nonperiodic_curved.jl", "p7_rhs!"]`	0.78 (5%) ✅	1.00 (1%)

TODO

Adapt all 1D, 2D (serial and parallel), and 3D signatures of functions called in rhs! to dispatch on the mesh
Implement fluxes dispatching on normal::AbstractVector to improve performance
Get rid of FluxRotated in elixirs (but keep one example for CI)
Add some docs (curvilinear meshes, FluxRotated, performance)
Fix some TODO: Meshes (also tracked in Consolidate mesh infrastructure after adding structured and unstructured ones #542)

Closes #506, closes #515

…ured mesh solver for Euler 2D

…n Euler elixirs

codecov · 2021-04-17T09:34:18Z

Codecov Report

Merging #550 (42c3435) into main (77ee2f3) will decrease coverage by 0.02%.
The diff coverage is 79.41%.

@@            Coverage Diff             @@
##             main     #550      +/-   ##
==========================================
- Coverage   93.87%   93.84%   -0.03%     
==========================================
  Files         138      138              
  Lines       13799    13847      +48     
==========================================
+ Hits        12954    12995      +41     
- Misses        845      852       +7

Flag	Coverage Δ
unittests	`93.84% <79.41%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ples/2d/elixir_euler_source_terms_parallelogram.jl	`100.00% <ø> (ø)`
examples/2d/elixir_euler_source_terms_rotated.jl	`95.83% <ø> (ø)`
...amples/2d/elixir_euler_source_terms_waving_flag.jl	`100.00% <ø> (ø)`
examples/3d/elixir_euler_source_terms_curved.jl	`100.00% <ø> (ø)`
src/callbacks_stage/positivity_zhang_shu.jl	`87.50% <0.00%> (ø)`
src/equations/acoustic_perturbation_2d.jl	`90.08% <0.00%> (ø)`
.../equations/compressible_euler_multicomponent_2d.jl	`97.52% <0.00%> (ø)`
src/equations/equations.jl	`96.55% <ø> (ø)`
src/equations/hyperbolic_diffusion_1d.jl	`85.71% <0.00%> (ø)`
src/equations/hyperbolic_diffusion_2d.jl	`86.52% <0.00%> (ø)`
... and 57 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77ee2f3...42c3435. Read the comment docs.

efaulhaber

We have a few questions, but otherwise everything looks good to us (and more tidied up than before).

src/solvers/dg_curved/dg_2d.jl

src/equations/compressible_euler_2d.jl

src/equations/equations.jl

src/equations/numerical_fluxes.jl

efaulhaber · 2021-04-17T11:33:15Z

src/solvers/dg_curved/dg_3d.jl

+@inline function calc_interface_flux!(surface_flux_values, left_element, right_element,
+                                      orientation, u,
+                                      mesh::CurvedMesh{3}, equations,
+                                      dg::DG, cache)


Is there a convention how to split lines like this?
It seems like you had to change this everywhere in our code.
We would've tried to make the line lengths as similar as possible, for example:

Suggested change

@inline function calc_interface_flux!(surface_flux_values, left_element, right_element,

orientation, u,

mesh::CurvedMesh{3}, equations,

dg::DG, cache)

@inline function calc_interface_flux!(surface_flux_values, left_element, right_element,

orientation, u, mesh::CurvedMesh{3},

equations, dg::DG, cache)

Making the line lengths similar is usually a good approach and I wouldn't say that your version before was somehow "wrong" in any sense. It was just much more convenient for me to shorten some lines a bit when working with multiple open files next to each other (which was helpful to keep consistency between different files).
A rough rule of thumb that I often use is

not to split arguments belonging to each other such as nonconservative_terms, equations

to put arguments important for dispatch often at the beginning of lines to stress their importance

src/solvers/dg_curved/dg_3d.jl

Co-authored-by: erik-f <44124897+erik-f@users.noreply.github.com>

sloede

LGTM in general.

So what does this PR mean for numerical fluxes in curved meshes: flux_lax_friedrichs can directly be used in a curved way, so does flux, but all other numerical fluxes would have to be fixed?

benchmark/mesh_uniform_cartesian.mesh

src/callbacks_step/stepsize_dg3d.jl

src/equations/equations.jl

src/solvers/dg_tree/dg_1d.jl

src/solvers/dg_curved/dg_1d.jl

src/solvers/dg_curved/dg_2d.jl

ranocha · 2021-04-18T06:36:26Z

So what does this PR mean for numerical fluxes in curved meshes: flux_lax_friedrichs can directly be used in a curved way, so does flux, but all other numerical fluxes would have to be fixed?

Yes, exactly. Alternatively, other numerical fluxes can use FluxRotated for now unless someone needs more performance and implements versions dispatching on normal::AbstractVector.

sloede · 2021-04-18T06:47:51Z

So what does this PR mean for numerical fluxes in curved meshes: flux_lax_friedrichs can directly be used in a curved way, so does flux, but all other numerical fluxes would have to be fixed?

Yes, exactly. Alternatively, other numerical fluxes can use FluxRotated for now unless someone needs more performance and implements versions dispatching on normal::AbstractVector.

This sounds like a good compromise to me. However, this should definitely be introduced at the next Trixi meeting and preferably also captured in the docs, as it is not 100% clear from reading the code alone

Co-authored-by: Michael Schlottke-Lakemper <michael@sloede.com>

sloede · 2021-04-18T08:59:39Z

Thanks a lot for the docs, @ranocha!

sloede

LGTM! Maybe you need to add Downloads to the [deps] in benchmark/Project.toml? If not, feel free to merge once all tests pass and coverage is ok

ranocha added 2 commits April 16, 2021 17:21

WIP: start unifying some interfaces and improve performance of struct…

a674cbb

…ured mesh solver for Euler 2D

add elixirs used for performance comparisons in 2D

4bcb2b7

ranocha mentioned this pull request Apr 17, 2021

Consolidate mesh infrastructure after adding structured and unstructured ones #542

Closed

23 tasks

ranocha added 11 commits April 17, 2021 06:46

rename dg/dg -> dg/dg_tree

3c990b4

Merge branch 'main' into hr/compare_meshes

4d42c4d

add NEWS entries for new mesh types

32b2f84

unify 2D calling signatures

9b67cd7

unify 1D, 2D, 3D calling signatures

147c283

add 3D mesh comparison benchmarks

6aec9e4

Euler flux in 3D for normal::AbstractVector

4dfdf89

rename new benchmark elixirs consistently

7a48626

Merge branch 'main' into hr/compare_meshes

59b883f

use get_contravariant_vector to abstract memory layout away

487d761

swap index <-> dimension in the memory layout of contravariant_vectors

36befbf

ranocha changed the title ~~WIP: compare meshes, unify signatures, and improve performance~~ compare meshes, unify signatures, and improve performance Apr 17, 2021

use flux_lax_friedrichs instead of FluxRotated(flux_lax_friedrichs) i…

4610206

…n Euler elixirs

specify orientation/normal in many methods

ccafced

ranocha marked this pull request as ready for review April 17, 2021 10:11

ranocha requested review from efaulhaber and sloede April 17, 2021 10:11

efaulhaber requested changes Apr 17, 2021

View reviewed changes

Apply suggestions from code review

9448abf

Co-authored-by: erik-f <44124897+erik-f@users.noreply.github.com>

sloede requested changes Apr 17, 2021

View reviewed changes

ranocha and others added 4 commits April 18, 2021 08:51

Apply suggestions from code review

bcbfb12

Co-authored-by: Michael Schlottke-Lakemper <michael@sloede.com>

Merge branch 'main' into hr/compare_meshes

9827369

add some docs

08b33ba

dispatch only on the mesh (dimension)

e40b61e

ranocha requested a review from sloede April 18, 2021 08:43

download the mesh file

42c3435

ranocha requested a review from efaulhaber April 18, 2021 14:56

sloede approved these changes Apr 18, 2021

View reviewed changes

ranocha enabled auto-merge (squash) April 18, 2021 15:08

ranocha disabled auto-merge April 18, 2021 15:23

ranocha merged commit 44fefb0 into main Apr 19, 2021

ranocha deleted the hr/compare_meshes branch April 19, 2021 10:42

ranocha removed the request for review from efaulhaber April 19, 2021 10:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compare meshes, unify signatures, and improve performance #550

compare meshes, unify signatures, and improve performance #550

ranocha commented Apr 16, 2021 •

edited

codecov bot commented Apr 17, 2021 •

edited

efaulhaber left a comment

efaulhaber Apr 17, 2021

ranocha Apr 17, 2021

sloede left a comment

ranocha commented Apr 18, 2021

sloede commented Apr 18, 2021

sloede commented Apr 18, 2021

sloede left a comment

compare meshes, unify signatures, and improve performance #550

compare meshes, unify signatures, and improve performance #550

Conversation

ranocha commented Apr 16, 2021 • edited

Results from Rocinante using one thread at ccafced

TODO

codecov bot commented Apr 17, 2021 • edited

Codecov Report

efaulhaber left a comment

Choose a reason for hiding this comment

efaulhaber Apr 17, 2021

Choose a reason for hiding this comment

ranocha Apr 17, 2021

Choose a reason for hiding this comment

sloede left a comment

Choose a reason for hiding this comment

ranocha commented Apr 18, 2021

sloede commented Apr 18, 2021

sloede commented Apr 18, 2021

sloede left a comment

Choose a reason for hiding this comment

ranocha commented Apr 16, 2021 •

edited

Results from Rocinante using one thread at `ccafced`

codecov bot commented Apr 17, 2021 •

edited