Repository navigation
algorithms: do not skip leading elements of the second range in parallel set operations - #7844
aneek22112007-tech wants to merge 3 commits into
Conversation
…lel set operations
set_operation() decided whether a chunk was the first partition before
moving start1 backwards over elements equal to their predecessor. When
the original first chunk bailed out (its start and end values compare
equal) and the next chunk moved start1 back to 0, that chunk still
computed start2 with lower_bound(first1[0]), silently dropping every
element of the second range smaller than first1[0]. With 2 or more
worker threads hpx::set_union(par, {1,1,2}, {0}) returned 1 1 2 and
set_symmetric_difference lost elements the same way. Decide based on
the adjusted start1 instead.
Signed-off-by: aneek22112007-tech <[email protected]>
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes.
 |
| Layer / File(s) | Summary |
|---|---|
Direct partition-start check libs/core/algorithms/include/hpx/parallel/algorithms/detail/set_operation.hpp |
The sequence-2 lower-bound search checks start1 != 0 directly. The local first_partition flag is removed. |
Shifted-input regression tests libs/core/algorithms/tests/unit/algorithms/set_union.cpp, libs/core/algorithms/tests/unit/algorithms/set_symmetric_difference.cpp |
New tests compare parallel results and returned output iterators with standard-library results. Both tests run four times and are called from hpx_main. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix
Merge Risk: 
589e3
-
No unresolved issue is evident from the supplied change context; the change is ready for normal merge checks.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description check The description clearly explains the parallel set-operation bug, the implementation change, and the added regression tests. Title check The title clearly and concisely identifies the main fix: preserving leading elements from the second range in parallel set operations. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
-
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
Up to standards ✅🟢 Issues
|
|
Can one of the admins verify this patch? |
|
I ran into the same bug independently while comparing the parallel set algorithms against |
|
Thanks @Ishaan400 for independently confirming this, and for the randomized check against |
|
@hkaiser the two red checks are unrelated to this change: |
Proposed Changes
set_operation(), decide whether a chunk covers the start of the first range fromstart1after walking it back over equal elements.test_set_union_first_chunk_shiftedandtest_set_symmetric_difference_first_chunk_shifted.Any background context you want to provide?
set_operation()splits the first input across worker threads. Before adjusting the chunk bounds, it recordedfirst_partition = (start1 == 0). A chunk that was not the original first partition then setstart2withlower_boundof its first value in the second range.Every chunk except the last one returns immediately when its start and end values compare equal, leaving that partition for the next chunk. The next chunk walks
start1backward over elements equal to its predecessor. That walk can movestart1back to 0, so the chunk now covers the beginning of the first range. It still used the flag captured before the walk, computedstart2withlower_bound(first1[0]), and skipped every element of the second range smaller thanfirst1[0].With two or more worker threads,
hpx::set_union(hpx::execution::par, {1, 1, 2}, {0})returned1 1 2instead of0 1 1 2.hpx::set_symmetric_difference(hpx::execution::par, {3, 3}, {0, 2})returned3 3instead of0 2 3 3. One thread does not split the range, so the same calls were correct at--hpx:threads=1.The new tests compare those calls with
std::set_unionandstd::set_symmetric_difference. With the fix removed, both fail at--hpx:threads=4(8 checks each). With the fix,set_union_testandset_symmetric_difference_testpassed 5/5 at--hpx:threads=1and 5/5 at--hpx:threads=4.set_difference_testandset_intersection_testpassed once each at 4 threads.Using the adjusted
start1keepsstart2at 0 when the chunk has moved back to the start of the first range.Checklist