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

Add @test (@allocated Trixi.rhs!(du_ode, u_ode, semi, t)) to every test? #1664

Closed
DanielDoehring opened this issue Oct 8, 2023 · 5 comments
Labels

Comments

@DanielDoehring
Copy link
Contributor

DanielDoehring commented Oct 8, 2023

Recently, we observed allocations due to upstream/Julia changes for a variety of cases, see e.g.
#1656 , #1642, #1635 #1626.

We test for a variety of cases whether the rhs! allocates, see for instance

# Ensure that we do not have excessive memory allocations
# (e.g., from type instabilities)
let
t = sol.t[end]
u_ode = sol.u[end]
du_ode = similar(u_ode)
@test (@allocated Trixi.rhs!(du_ode, u_ode, semi, t)) < 1000
end

Any opinions on including this for every/more tests to spot allocations faster?

@jlchan
Copy link
Contributor

jlchan commented Oct 8, 2023

Sounds reasonable to me, maybe we could move these to a separate set of allocation tests. Are there downsides you can think of (i.e., runtime, inconsistent failures, etc)?

@DanielDoehring
Copy link
Contributor Author

DanielDoehring commented Oct 8, 2023

Runtime should not increase significantly (unless we move them to own tests, as argued by Hendrik below), as (IIRC) 75%+ test time is spent for compilation.

@ranocha
Copy link
Member

ranocha commented Oct 9, 2023

Yes, this would be nice!

Sounds reasonable to me, maybe we could move these to a separate set of allocation tests. Are there downsides you can think of (i.e., runtime, inconsistent failures, etc)?

Moving them to a separate test job would likely have a significant impact on total test time since most time is spent in compilation. If we just include the stuff directly after testing an elixir, everything should be compiled already and the impact on total CI time should be small.

@DanielDoehring
Copy link
Contributor Author

Ok, I will start with the non-threaded tests and see how this evolves.

@DanielDoehring
Copy link
Contributor Author

DanielDoehring commented Oct 20, 2023

Added tests (currently passing):

  • test_dgmulti_1d.jl
  • test_dgmulti_2d.jl
  • test_dgmulti_3d.jl
  • test_mpi_p4est_2d.jl
  • test_mpi_p4est_3d.jl
  • test_mpi_tree.jl
  • test_p4est_2d.jl
  • test_p4est_3d.jl
  • test_paper_self_gravitating_gas_dynamics.jl
  • test_parabolic_1d.jl
  • test_parabolic_2d.jl
  • test_parabolic_3d.jl
  • test_structured_1d.jl
  • test_structured_2d.jl
  • test_structured_3d.jl
  • test_t8code_2d.jl
  • test_threaded.jl
  • test_tree_1d_advection.jl
  • test_tree_1d_burgers.jl
  • test_tree_1d_euler.jl
  • test_tree_1d_eulergravity.jl
  • test_tree_1d_eulermulti.jl
  • test_tree_1d_fdsbp.jl
  • test_tree_1d_hypdiff.jl
  • test_tree_1d_mhd.jl
  • test_tree_1d_mhdmulti.jl
  • test_tree_1d_shallowwater_twolayer.jl
  • test_tree_1d_shallowwater.jl
  • test_tree_2d_acoustics.jl
  • test_tree_2d_advection.jl
  • test_tree_2d_euler.jl
  • test_tree_2d_euleracoustics.jl
  • test_tree_2d_eulermulti.jl
  • test_tree_2d_fdsbp.jl
  • test_tree_2d_hypdiff.jl
  • test_tree_2d_kpp.jl
  • test_tree_2d_lbm.jl
  • test_tree_2d_linearizedeuler.jl
  • test_tree_2d_mhd.jl
  • test_tree_2d_mhdmulti.jl
  • test_tree_2d_shallowwater_twolayer.jl
  • test_tree_2d_shallowwater.jl
  • test_tree_3d_advection.jl
  • test_tree_3d_euler.jl
  • test_tree_3d_eulergravity.jl
  • test_tree_3d_fdsbp.jl
  • test_tree_3d_hypdiff.jl
  • test_tree_3d_lbm.jl
  • test_tree_3d_mhd.jl
  • test_unstructured_2d.jl

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

No branches or pull requests

3 participants