Duplication of the thread queues in the schedulers#7360
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds the ChangesLocal-priority-fifo-double scheduler
Runtime synchronization and lifecycle updates
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Up to standards ✅🟢 Issues
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/core/command_line_handling_local/src/parse_command_line_local.cpp (1)
561-569: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMissing space in updated help text.
'local-priority-fifo','local-priority-fifo-double'lacks the space after the comma, inconsistent with the rest of the list's formatting ('abp-priority-fifo', 'abp-priority-lifo', ...).✏️ Proposed fix
- "'local', 'local-priority-fifo','local-priority-fifo-double', " + "'local', 'local-priority-fifo', 'local-priority-fifo-double', "🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/core/command_line_handling_local/src/parse_command_line_local.cpp` around lines 561 - 569, The help text in parse_command_line_local’s hpx:queuing option has a formatting inconsistency where the list item for local-priority-fifo and local-priority-fifo-double is missing a space after the comma. Update the string in the argument description to match the formatting used by the other scheduling policy names so the options list reads consistently.
🧹 Nitpick comments (1)
.github/workflows/linux_debug.yml (1)
39-40: 🩺 Stability & Availability | 🔵 TrivialConfirm forcing
local-priority-fifo-doubledoesn't reduce default-scheduler coverage in this job.
HPX_WITH_TESTS_COMMAND_LINE=--hpx:queuing=local-priority-fifo-doubleapplies to the entire test/examples run for this workflow, not just the resource-partitioner scheduler tests. If this is the only debug CI job exercising the default queuing policy end-to-end, switching it wholesale to the new experimental scheduler could mask regressions in the default scheduler for this configuration, and could also make this job flaky if the new backend has undiscovered issues. Worth confirming this is intentional (e.g., another job still covers default queuing) rather than an oversight.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/linux_debug.yml around lines 39 - 40, The workflow-wide HPX_WITH_TESTS_COMMAND_LINE override is forcing local-priority-fifo-double for every test/example in the debug job, which may reduce default-scheduler coverage. Update the linux_debug workflow so the experimental scheduler is applied only to the intended resource-partitioner scheduler tests, or verify that another job still covers the default queuing path end-to-end; use the workflow step setting HPX_WITH_TESTS_COMMAND_LINE as the place to narrow or split the command.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@libs/core/resource_partitioner/include/hpx/resource_partitioner/partitioner_fwd.hpp`:
- Around line 98-107: The public scheduler policy enum in
resource_partitioner::partitioner_fwd must keep existing numeric values stable;
the new local_priority_fifo_double entry should not shift any existing policies.
Update the enum definition so local_priority_fifo_double uses the next unused
value while leaving local_priority_lifo, static_, static_priority,
abp_priority_fifo, abp_priority_lifo, shared_priority,
local_workrequesting_fifo, local_workrequesting_lifo, and
local_workrequesting_mc unchanged.
---
Outside diff comments:
In `@libs/core/command_line_handling_local/src/parse_command_line_local.cpp`:
- Around line 561-569: The help text in parse_command_line_local’s hpx:queuing
option has a formatting inconsistency where the list item for
local-priority-fifo and local-priority-fifo-double is missing a space after the
comma. Update the string in the argument description to match the formatting
used by the other scheduling policy names so the options list reads
consistently.
---
Nitpick comments:
In @.github/workflows/linux_debug.yml:
- Around line 39-40: The workflow-wide HPX_WITH_TESTS_COMMAND_LINE override is
forcing local-priority-fifo-double for every test/example in the debug job,
which may reduce default-scheduler coverage. Update the linux_debug workflow so
the experimental scheduler is applied only to the intended resource-partitioner
scheduler tests, or verify that another job still covers the default queuing
path end-to-end; use the workflow step setting HPX_WITH_TESTS_COMMAND_LINE as
the place to narrow or split the command.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 19482f86-b100-4ab9-be3b-c19995304b75
📒 Files selected for processing (20)
.github/workflows/linux_debug.yml.github/workflows/macos_debug.yml.github/workflows/windows_release_2022.yml.jenkins/lsu-perftests/launch_perftests.shlibs/core/command_line_handling_local/src/parse_command_line_local.cpplibs/core/resource_partitioner/include/hpx/resource_partitioner/partitioner_fwd.hpplibs/core/resource_partitioner/src/detail_partitioner.cpplibs/core/resource_partitioner/tests/unit/cross_pool_injection.cpplibs/core/resource_partitioner/tests/unit/shutdown_suspended_pus.cpplibs/core/resource_partitioner/tests/unit/suspend_pool.cpplibs/core/resource_partitioner/tests/unit/suspend_pool_external.cpplibs/core/resource_partitioner/tests/unit/suspend_runtime.cpplibs/core/resource_partitioner/tests/unit/suspend_thread.cpplibs/core/resource_partitioner/tests/unit/suspend_thread_external.cpplibs/core/resource_partitioner/tests/unit/suspend_thread_timed.cpplibs/core/schedulers/include/hpx/schedulers/lockfree_queue_backends.hpplibs/core/schedulers/include/hpx/schedulers/thread_queue.hpplibs/core/thread_pools/src/scheduled_thread_pool.cpplibs/core/threadmanager/include/hpx/threadmanager/threadmanager.hpplibs/core/threadmanager/src/threadmanager.cpp
Collaborator
Performance test reportHPX PerformanceComparison
Info
Comparison
Info
Comparison
Info
Explanation of Symbols
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/core/threading_base/src/thread_data.cpp (1)
135-154: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftPreserve
ran_exit_funcs_on the empty fast path
run_thread_exit_callbacks()now returns before settingran_exit_funcs_ = truewhen there are no exit callbacks. That re-opens the window for a concurrentadd_thread_exit_callback()to succeed while the thread is still active, and the callback can then be dropped;free_thread_exit_callbacks()will also tripHPX_ASSERT(exit_funcs_.empty() || ran_exit_funcs_)in debug. Keep the guard set on the empty path, or move the fast-path check after locking.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/core/threading_base/src/thread_data.cpp` around lines 135 - 154, Update thread_data::run_thread_exit_callbacks() so ran_exit_funcs_ is set to true even when exit_funcs_ is empty; either assign it before returning from the fast path or acquire the spinlock before checking. Preserve the synchronization semantics so add_thread_exit_callback() cannot enqueue callbacks after exit processing has completed, and free_thread_exit_callbacks() remains assertion-safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@libs/core/threading_base/src/thread_data.cpp`:
- Around line 135-154: Update thread_data::run_thread_exit_callbacks() so
ran_exit_funcs_ is set to true even when exit_funcs_ is empty; either assign it
before returning from the fast path or acquire the spinlock before checking.
Preserve the synchronization semantics so add_thread_exit_callback() cannot
enqueue callbacks after exit processing has completed, and
free_thread_exit_callbacks() remains assertion-safe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 112f37af-2cd9-4b04-bc95-fd9cc63cf6f7
📒 Files selected for processing (5)
libs/core/coroutines/include/hpx/coroutines/detail/context_base.hpplibs/core/coroutines/src/detail/coroutine_impl.cpplibs/core/synchronization/include/hpx/synchronization/latch.hpplibs/core/threading_base/include/hpx/threading_base/thread_data.hpplibs/core/threading_base/src/thread_data.cpp
hkaiser
force-pushed
the
fixing_7111
branch
2 times, most recently
from
July 10, 2026 13:21
4eb6655 to
3cd437a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/core/threading_base/src/thread_data.cpp`:
- Around line 146-156: Update the exit-callback draining path in the relevant
thread-data cleanup function to set ran_exit_funcs_ = true after callbacks are
drained, matching the early-return path. Ensure the flag is set before releasing
or exiting the drain logic so add_thread_exit_callback() and
free_thread_exit_callbacks() observe the correct state.
- Around line 137-144: Update the exit-callback handling around the early
`has_exit_funcs_` check so it acquires `spinlock_pool::spinlock_for(this)`
before checking the flag and setting `ran_exit_funcs_`. Keep the check and state
update within the same critical section used by `add_thread_exit_callback()`,
avoiding any unlocked early return.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2604af2f-f03c-4f2b-b7f3-90e6d0f7b838
📒 Files selected for processing (6)
libs/core/coroutines/include/hpx/coroutines/detail/context_base.hpplibs/core/coroutines/src/detail/coroutine_impl.cpplibs/core/schedulers/include/hpx/schedulers/lockfree_queue_backends.hpplibs/core/synchronization/include/hpx/synchronization/latch.hpplibs/core/threading_base/include/hpx/threading_base/thread_data.hpplibs/core/threading_base/src/thread_data.cpp
🚧 Files skipped from review as they are similar to previous changes (5)
- libs/core/threading_base/include/hpx/threading_base/thread_data.hpp
- libs/core/coroutines/include/hpx/coroutines/detail/context_base.hpp
- libs/core/schedulers/include/hpx/schedulers/lockfree_queue_backends.hpp
- libs/core/coroutines/src/detail/coroutine_impl.cpp
- libs/core/synchronization/include/hpx/synchronization/latch.hpp
hkaiser
force-pushed
the
fixing_7111
branch
2 times, most recently
from
July 10, 2026 13:40
852d3f6 to
079e469
Compare
Collaborator
Performance test reportHPX PerformanceComparison
Info
Comparison
Info
Comparison
Info
Explanation of Symbols
|
- Allow to reduce the work-stealing overheads by minimizing cache interference. The idea is to add a 'passive' queue next to each 'active' one. The 'active' queue is used as long as it holds threads to run. As soon as it runs empty the scheduler swaps the queues and continues executing threads from the now 'active' (formerly 'passive') queue. New threads are scheduled on the currently 'passive' queue. Work-stealing happens from the 'passive' queue first and only if that was unsuccessful tries to steal from the 'active' queues (as before). Signed-off-by: Hartmut Kaiser <hartmut.kaiser@gmail.com>
- flyby: working around clang-format problem Signed-off-by: Hartmut Kaiser <hartmut.kaiser@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/core/resource_partitioner/src/detail_partitioner.cpp`:
- Around line 80-133: Update get_scheduling_policy to detect an empty or null
scheduler_name before any prefix matching and return
scheduling_policy::unspecified. Preserve the existing prefix-based policy
selection for non-empty names so setup_schedulers() can handle missing
hpx.scheduler entries through its command_line_error path.
In `@libs/core/threading/src/thread.cpp`:
- Around line 210-228: Update the join state in the callback/wait path around
add_thread_exit_callback and detach_locked so cv and done have shared ownership
rather than stack lifetime. Store both in a shared state object captured by the
exit callback and retained by the waiter through completion, then use that
shared state for notification and waiting while preserving the existing
added-condition behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: afe45c74-d906-42a3-90ae-1391bbe91711
📒 Files selected for processing (39)
.github/workflows/linux_debug.yml.github/workflows/macos_debug.yml.github/workflows/windows_release_2022.yml.jenkins/lsu-perftests/launch_perftests.shlibs/core/command_line_handling_local/src/parse_command_line_local.cpplibs/core/concurrency/include/hpx/concurrency/deque.hpplibs/core/concurrency/include/hpx/concurrency/queue.hpplibs/core/coroutines/include/hpx/coroutines/detail/context_base.hpplibs/core/coroutines/src/detail/coroutine_impl.cpplibs/core/init_runtime_local/src/init_runtime_local.cpplibs/core/resource_partitioner/include/hpx/resource_partitioner/partitioner.hpplibs/core/resource_partitioner/include/hpx/resource_partitioner/partitioner_fwd.hpplibs/core/resource_partitioner/src/detail_partitioner.cpplibs/core/resource_partitioner/tests/unit/CMakeLists.txtlibs/core/resource_partitioner/tests/unit/cross_pool_injection.cpplibs/core/resource_partitioner/tests/unit/scheduling_policy_wiring.cpplibs/core/resource_partitioner/tests/unit/shutdown_suspended_pus.cpplibs/core/resource_partitioner/tests/unit/suspend_pool.cpplibs/core/resource_partitioner/tests/unit/suspend_pool_external.cpplibs/core/resource_partitioner/tests/unit/suspend_runtime.cpplibs/core/resource_partitioner/tests/unit/suspend_thread.cpplibs/core/resource_partitioner/tests/unit/suspend_thread_external.cpplibs/core/resource_partitioner/tests/unit/suspend_thread_timed.cpplibs/core/schedulers/include/hpx/schedulers/lockfree_queue_backends.hpplibs/core/schedulers/include/hpx/schedulers/thread_queue.hpplibs/core/schedulers/tests/unit/CMakeLists.txtlibs/core/schedulers/tests/unit/lockfree_fifo_double_backend.cpplibs/core/synchronization/include/hpx/synchronization/latch.hpplibs/core/synchronization/tests/unit/CMakeLists.txtlibs/core/synchronization/tests/unit/latch_notification.cpplibs/core/thread_pools/src/scheduled_thread_pool.cpplibs/core/threading/src/thread.cpplibs/core/threading_base/include/hpx/threading_base/thread_data.hpplibs/core/threading_base/src/thread_data.cpplibs/core/threading_base/tests/unit/CMakeLists.txtlibs/core/threading_base/tests/unit/thread_exit_callbacks.cpplibs/core/threadmanager/include/hpx/threadmanager/threadmanager.hpplibs/core/threadmanager/src/threadmanager.cpplibs/full/init_runtime/src/hpx_init.cpp
🚧 Files skipped from review as they are similar to previous changes (21)
- libs/core/resource_partitioner/tests/unit/suspend_thread_timed.cpp
- .jenkins/lsu-perftests/launch_perftests.sh
- libs/core/resource_partitioner/tests/unit/suspend_pool.cpp
- .github/workflows/linux_debug.yml
- libs/core/command_line_handling_local/src/parse_command_line_local.cpp
- .github/workflows/macos_debug.yml
- .github/workflows/windows_release_2022.yml
- libs/core/resource_partitioner/tests/unit/cross_pool_injection.cpp
- libs/core/thread_pools/src/scheduled_thread_pool.cpp
- libs/core/resource_partitioner/tests/unit/shutdown_suspended_pus.cpp
- libs/core/resource_partitioner/tests/unit/suspend_pool_external.cpp
- libs/core/threadmanager/include/hpx/threadmanager/threadmanager.hpp
- libs/core/threading_base/include/hpx/threading_base/thread_data.hpp
- libs/core/coroutines/include/hpx/coroutines/detail/context_base.hpp
- libs/core/concurrency/include/hpx/concurrency/queue.hpp
- libs/core/threadmanager/src/threadmanager.cpp
- libs/core/concurrency/include/hpx/concurrency/deque.hpp
- libs/core/threading_base/src/thread_data.cpp
- libs/core/coroutines/src/detail/coroutine_impl.cpp
- libs/core/synchronization/include/hpx/synchronization/latch.hpp
- libs/core/schedulers/include/hpx/schedulers/lockfree_queue_backends.hpp
hkaiser
force-pushed
the
fixing_7111
branch
3 times, most recently
from
July 12, 2026 21:52
e2590b9 to
aa7b7c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
libs/core/threading_base/include/hpx/threading_base/thread_data.hpp (1)
597-604: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo coexisting per-object lock mechanisms.
mtx_now guardsexit_funcs_/description_, whileinterrupt(),backtrace_, andget_backtrace/set_backtracestill use the separatespinlock_pool::spinlock_for(this)hash-based lock (Lines 378-469, unchanged). Consider consolidating ontomtx_for consistency and to avoid maintaining two locking schemes on the same object, unless the split is intentional (e.g., to avoid contention between hot exit-callback paths and rarely-used backtrace/interrupt paths).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/core/threading_base/include/hpx/threading_base/thread_data.hpp` around lines 597 - 604, Consolidate the per-object locking in thread_data around the existing mtx_ member: update interrupt(), backtrace access, and get_backtrace/set_backtrace to use mtx_ instead of spinlock_pool::spinlock_for(this), while preserving their current synchronization behavior. Remove the now-unneeded hash-based lock usage for this object and retain mtx_ as the single lock guarding all affected state.libs/core/schedulers/tests/unit/lockfree_fifo_double_backend.cpp (1)
218-289: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUnbounded spin-wait risks a hang instead of a clear test failure.
The stealer (Lines 247-268) and owner (Lines 271-289) loops spin on
yield()forever untilproducers_done && backend.empty(). If a regression in the backend under test causes an item to be lost (never observable viapop), these loops spin forever and the test hangs rather than failing with a diagnostic message — relying solely on an external CTest timeout to eventually kill it.Consider bounding the spin with an iteration/time cap and asserting failure explicitly so a regression produces a fast, informative failure instead of a hang.
♻️ Example: bound the spin with a retry cap
consumers.emplace_back([&backend, &producers_done, &collected, s]() { std::uint64_t val = 0; + std::uint64_t idle_spins = 0; + constexpr std::uint64_t max_idle_spins = 10'000'000; while (true) { if (backend.pop(val, /*steal=*/true)) { collected[s].push_back(val); + idle_spins = 0; } else if (producers_done.load(std::memory_order_acquire) && backend.empty()) { break; } else { + HPX_TEST(++idle_spins < max_idle_spins); + if (idle_spins >= max_idle_spins) + break; std::this_thread::yield(); } } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/core/schedulers/tests/unit/lockfree_fifo_double_backend.cpp` around lines 218 - 289, Bound the retry loops in test_concurrent_stress for both the stealer and owner consumers so they cannot spin indefinitely when items are lost. Track failed pop attempts or elapsed time, and assert a clear test failure with diagnostic context once the cap is exceeded, while preserving the existing producers_done && backend.empty() termination path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/core/threading_base/src/thread_data.cpp`:
- Around line 313-324: Update thread_data::get_lco_description and
thread_data::set_lco_description to acquire mtx_ before reading or swapping
lco_description_, matching the locking used for description_. Preserve the
existing return values and swap behavior while ensuring concurrent access is
synchronized.
- Around line 138-155: Update thread_data::run_thread_exit_callbacks to
move/remove the current front callback from exit_funcs_ while mtx_ is held,
before releasing the lock to invoke it. Then execute the moved callback outside
the lock, ensuring callbacks added during f() remain queued and the executed
callback is not popped afterward.
---
Nitpick comments:
In `@libs/core/schedulers/tests/unit/lockfree_fifo_double_backend.cpp`:
- Around line 218-289: Bound the retry loops in test_concurrent_stress for both
the stealer and owner consumers so they cannot spin indefinitely when items are
lost. Track failed pop attempts or elapsed time, and assert a clear test failure
with diagnostic context once the cap is exceeded, while preserving the existing
producers_done && backend.empty() termination path.
In `@libs/core/threading_base/include/hpx/threading_base/thread_data.hpp`:
- Around line 597-604: Consolidate the per-object locking in thread_data around
the existing mtx_ member: update interrupt(), backtrace access, and
get_backtrace/set_backtrace to use mtx_ instead of
spinlock_pool::spinlock_for(this), while preserving their current
synchronization behavior. Remove the now-unneeded hash-based lock usage for this
object and retain mtx_ as the single lock guarding all affected state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0fe9c6e8-8abf-466b-a141-aecea0ecd500
📒 Files selected for processing (16)
libs/core/init_runtime_local/src/init_runtime_local.cpplibs/core/resource_partitioner/include/hpx/resource_partitioner/partitioner.hpplibs/core/resource_partitioner/include/hpx/resource_partitioner/partitioner_fwd.hpplibs/core/resource_partitioner/src/detail_partitioner.cpplibs/core/resource_partitioner/tests/unit/CMakeLists.txtlibs/core/resource_partitioner/tests/unit/scheduling_policy_wiring.cpplibs/core/schedulers/tests/unit/CMakeLists.txtlibs/core/schedulers/tests/unit/lockfree_fifo_double_backend.cpplibs/core/synchronization/tests/unit/CMakeLists.txtlibs/core/synchronization/tests/unit/latch_notification.cpplibs/core/threading/src/thread.cpplibs/core/threading_base/include/hpx/threading_base/thread_data.hpplibs/core/threading_base/src/thread_data.cpplibs/core/threading_base/tests/unit/CMakeLists.txtlibs/core/threading_base/tests/unit/thread_exit_callbacks.cpplibs/full/init_runtime/src/hpx_init.cpp
🚧 Files skipped from review as they are similar to previous changes (13)
- libs/core/schedulers/tests/unit/CMakeLists.txt
- libs/core/threading_base/tests/unit/CMakeLists.txt
- libs/core/synchronization/tests/unit/CMakeLists.txt
- libs/core/resource_partitioner/tests/unit/CMakeLists.txt
- libs/core/resource_partitioner/include/hpx/resource_partitioner/partitioner.hpp
- libs/core/init_runtime_local/src/init_runtime_local.cpp
- libs/core/resource_partitioner/tests/unit/scheduling_policy_wiring.cpp
- libs/core/threading_base/tests/unit/thread_exit_callbacks.cpp
- libs/core/resource_partitioner/include/hpx/resource_partitioner/partitioner_fwd.hpp
- libs/core/threading/src/thread.cpp
- libs/full/init_runtime/src/hpx_init.cpp
- libs/core/resource_partitioner/src/detail_partitioner.cpp
- libs/core/synchronization/tests/unit/latch_notification.cpp
Collaborator
Performance test reportHPX PerformanceComparison
Info
Comparison
Info
Comparison
Info
Explanation of Symbols
|
- flyby: fix command line handling of --hpx:queuing option - flyby: fixing races during hpx::thread::join Signed-off-by: Hartmut Kaiser <hartmut.kaiser@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
libs/core/init_runtime_local/src/init_runtime_local.cpp (1)
503-506: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCleanup call is correct; message prefix now inconsistent with a sibling message.
delete_partitioner()before reporting is a good fix and safe even if the partitioner was never constructed. Separately, the prefix was corrected here fromhpx::init:tohpx::local::init:, butensure_no_runtime_is_up()(Line 421-423, unchanged) still emits"hpx::init: can't initialize runtime system...", so the two error paths in this same file/namespace now disagree on the prefix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/core/init_runtime_local/src/init_runtime_local.cpp` around lines 503 - 506, Align the error prefix in ensure_no_runtime_is_up() with the local initialization path by changing its hpx::init prefix to hpx::local::init:. Keep the existing message text and cleanup behavior unchanged.libs/core/schedulers/tests/unit/lockfree_fifo_double_backend.cpp (1)
254-327: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the shared stealer/owner consumer loop.
The stealer-thread lambda and the owner-thread lambda are nearly identical (poll/steal-vs-not, stall-timeout check, break condition), differing only in the
stealflag, thecollectedindex, and the failure message. Factoring the common loop into a helper takingbool stealand acollectedindex would remove ~35 lines of duplication.♻️ Sketch of a shared helper
+auto make_consumer = [&backend, &producers_done, &collected, stress_stall_timeout]( + std::size_t idx, bool steal) { + return [&backend, &producers_done, &collected, idx, steal, stress_stall_timeout]() { + std::uint64_t val = 0; + auto last_progress = std::chrono::steady_clock::now(); + while (true) + { + if (backend.pop(val, steal)) + { + collected[idx].push_back(val); + last_progress = std::chrono::steady_clock::now(); + } + else if (producers_done.load(std::memory_order_acquire) && + backend.empty()) + { + break; + } + else + { + if (std::chrono::steady_clock::now() - last_progress > + stress_stall_timeout) + { + HPX_TEST_MSG(false, + (steal ? "stealer" : "owner") + + std::string(" thread stalled: no progress for 30s")); + return; + } + std::this_thread::yield(); + } + } + }; +};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/core/schedulers/tests/unit/lockfree_fifo_double_backend.cpp` around lines 254 - 327, Extract the duplicated consumer polling logic from the stealer and owner lambdas into a shared helper that accepts the steal flag, collected-vector index, and thread-role label or failure-message context. Update both consumer creations to invoke this helper while preserving their existing pop behavior, termination condition, progress tracking, stall timeout, and diagnostics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/core/threading_base/tests/unit/thread_exit_callbacks.cpp`:
- Around line 7-11: Update the header comment in the thread exit-callback tests
to remove references to the deleted has_exit_funcs_ atomic and lock-free fast
path. Describe the current synchronization model instead:
run_thread_exit_callbacks() and free_thread_exit_callbacks() acquire mtx_, while
running_exit_funcs_ and ran_exit_funcs_ guard callback state.
---
Nitpick comments:
In `@libs/core/init_runtime_local/src/init_runtime_local.cpp`:
- Around line 503-506: Align the error prefix in ensure_no_runtime_is_up() with
the local initialization path by changing its hpx::init prefix to
hpx::local::init:. Keep the existing message text and cleanup behavior
unchanged.
In `@libs/core/schedulers/tests/unit/lockfree_fifo_double_backend.cpp`:
- Around line 254-327: Extract the duplicated consumer polling logic from the
stealer and owner lambdas into a shared helper that accepts the steal flag,
collected-vector index, and thread-role label or failure-message context. Update
both consumer creations to invoke this helper while preserving their existing
pop behavior, termination condition, progress tracking, stall timeout, and
diagnostics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 01da29c3-b59c-4b28-9b31-ce89409c2f74
📒 Files selected for processing (16)
libs/core/init_runtime_local/src/init_runtime_local.cpplibs/core/resource_partitioner/include/hpx/resource_partitioner/partitioner.hpplibs/core/resource_partitioner/include/hpx/resource_partitioner/partitioner_fwd.hpplibs/core/resource_partitioner/src/detail_partitioner.cpplibs/core/resource_partitioner/tests/unit/CMakeLists.txtlibs/core/resource_partitioner/tests/unit/scheduling_policy_wiring.cpplibs/core/schedulers/tests/unit/CMakeLists.txtlibs/core/schedulers/tests/unit/lockfree_fifo_double_backend.cpplibs/core/synchronization/tests/unit/CMakeLists.txtlibs/core/synchronization/tests/unit/latch_notification.cpplibs/core/threading/src/thread.cpplibs/core/threading_base/include/hpx/threading_base/thread_data.hpplibs/core/threading_base/src/thread_data.cpplibs/core/threading_base/tests/unit/CMakeLists.txtlibs/core/threading_base/tests/unit/thread_exit_callbacks.cpplibs/full/init_runtime/src/hpx_init.cpp
🚧 Files skipped from review as they are similar to previous changes (9)
- libs/core/threading_base/tests/unit/CMakeLists.txt
- libs/full/init_runtime/src/hpx_init.cpp
- libs/core/schedulers/tests/unit/CMakeLists.txt
- libs/core/resource_partitioner/include/hpx/resource_partitioner/partitioner_fwd.hpp
- libs/core/resource_partitioner/tests/unit/scheduling_policy_wiring.cpp
- libs/core/resource_partitioner/include/hpx/resource_partitioner/partitioner.hpp
- libs/core/resource_partitioner/src/detail_partitioner.cpp
- libs/core/threading/src/thread.cpp
- libs/core/synchronization/tests/unit/latch_notification.cpp
Collaborator
Performance test reportHPX PerformanceComparison
Info
Comparison
Info
Comparison
Info
Explanation of Symbols
|
Collaborator
Performance test reportHPX PerformanceComparison
Info
Comparison
Info
Comparison
Info
Explanation of Symbols
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libs/core/schedulers/tests/unit/lockfree_fifo_double_backend.cpp (1)
240-252: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winProducer push loop has no stall/timeout guard, unlike consumers.
The consumer loop (lines 229-288) explicitly adds a 30s stall-timeout to avoid a silent CI hang if an item is ever lost. The producer retry loop (
while (!backend.push(...)) yield();) has no equivalent bound — ifpush()ever fails to make progress (e.g. a capacity/parity bug under concurrent toggling), the producer spins forever andt.join()at line 305 blocks indefinitely, turning the failure into an opaque CI timeout instead of the actionableHPX_TEST_MSGfailure this test otherwise aims for.Example: bound the producer retry with the same stall-detection pattern used for consumers
producers.emplace_back([&backend, p]() { std::uint64_t const base = p * items_per_producer; + auto last_progress = std::chrono::steady_clock::now(); for (std::uint64_t i = 0; i != items_per_producer; ++i) { - while (!backend.push(base + i)) - { - std::this_thread::yield(); - } + while (!backend.push(base + i)) + { + if (std::chrono::steady_clock::now() - last_progress > + stress_stall_timeout) + { + HPX_TEST_MSG(false, "producer thread stalled: " + "no progress for 30s (possible push bug)"); + return; + } + std::this_thread::yield(); + } + last_progress = std::chrono::steady_clock::now(); } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/core/schedulers/tests/unit/lockfree_fifo_double_backend.cpp` around lines 240 - 252, Update the producer retry loop in the producer thread setup to use the same 30-second stall-detection and failure-reporting pattern as the consumer loop, replacing the unbounded while (!backend.push(...)) spin. Ensure stalled pushes trigger an actionable HPX_TEST_MSG failure and allow the thread to exit so t.join() cannot hang indefinitely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@libs/core/schedulers/tests/unit/lockfree_fifo_double_backend.cpp`:
- Around line 240-252: Update the producer retry loop in the producer thread
setup to use the same 30-second stall-detection and failure-reporting pattern as
the consumer loop, replacing the unbounded while (!backend.push(...)) spin.
Ensure stalled pushes trigger an actionable HPX_TEST_MSG failure and allow the
thread to exit so t.join() cannot hang indefinitely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e9ecb2b7-499f-474e-8377-c81a5e8f685f
📒 Files selected for processing (16)
libs/core/init_runtime_local/src/init_runtime_local.cpplibs/core/resource_partitioner/include/hpx/resource_partitioner/partitioner.hpplibs/core/resource_partitioner/include/hpx/resource_partitioner/partitioner_fwd.hpplibs/core/resource_partitioner/src/detail_partitioner.cpplibs/core/resource_partitioner/tests/unit/CMakeLists.txtlibs/core/resource_partitioner/tests/unit/scheduling_policy_wiring.cpplibs/core/schedulers/tests/unit/CMakeLists.txtlibs/core/schedulers/tests/unit/lockfree_fifo_double_backend.cpplibs/core/synchronization/tests/unit/CMakeLists.txtlibs/core/synchronization/tests/unit/latch_notification.cpplibs/core/threading/src/thread.cpplibs/core/threading_base/include/hpx/threading_base/thread_data.hpplibs/core/threading_base/src/thread_data.cpplibs/core/threading_base/tests/unit/CMakeLists.txtlibs/core/threading_base/tests/unit/thread_exit_callbacks.cpplibs/full/init_runtime/src/hpx_init.cpp
🚧 Files skipped from review as they are similar to previous changes (15)
- libs/core/threading_base/tests/unit/CMakeLists.txt
- libs/core/schedulers/tests/unit/CMakeLists.txt
- libs/core/resource_partitioner/tests/unit/scheduling_policy_wiring.cpp
- libs/core/resource_partitioner/tests/unit/CMakeLists.txt
- libs/core/resource_partitioner/include/hpx/resource_partitioner/partitioner_fwd.hpp
- libs/core/init_runtime_local/src/init_runtime_local.cpp
- libs/core/threading_base/tests/unit/thread_exit_callbacks.cpp
- libs/core/resource_partitioner/src/detail_partitioner.cpp
- libs/core/synchronization/tests/unit/CMakeLists.txt
- libs/core/threading_base/include/hpx/threading_base/thread_data.hpp
- libs/core/threading/src/thread.cpp
- libs/core/resource_partitioner/include/hpx/resource_partitioner/partitioner.hpp
- libs/full/init_runtime/src/hpx_init.cpp
- libs/core/threading_base/src/thread_data.cpp
- libs/core/synchronization/tests/unit/latch_notification.cpp
Collaborator
Performance test reportHPX PerformanceComparison
Info
Comparison
Info
Comparison
Info
Explanation of Symbols
|
hkaiser
force-pushed
the
fixing_7111
branch
2 times, most recently
from
July 13, 2026 13:51
8595bee to
d2c8e4d
Compare
Collaborator
Performance test reportHPX PerformanceComparison
Info
Comparison
Info
Comparison
Info
Explanation of Symbols
|
- flyby: fixing more nitpicks reported by CodeRabbit Signed-off-by: Hartmut Kaiser <hartmut.kaiser@gmail.com>
Collaborator
Performance test reportHPX PerformanceComparison
Info
Comparison
Info
Comparison
Info
Explanation of Symbols
|
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
2 participants
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.
Allow to reduce the work-stealing overheads by minimizing cache interference. The idea is to add a 'passive' queue next to each 'active' one. The 'active' queue is used as long as it holds threads to run. As soon as it runs empty the scheduler swaps the queues and continues executing threads from the now 'active' (formerly 'passive') queue. New threads are scheduled on the currently 'passive' queue. Work-stealing happens from the 'passive' queue first and only if that was unsuccessful tries to steal from the 'active' queues (as before).
Fixes #7111
Depends on #7367
As flyby this adds a fix for a significant performance regression and fixes a race in hpx::thread;;join.