[SPH][Setup] allow speculative load balancing during injection#1797
Draft
tdavidcl wants to merge 9 commits intoShamrock-code:mainfrom
Draft
[SPH][Setup] allow speculative load balancing during injection#1797tdavidcl wants to merge 9 commits intoShamrock-code:mainfrom
tdavidcl wants to merge 9 commits intoShamrock-code:mainfrom
Conversation
Contributor
|
Thanks @tdavidcl for opening this PR! You can do multiple things directly here: Once the workflow completes a message will appear displaying informations related to the run. Also the PR gets automatically reviewed by gemini, you can: |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a speculative load balancing mechanism and adds support for discontinuous HCP lattice generation in the SPH setup. The speculative balancing logic calculates particle counts per patch using a device kernel and MPI reduction to improve load distribution during initialization. Feedback identifies a performance bottleneck in the load-counting kernel, which currently uses a brute-force
Comment on lines
+383
to
+394
| for (size_t j = 0; j < npatch; j++) { | ||
| shammath::CoordRange<Tvec> patch_coord | ||
| = {patch_aabb_min[j], patch_aabb_max[j]}; | ||
| if (patch_coord.contain_pos(pos)) { | ||
| sycl::atomic_ref< | ||
| u64, | ||
| sycl::memory_order::relaxed, | ||
| sycl::memory_scope::device> | ||
| atomic_local_load_values(local_load_values[j]); | ||
| atomic_local_load_values++; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
The speculative load counting kernel uses a brute-force approach with a nested loop over all global patches for every local particle, resulting in SerialPatchTree is already available and used elsewhere in this file (e.g., line 635) to perform
References
- Refactor duplicated logic into a helper function or lambda to improve readability and maintainability.
| u64 npatch = scheduler().patch_list.global.size(); | ||
|
|
||
| // check if the number of patches has changed, rebuild otherwise | ||
| if (npatch != speculative_last_npatch) { |
Contributor
There was a problem hiding this comment.
The speculative load values are only recomputed when the number of patches (npatch) changes. However, during the injection process, particles are extracted from to_insert and moved between ranks. If the load values are not updated to reflect the current state of to_insert, the load balancer will make decisions based on stale distribution data, potentially leading to severe load imbalance. The counting logic should be executed whenever compute_load is called to ensure it reflects the current "pending" load.
Comment on lines
+423
to
+425
| scheduler().update_local_load_value([&](shamrock::patch::Patch p) { | ||
| return speculative_load_values.get(p.id_patch); | ||
| }); |
Contributor
There was a problem hiding this comment.
The speculative load balancing logic currently replaces the patch load value with the count from to_insert. This ignores any particles that have already been successfully injected into the patches. To accurately represent the total expected load for a patch, the speculative count (particles yet to be injected) should be added to the current count of particles already present in the patch.
scheduler().update_local_load_value([&](shamrock::patch::Patch p) {
u64 current_load = scheduler().patch_data.owned_data.get(p.id_patch).get_obj_cnt();
return current_load + speculative_load_values.get(p.id_patch);
});
Contributor
Workflow reportworkflow report corresponding to commit eb6c610 Pre-commit check reportPre-commit check: ✅ Test pipeline can run. Clang-tidy diff reportDoxygen diff with
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.