Threading for periodic FDSBP flux differencing

[jlchan]

Trixi parallelizes over elements, but periodic FDSBP solvers are "single element" and can't take advantage of threading. This implements a naive threading (without exploiting symmetry or skew-symmetry to reduce total flux evaluations). which gives about 4x speedup on my 8-core machine.

On main

rhs!                      435    21.5s  99.1%  49.5ms    971KiB  4.52%  2.23KiB
   volume integral         435    21.0s  96.7%  48.3ms    136KiB  0.63%     319B

On this PR

 rhs!                      435    5.55s  92.1%  12.8ms   1.08MiB  3.56%  2.54KiB
   volume integral         435    5.07s  84.1%  11.6ms    265KiB  0.85%     624B

[codecov]

Codecov Report

Merging #1029 (77d7055) into main (5986c2b) will decrease coverage by 0.08%.
The diff coverage is 48.65%.

@@            Coverage Diff             @@
##             main    #1029      +/-   ##
==========================================
- Coverage   93.99%   93.91%   -0.08%     
==========================================
  Files         291      291              
  Lines       22518    22550      +32     
==========================================
+ Hits        21164    21177      +13     
- Misses       1354     1373      +19     

Flag	Coverage Δ
unittests	93.91% <48.65%> (-0.08%)	⬇️

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

Impacted Files	Coverage Δ
src/solvers/dgmulti/sbp.jl	88.10% <36.67%> (-8.57%)	⬇️
src/solvers/dgmulti/dg.jl	96.15% <100.00%> (+0.09%)	⬆️
src/solvers/dgmulti/flux_differencing.jl	84.30% <100.00%> (-0.13%)	⬇️

---

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 5986c2b...77d7055. Read the comment docs.

[jlchan]

The most recent changes invoke existing calc_volume_integral! routines for SBP flux differencing if Threads.nthreads() < 2. This is motivated by the fact that the threading implementation does not exploit symmetry, so it loses roughly a factor of 2 on a single thread compared with previous optimized implementations.

On one thread N=32

 rhs!                      246    1.32s  97.8%  5.37ms    402KiB  13.4%  1.63KiB
   volume integral         246    1.29s  95.7%  5.26ms   34.6KiB  1.15%     144B

On 8 threads with N=32

 rhs!                      246    370ms  94.1%  1.51ms    629KiB  19.5%  2.56KiB
   volume integral         246    358ms  91.0%  1.45ms    150KiB  4.64%     624B

This is about a 4x speedup, which we'd expect (8x speedup from threading but 1/2 efficiency due to not exploiting symmetry).

[ranocha]

Thanks for this PR! I just have some minor comments.

[sloede]

Just a minor comment from my side (which can be resolved or just left as it is). Otherwise it LGTM

[jlchan]

Hopefully tests should pass this time

[jlchan]

@sloede @ranocha the coverage tests are failing, and I think it's because a branch with Threads.nthreads() > 1 isn't getting picked up. I added a periodic FDSBP test in test_threaded.jl though, so I'm not sure why it's not detecting coverage. Any ideas?

[ranocha]

I guess it's just running with two threads and you check for more than two threads?

We do not collect coverage info for threaded tests:

Trixi.jl/test/runtests.jl

Line 37 in 5986c2b

run(`$(Base.julia_cmd()) --threads=$TRIXI_NTHREADS --check-bounds=yes --code-coverage=none $(abspath("test_threaded.jl"))`)

Collecting coverage info with multiple threads is awfully slow, see JuliaLang/julia#36142...

From my point of view, it's okay to merge this as it is, even with reduced coverage info (since we know that we test the new code paths, just without coverage due to the issue mentioned above). What do you think, @sloede?

[jlchan]

@sloede are you OK if I merge this manually? The coverage drop is due to threading tests not being run w/coverage.