Conversation
Test Results for Commit e646703Pull Request 4950: Results Test Case Results
Last updated: 2025-09-24 15:11:02 UTC |
pwojcikdev
force-pushed
the
optimize-thread-pool
branch
from
7f9626a to
e45ba35
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR reworks the thread_pool class to implement a high-performance version that avoids condition variable overhead under heavy task processing loads, while preserving the original implementation as timed_thread_pool.
- Introduced a new optimized
thread_poolclass with spinning workers and batch processing - Renamed the existing thread pool implementation to
timed_thread_pool - Updated node components to use
timed_thread_poolwhere time-sensitive operations are needed - Removed
bootstrap_workersreferences from the node class
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| nano/lib/thread_pool.hpp | Implements new optimized thread_pool class and renames existing to timed_thread_pool |
| nano/node/pruning.hpp | Updates pruning to use timed_thread_pool instead of thread_pool |
| nano/node/node.hpp | Updates node worker references to use timed_thread_pool and removes bootstrap_workers |
| nano/node/node.cpp | Updates constructor and cleanup code to use timed_thread_pool and removes bootstrap_workers |
| nano/core_test/thread_pool.cpp | Adds comprehensive tests for new thread_pool and updates existing tests for timed_thread_pool |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| ~thread_pool () | ||
| ~timed_thread_pool () | ||
| { | ||
| // Must be stopped before destruction to avoid running takss when node components are being destroyed |
There was a problem hiding this comment.
There's a typo in the comment: 'takss' should be 'tasks'.
| // Must be stopped before destruction to avoid running takss when node components are being destroyed | |
| // Must be stopped before destruction to avoid running tasks when node components are being destroyed |
| template <typename F> | ||
| void post (F && task) | ||
| { | ||
| if (stopped) |
There was a problem hiding this comment.
Reading stopped without synchronization could lead to race conditions. This should be stopped.load() for proper atomic access.
| if (stopped) | |
| if (stopped.load()) |
| { | ||
| // Spinning phase before blocking | ||
| bool found_work = false; | ||
| for (int spin = 0; spin < 64; ++spin) // Moderate spin count |
There was a problem hiding this comment.
The magic number 64 for spin count should be defined as a named constant to improve maintainability and allow for easier tuning.
| auto sleeping = sleeping_workers.load (); | ||
|
|
||
| // Only notify if we have sleeping workers and queue isn't too large | ||
| if (sleeping > 0 && queue_size <= 4) |
There was a problem hiding this comment.
The magic number 4 for queue size threshold should be defined as a named constant to improve maintainability and make the heuristic more explicit.
pwojcikdev
force-pushed
the
optimize-thread-pool
branch
from
e45ba35 to
e646703
Compare
pwojcikdev
force-pushed
the
optimize-thread-pool
branch
3 times, most recently
from
26c9087 to
5e6c6aa
Compare
pwojcikdev
force-pushed
the
optimize-thread-pool
branch
from
5e6c6aa to
ee9da7f
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (stopped.load (std::memory_order_acquire)) | ||
| return; |
There was a problem hiding this comment.
Early return at line 136 without decrementing sleeping_workers counter could lead to a permanent counter drift. If the worker thread exits during the spinning phase after sleeping_workers has been incremented at line 156, the counter will remain incremented forever. Move the check before line 156 or ensure sleeping_workers is decremented before returning.
| sleeping_workers.fetch_add (1, std::memory_order_acq_rel); | ||
| { | ||
| std::unique_lock lock{ mutex }; | ||
|
|
||
| condition.wait_for (lock, wakeup_interval, [this] { | ||
| return !tasks.empty () || stopped.load (std::memory_order_acquire); | ||
| }); |
There was a problem hiding this comment.
Race condition: sleeping_workers is incremented at line 156 before acquiring the mutex at line 158. This creates a window where a posting thread could read sleeping_workers > 0, decide to notify, but the worker hasn't entered wait_for yet, causing a missed wakeup. The increment should happen inside the locked section before the wait.
| bool should_notify = false; | ||
| { | ||
| std::lock_guard lock{ mutex }; | ||
|
|
There was a problem hiding this comment.
Unlike timed_thread_pool::post() which checks if stopped before accepting tasks (line 236), thread_pool::post() does not check the stopped flag and will accept tasks even after stop() is called. This inconsistency could lead to tasks being queued but never executed. Consider adding a stopped check similar to timed_thread_pool.
| if (stopped.load (std::memory_order_acquire)) | |
| { | |
| // Do not accept new tasks if stopped | |
| return; | |
| } |
| /* | ||
| * High-performance thread pool implementation that avoids condition variable overhead under high load | ||
| */ | ||
| class thread_pool final |
There was a problem hiding this comment.
[nitpick] The class documentation should clarify when to use thread_pool versus timed_thread_pool. Based on the implementation, thread_pool is optimized for high-frequency immediate tasks with spin-locking, while timed_thread_pool supports delayed execution. Adding guidance on the trade-offs (e.g., CPU usage from spinning vs. lower latency) would help API consumers choose the right implementation.
This reworks
thread_poolclass to reduce condition variable wake up overhead during heavy tasks processing. Preserves the current implementation astimed_thread_pool.