Skip to content

Honor min_tasks_to_steal_pending in the work-requesting schedulers - #7771

Merged
hkaiser merged 1 commit into
masterfrom
workrequesting-min-tasks-to-steal
Oct 19, 2026
Merged

hkaiser merged 1 commit into
masterfrom
workrequesting-min-tasks-to-steal

Conversation

@hkaiser

@hkaiser hkaiser commented Oct 18, 2026

Copy link
Copy Markdown
Contributor

Proposed Changes

  • local_workrequesting_scheduler::handle_steal_request: a worker receiving a steal request no longer sends any tasks while it has fewer pending tasks than hpx.thread_queue.min_tasks_to_steal_pending, as the other schedulers already do. It took the tasks with allow_stealing = false, so the minimum was never checked. Nothing changes for the default minimum of 0.
  • thread_queue::get_next_threads: when stealing, compare the minimum with all pending tasks instead of the number of tasks requested. The count was clipped to max_items before the check, so asking for fewer tasks than the minimum was always refused, however many tasks were pending.
  • thread_queue_get_next_threads test: steal with a minimum set, and check that nothing can be stolen once fewer pending tasks are left.
  • min_tasks_to_steal_staged test: new --scheduler=default|workrequesting option, which selects the scheduler through the resource partitioner, and a new registration min_tasks_to_steal_staged_workrequesting. It uses this option instead of --hpx:queuing so that it doesn't conflict with a --hpx:queuing given through HPX_WITH_TESTS_COMMAND_LINE.

Any background context you want to provide?

min_tasks_to_steal_staged (added in #7730) fails intermittently on CI with state->ran_elsewhere.load() == 0 failed, '1' != '0', on master, release-2.0.X and several PRs (e.g. #7756). CI runs the tests with --hpx:queuing=local-workrequesting-fifo, and since #7726 the work-requesting schedulers actually hand tasks to idle workers, which made the missing check visible: one of the three tasks posted to a worker was sent to a worker requesting work, although the test sets min_tasks_to_steal_pending to 10.

I could reproduce this locally with --hpx:threads=2 --hpx:queuing=local-workrequesting-fifo and 16 runs in parallel: 3 out of 320 runs failed on master, none out of 320 with this change (also none for the new registration, and none with both --hpx:queuing and --scheduler given). The scheduler, thread pool, threading and resource partitioner unit tests pass.

Checklist

Not all points below apply to all pull requests.

  • I have added a new feature and have added tests to go along with it.
  • I have fixed a bug and have added a regression test.
  • I have added a test using random numbers; I have made sure it uses a seed, and that random numbers generated are valid inputs for the tests.

A worker handling a steal request sent tasks to the requesting worker
regardless of hpx.thread_queue.min_tasks_to_steal_pending, as it took
them from its queue with allow_stealing = false. Since #7726 made the
work-requesting schedulers hand out tasks, this made the
min_tasks_to_steal_staged test fail intermittently on CI, which runs
the tests with --hpx:queuing=local-workrequesting-fifo.

thread_queue::get_next_threads compared the minimum with the number of
requested items instead of all pending items, which refused to steal
fewer items than the minimum however many were pending.

Adds a --scheduler option to the min_tasks_to_steal_staged test, which
selects the scheduler through the resource partitioner, so that it can
be registered for the work-requesting scheduler without conflicting
with --hpx:queuing given through HPX_WITH_TESTS_COMMAND_LINE.

Signed-off-by: Hartmut Kaiser <[email protected]>
@coderabbitai

coderabbitai Bot commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: a2b2bfa4-9d86-4cda-bd61-20492620510e
📥 Commits

Reviewing files that changed from the base of the PR and between cba138f and fab677a.

📒 Files selected for processing: 5
  • libs/core/schedulers/include/hpx/schedulers/local_workrequesting_scheduler.hpp
  • libs/core/schedulers/include/hpx/schedulers/thread_queue.hpp
  • libs/core/schedulers/tests/unit/CMakeLists.txt
  • libs/core/schedulers/tests/unit/min_tasks_to_steal_staged.cpp
  • libs/core/schedulers/tests/unit/thread_queue_get_next_threads.cpp

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

📝 Summary

Summary by CodeRabbit

  • Bug Fixes
    • Work-stealing schedulers now check the total number of pending tasks against the minimum threshold before requesting work. This avoids treating a capped batch size as the total amount of pending work.
    • When the threshold is met, schedulers steal half the pending tasks when half-stealing is enabled, or one task otherwise. Stealing requests also respect their requested maximum, and subsequent requests return no work when none is available.

The queue and work-requesting scheduler now compare minimum stealing thresholds with total pending work. Tests cover queue retrieval and staged stealing with the work-requesting scheduler.

Changes: Pending-work stealing thresholds

Layer / File(s) Summary
Thread queue threshold and retrieval
libs/core/schedulers/include/hpx/schedulers/thread_queue.hpp, libs/core/schedulers/tests/unit/thread_queue_get_next_threads.cpp
The queue checks the minimum threshold against total pending work and caps retrieval at max_items. The test checks retrieval limits and verifies that a later steal returns no threads when the remaining count is below the threshold.
Work-requesting scheduler threshold coverage
libs/core/schedulers/include/hpx/schedulers/local_workrequesting_scheduler.hpp, libs/core/schedulers/tests/unit/min_tasks_to_steal_staged.cpp, libs/core/schedulers/tests/unit/CMakeLists.txt
The scheduler checks the pending count before choosing how many tasks to steal. The staged-steal test supports selecting the work-requesting scheduler, and CMake registers that test when the scheduler is enabled.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: the-ivii

Merge Risk: ⚪ Minimal · up to fab67

No actionable merge-blocking risk remains; the change is ready for normal merge checks.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 4 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly and concisely summarizes the main change: applying min_tasks_to_steal_pending in work-requesting schedulers.
Description check Passed The description directly explains the scheduler changes, regression tests, CI issue, and validation results.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 4 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

  • Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 2
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codacy-production

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

@StellarBot

Copy link
Copy Markdown
Collaborator
Performance test report

HPX Performance

Comparison

BENCHMARKFORK_JOIN_EXECUTORFORK_JOIN_EXECUTOR_NOSTACKPARALLEL_EXECUTORPARALLEL_SCHEDULERSCHEDULER_EXECUTOR
For Each(=)(=)(=)(=)(=)

Info

PropertyBeforeAfter
HPX Commit36d11de14571c0
HPX Datetime2026-10-14T17:56:09+00:002026-10-18T17:53:03+00:00
Envfile
Clusternamerostamrostam
Datetime2026-10-14T13:06:52.713280-05:002026-10-18T13:01:52.426526-05:00
Compiler/opt/apps/llvm/18.1.8/bin/clang++ 18.1.8/opt/apps/llvm/18.1.8/bin/clang++ 18.1.8
Hostnamemedusa08.rostam.cct.lsu.edumedusa08.rostam.cct.lsu.edu

Comparison

BENCHMARKNO-EXECUTOR
Future Overhead - Create Thread Hierarchical - Latch(=)

Info

PropertyBeforeAfter
HPX Commit36d11de14571c0
HPX Datetime2026-10-14T17:56:09+00:002026-10-18T17:53:03+00:00
Envfile
Clusternamerostamrostam
Datetime2026-10-14T13:08:45.592191-05:002026-10-18T13:03:43.717385-05:00
Compiler/opt/apps/llvm/18.1.8/bin/clang++ 18.1.8/opt/apps/llvm/18.1.8/bin/clang++ 18.1.8
Hostnamemedusa08.rostam.cct.lsu.edumedusa08.rostam.cct.lsu.edu

Comparison

BENCHMARKFORK_JOIN_EXECUTOR_DEFAULT_FORK_JOIN_POLICY_ALLOCATORPARALLEL_EXECUTOR_DEFAULT_PARALLEL_POLICY_ALLOCATORSCHEDULER_EXECUTOR_DEFAULT_SCHEDULER_EXECUTOR_ALLOCATOR
Stream Benchmark - Add(=)--(=)
Stream Benchmark - Scale(=)(=)(=)
Stream Benchmark - Triad(=)(=)=
Stream Benchmark - Copy(=)++

Info

PropertyBeforeAfter
HPX Commit36d11de14571c0
HPX Datetime2026-10-14T17:56:09+00:002026-10-18T17:53:03+00:00
Envfile
Clusternamerostamrostam
Datetime2026-10-14T13:09:05.868380-05:002026-10-18T13:04:03.967431-05:00
Compiler/opt/apps/llvm/18.1.8/bin/clang++ 18.1.8/opt/apps/llvm/18.1.8/bin/clang++ 18.1.8
Hostnamemedusa08.rostam.cct.lsu.edumedusa08.rostam.cct.lsu.edu

Explanation of Symbols

SymbolMEANING
=No performance change (confidence interval within ±1%)
(=)Probably no performance change (confidence interval within ±2%)
(+)/(-)Very small performance improvement/degradation (≤1%)
+/-Small performance improvement/degradation (≤5%)
++/--Large performance improvement/degradation (≤10%)
+++/---Very large performance improvement/degradation (>10%)
?Probably no change, but quite large uncertainty (confidence interval with ±5%)
??Unclear result, very large uncertainty (±10%)
???Something unexpected…

@hkaiser

hkaiser commented Oct 18, 2026

Copy link
Copy Markdown
Contributor Author

For the records: the queuing_options_7743 failure will be fixed by merging #7764.

@hkaiser hkaiser added this to the 2.0.0 milestone Oct 19, 2026
@hkaiser
hkaiser merged commit 87ecc51 into master Oct 19, 2026
102 of 110 checks passed
@hkaiser
hkaiser deleted the workrequesting-min-tasks-to-steal branch October 19, 2026 12:15
@hkaiser hkaiser mentioned this pull request Oct 19, 2026
1 of 2 tasks
hkaiser added a commit that referenced this pull request Oct 19, 2026
Describe the PRs merged since the release branch was rebuilt on master cba138f (#7675, #7720, #7723, #7725, #7726, #7735, #7736, #7738, #7740, #7741, #7749, #7754, #7755, #7757, #7759), and since it was rebuilt on master 97c7e25 (#7750, #7752, #7762, #7771, #7781, #7782, #7791, #7793). Regenerate the closed issue and pull request lists for release-2.0.X at 0e973ff against the v1.11.0 tag (754 PRs, 153 issues).

Signed-off-by: Hartmut Kaiser <[email protected]>
hkaiser added a commit that referenced this pull request Oct 21, 2026
Describe the PRs merged since the release branch was rebuilt on master cba138f (#7675, #7720, #7723, #7725, #7726, #7735, #7736, #7738, #7740, #7741, #7749, #7754, #7755, #7757, #7759), since it was rebuilt on master 97c7e25 (#7750, #7752, #7762, #7771, #7781, #7782, #7791, #7793), and since it was rebuilt on master 67e52ba (#7718, #7739, #7751, #7753, #7756, #7770, #7797, #7800, #7802, #7804, #7805). Regenerate the closed issue and pull request lists for release-2.0.X at a55fd37 against the v1.11.0 tag (765 PRs, 157 issues).

Signed-off-by: Hartmut Kaiser <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants