Change backend threading behavior#2417
Change backend threading behavior#2417afilogo wants to merge 7 commits intotrixi-framework:mainfrom afilogo:main
Conversation
Review checklistThis checklist is meant to assist creators of PRs (to let them know what reviewers will typically look for) and reviewers (to guide them in a structured review process). Items do not need to be checked explicitly for a PR to be eligible for merging. Purpose and scope
Code quality
Documentation
Testing
Performance
Verification
Created with ❤️ by the Trixi.jl community. |
|
Thanks for your contribution! I am not so sure if we really want to offer other threading strategies (namely |
|
Thanks for the feedback @DanielDoehring and @vchuravy. Appreciate you taking the time to review it. I thought it might increase user control, but I also understand your perspective. |
Co-authored-by: Valentin Churavy <v.churavy@gmail.com>
vchuravy
left a comment
There was a problem hiding this comment.
Thanks that seems like a good improvement.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2417 +/- ##
==========================================
- Coverage 96.80% 96.79% -0.01%
==========================================
Files 495 495
Lines 40917 40919 +2
==========================================
- Hits 39608 39607 -1
- Misses 1309 1312 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
CI is failing, so it appears that something is off. Could you please take a look and see what's going on? |
I believe the issue is the in static macro "@static if _PREFERENCE_THREADING === :polyester && LoopVectorization.check_args(u_ode)" code line. It should be fine now. |
|
I've also missed a previous _PREFERENCE_POLYESTER line. Sorry about that. |
ranocha
left a comment
There was a problem hiding this comment.
Could you please add an entry to the NEWS.md file since this is a breaking change?
| elseif _PREFERENCE_THREADING === :dynamic | ||
| quote | ||
| let | ||
| if $Threads.nthreads() == 1 | ||
| $(expr) | ||
| else | ||
| $Threads.@threads :dynamic $(expr) | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
This will introduce bugs for code like
Trixi.jl/src/solvers/dgsem_tree/dg_2d.jl
Lines 945 to 950 in f35d749
won't it? @vchuravy
There was a problem hiding this comment.
Oof, yes it will. @benegee and I were just talking about #2212 and the fact that creating or adapting the cache is a weird operation due to these backend specific containers. We are baking quite a bit of structure that is dependent on how we are performing the computation into the cache object.
So eventually we might need something like create_cache(..., backend), but even that is fraught since with #2212 I am able to change the storage type and thus the backend.
Maybe, we need struct StaticThreadIDCache end and struct GPUCache end...
There was a problem hiding this comment.
So I think we will need to remove the "dynamic" option for now from this PR.
There was a problem hiding this comment.
| elseif _PREFERENCE_THREADING === :dynamic | |
| quote | |
| let | |
| if $Threads.nthreads() == 1 | |
| $(expr) | |
| else | |
| $Threads.@threads :dynamic $(expr) | |
| end | |
| end | |
| end |
This PR is based on the discussion #2159, having into account your feedback. I apologize for the late reply and hope did not make any mistake.