trixi-framework / Trixi.jl

Trixi.jl: Adaptive high-order numerical simulations of conservation laws in Julia
https://trixi-framework.github.io/Trixi.jl
MIT License
540 stars 109 forks source link

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

Closed DanielDoehring closed 1 year ago

DanielDoehring commented 1 year ago

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

https://github.com/trixi-framework/Trixi.jl/blob/22856f4af27a06d38935d4f373a46f3492a4bf08/test/test_p4est_2d.jl#L27-L34

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

jlchan commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

DanielDoehring commented 1 year ago

Added tests (currently passing):