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

unstructured BCs #573

Merged
merged 7 commits into from
May 12, 2021
Merged

unstructured BCs #573

merged 7 commits into from
May 12, 2021

Conversation

ranocha
Copy link
Member

@ranocha ranocha commented May 12, 2021

These are some first and minor performance improvements of the BCs for unstructured meshes. They only manifest for elixir_euler_unstructured_quad_wall_bc.jl since that's the only elixir with multiple BCs.

I used the following code with julia --check-bounds=no --threads=1

julia> trixi_include("examples/2d/elixir_euler_unstructured_quad_wall_bc.jl")

julia> du_ode = similar(ode.u0); du = Trixi.wrap_array(du_ode, semi); u_ode = copy(ode.u0); u = Trixi.wrap_array(u_ode, semi); Trixi.rhs!(du_ode, u_ode, semi, 0.0)

julia> @benchmark Trixi.calc_boundary_flux!(
           $(semi.cache),
           $(ode.tspan[1]),
           $(semi.boundary_conditions),
           $(mesh),
           $(equations),
           $(solver))
  • Baseline 43497b2
    BenchmarkTools.Trial:
      memory estimate:  82.50 KiB
      allocs estimate:  880
      --------------
      minimum time:     17.600 μs (0.00% GC)
      median time:      19.207 μs (0.00% GC)
      mean time:        31.350 μs (18.41% GC)
      maximum time:     7.938 ms (99.44% GC)
      --------------
      samples:          10000
      evals/sample:     1
    
  • Improve uniform_flow_state 17a9f92
    BenchmarkTools.Trial:
      memory estimate:  82.50 KiB
      allocs estimate:  880
      --------------
      minimum time:     16.781 μs (0.00% GC)
      median time:      17.519 μs (0.00% GC)
      mean time:        28.180 μs (17.80% GC)
      maximum time:     5.796 ms (99.35% GC)
      --------------
      samples:          10000
      evals/sample:     1
    
  • Use Symbols instead of Strings for boundary names 87868b1
    BenchmarkTools.Trial:
      memory estimate:  82.50 KiB
      allocs estimate:  880
      --------------
      minimum time:     16.273 μs (0.00% GC)
      median time:      17.082 μs (0.00% GC)
      mean time:        28.010 μs (18.19% GC)
      maximum time:     5.882 ms (99.42% GC)
      --------------
      samples:          10000
      evals/sample:     1
    
  • Introduce a function barrier for boundary fluxes cfec929
    BenchmarkTools.Trial:
      memory estimate:  22.50 KiB
      allocs estimate:  240
      --------------
      minimum time:     7.593 μs (0.00% GC)
      median time:      7.825 μs (0.00% GC)
      mean time:        9.900 μs (15.44% GC)
      maximum time:     1.610 ms (99.31% GC)
      --------------
      samples:          10000
      evals/sample:     4
    

Further performance improvements need some more experiments and probably a decision between at least two approaches suggested in #542 (sorting and earlier function barriers or function wrappers via ccall).

Copy link
Member

@andrewwinters5000 andrewwinters5000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @ranocha ! I just left a few comments for clarification but overall I think this is ready for merge (as long as the periodic test will now pass)

@codecov
Copy link

codecov bot commented May 12, 2021

Codecov Report

Merging #573 (845e08e) into main (0d1bf98) will increase coverage by 15.39%.
The diff coverage is 93.75%.

Impacted file tree graph

@@             Coverage Diff             @@
##             main     #573       +/-   ##
===========================================
+ Coverage   78.33%   93.72%   +15.39%     
===========================================
  Files         138      138               
  Lines       13831    13837        +6     
===========================================
+ Hits        10834    12969     +2135     
+ Misses       2997      868     -2129     
Flag Coverage Δ
unittests 93.72% <93.75%> (+15.39%) ⬆️

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

Impacted Files Coverage Δ
...mples/2d/elixir_euler_unstructured_quad_wall_bc.jl 90.00% <75.00%> (-10.00%) ⬇️
src/mesh/unstructured_quad_mesh.jl 98.23% <100.00%> (ø)
src/solvers/dg_unstructured_quad/containers_2d.jl 96.42% <100.00%> (ø)
src/solvers/dg_unstructured_quad/dg_2d.jl 97.77% <100.00%> (-0.72%) ⬇️
src/callbacks_step/analysis.jl 96.84% <0.00%> (+0.90%) ⬆️
src/mesh/mesh_io.jl 95.65% <0.00%> (+1.86%) ⬆️
...emidiscretization/semidiscretization_hyperbolic.jl 90.69% <0.00%> (+2.32%) ⬆️
...discretization/semidiscretization_euler_gravity.jl 91.37% <0.00%> (+2.53%) ⬆️
src/callbacks_step/analysis_dg3d.jl 97.19% <0.00%> (+15.88%) ⬆️
... and 20 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 0d1bf98...845e08e. Read the comment docs.

Copy link
Member

@sloede sloede left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Just one minor question...

@sloede sloede enabled auto-merge (squash) May 12, 2021 14:08
@sloede sloede merged commit ca0a78f into trixi-framework:main May 12, 2021
@ranocha ranocha deleted the hr/unstructured_bcs branch May 12, 2021 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants