Updated QueueCore#ensure_workers to reduce the runtime complexity - #88
Updated QueueCore#ensure_workers to reduce the runtime complexity#88gaojieliu wants to merge 2 commits into
Conversation
|
Welcome @gaojieliu! It looks like this is your first PR to tikv/yatp 🎉 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughShort-circuits the unpark loop in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/pool/tests.rs (1)
382-396: Test relies on FIFO ordering ofparking_lot_core— document the assumption.The assertions on the unpark order (thread 3 first, then skip 6, then thread 2) depend on
parking_lot_core::unpark_filtervisiting waiters in FIFO order. This is currently true but is an implementation detail. Consider adding a brief comment near the top of the test noting this dependency so future maintainers know the test may break if the parking lot changes its queuing discipline.Also applies to: 404-418
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pool/tests.rs` around lines 382 - 396, The test for core.ensure_workers uses the observed unpark order (checking unparked_ids contents and expecting id 3 then others) which relies on parking_lot_core::unpark_filter visiting waiters in FIFO order; add a short comment above this test (near the start of the test file) documenting that the assertions depend on parking_lot_core's FIFO queuing behavior and that this is an implementation detail that may change, referencing ensure_workers and unparked_ids so future maintainers know the dependency and can adjust the test if parking_lot_core changes its ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pool/tests.rs`:
- Around line 349-367: Compute the expected parked count before spawning the
thread to avoid the race where the spawned thread increments parked_count before
the parent computes expected; specifically, move the calculation expected =
parked_count.load(Ordering::SeqCst) + 1 to just before calling thread::spawn (so
it is done prior to the spawned thread running), keep the spawned closure that
calls parked_count.fetch_add(1, Ordering::SeqCst), and then after spawn use the
existing while loop waiting for parked_count to reach expected and the short
sleep to ensure the thread entered local.pop_or_sleep and recorded into
unparked_ids.
---
Nitpick comments:
In `@src/pool/tests.rs`:
- Around line 382-396: The test for core.ensure_workers uses the observed unpark
order (checking unparked_ids contents and expecting id 3 then others) which
relies on parking_lot_core::unpark_filter visiting waiters in FIFO order; add a
short comment above this test (near the start of the test file) documenting that
the assertions depend on parking_lot_core's FIFO queuing behavior and that this
is an implementation detail that may change, referencing ensure_workers and
unparked_ids so future maintainers know the dependency and can adjust the test
if parking_lot_core changes its ordering.
|
Close it for now. |
4014e8a to
e3ceb31
Compare
e3ceb31 to
39c2d92
Compare
This function is calling parking_lot_core::unpark_filter to unpark sleeping threads and every time, it would only unpark one thread, but the original code will iterate all the parked threads regardless. Here is the original discussion: https://github.com/tikv/yatp/pull/65/changes#r809711564 The reason is that we need to take care of the scenario, where the core_thread_count is smaller than max_thread_count, so the PR owner chooses to use FilterOp::Skip to scan all the parked threads all the time, which is very inefficient. To handle all the scenarios, this PR will check whether it has already unparked one thread or not, if yes, return FilterOp::Stop, otherwise, return FilterOp::Skip. This will reduce the runtime complexity from O(n) [n: parked thread] to O(1) when core_thread_count is equal to max_thread_count. This is important since this logic is in hot path (push) and the whole unpark logic is guarded by a global lock per queue. Signed-off-by: Gaojie Liu <goliu@linkedin.com>
39c2d92 to
fac1cca
Compare
|
@BusyJay @ethercflow @sticnarf |
|
@ethercflow |
Move parked_count expected computation before thread::spawn to prevent a race where the spawned thread increments the counter before the parent reads it, which could cause the wait loop to spin forever. Also add a comment noting the test depends on parking_lot_core's FIFO waiter ordering, which is an implementation detail. Signed-off-by: Gaojie Liu <goliu@linkedin.com>
402a1da to
8c2a5e7
Compare
1 participant
This function is calling parking_lot_core::unpark_filter to unpark sleeping threads and every time, it would only unpark one thread, but the original code will iterate all the parked threads regardless. Here is the original discussion:
https://github.com/tikv/yatp/pull/65/changes#r809711564 The reason is that we need to take care of the scenario, where the core_thread_count is smaller than max_thread_count, so the PR owner chooses to use FilterOp::Skip to scan all the parked threads all the time, which is very inefficient.
To handle all the scenarios, this PR will check whether it has already unparked one thread or not, if yes, return FilterOp::Stop, otherwise, return FilterOp::Skip. This will reduce the runtime complexity from O(n) [n: parked thread] to O(1) when core_thread_count is equal to max_thread_count.
This is important since this logic is in hot path (push) and the whole unpark logic is guarded by a global lock per queue.
Summary by CodeRabbit