Conversation
Signed-off-by: zyguan <zhongyangguan@gmail.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThe PR introduces task acquisition tracking by adding Changes
Sequence DiagramsequenceDiagram
participant Worker as Worker
participant LocalQueue as Local Queue
participant Injector as Global Injector
participant OtherWorker as Other Worker
participant TaskExtras as Task Extras
Worker->>Worker: pop(retry_after_park=false)<br/>state=Immediate
Worker->>LocalQueue: Try local pop<br/>(LocalQueue source)
alt Local pop succeeds
LocalQueue-->>Worker: (Pop, TaskSource::LocalQueue)
Worker->>TaskExtras: Set task_source & acquire_state
Note over TaskExtras: source=LocalQueue<br/>state=Immediate
else Local pop fails & spin
Worker->>Worker: Advance state<br/>to AfterSpin
Worker->>Injector: Steal from global<br/>(GlobalQueue source)
alt Global steal succeeds
Injector-->>Worker: (Pop, TaskSource::GlobalQueue)
Worker->>TaskExtras: Set task_source & acquire_state
Note over TaskExtras: source=GlobalQueue<br/>state=AfterSpin
else Exhausted, park
Worker->>Worker: Park thread
Worker->>Worker: Resume & set state<br/>to AfterPark
Worker->>OtherWorker: Steal from worker<br/>(OtherWorker source)
OtherWorker-->>Worker: (Pop, TaskSource::OtherWorker)
Worker->>TaskExtras: Set task_source & acquire_state
Note over TaskExtras: source=OtherWorker<br/>state=AfterPark
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
Signed-off-by: zyguan <zhongyangguan@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR expands task scheduling observability by introducing TaskSource (where a task was popped from) and AcquireState (how a worker acquired a task), then propagating these signals through queue Pop results and into Extras for inspection by runners.
Changes:
- Add
TaskSourceandAcquireStateenums and expose them viaqueue::Extrasaccessors. - Replace
Pop::from_localwithPop::task_sourceacross queue implementations and add tests validating task source attribution. - Track acquisition state in
WorkerThreadand persist both acquisition state and task source intoExtrasbefore handing tasks to the runner.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/queue/extras.rs | Introduces TaskSource/AcquireState and stores them in Extras with public getters. |
| src/queue.rs | Re-exports new enums and changes Pop to carry task_source instead of from_local. |
| src/queue/single_level.rs | Sets task_source on pops from local/global/steal paths; adds tests. |
| src/queue/multilevel.rs | Sets task_source on pops from local/global/steal paths; adds tests. |
| src/queue/priority.rs | Sets task_source for priority queue pops (always global); adds test. |
| src/pool/worker.rs | Computes AcquireState during pop/park flow and writes AcquireState + TaskSource into Extras; adds tests. |
| src/pool/spawn.rs | Suppresses dead_code warnings for compile-time Sync/Send assertion traits. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// The source of the task, indicating where the task comes from. | ||
| pub task_source: TaskSource, | ||
| } | ||
|
|
There was a problem hiding this comment.
Pop is a public struct; replacing the public from_local: bool field with task_source: TaskSource is a breaking API change for downstream users. If backward compatibility is needed, consider keeping from_local (possibly deprecated) or adding a from_local() accessor/compat shim while introducing task_source.
| impl<T> Pop<T> { | |
| /// Returns whether this task was popped from a local queue. | |
| /// | |
| /// This provides a compatibility shim for the former `from_local: bool` | |
| /// field, using the newer `task_source` information instead. | |
| #[deprecated( | |
| since = "0.0.0", | |
| note = "use `task_source` instead; this accessor is kept for backward compatibility" | |
| )] | |
| pub fn from_local(&self) -> bool { | |
| self.task_source.is_local() | |
| } | |
| } |
| let mut state = if retry_after_park { | ||
| AcquireState::AfterPark | ||
| } else { | ||
| AcquireState::Immediate | ||
| }; |
There was a problem hiding this comment.
When retry_after_park is true, state is initialized to AfterPark, so if a task is popped immediately in this call it will still be labeled AfterPark even though no spinning/parking happened during this acquisition. Consider deriving AcquireState based on what happened in the current pop() call (or clarify the semantics in docs), rather than carrying state across iterations via retry_after_park.
| if state == AcquireState::Immediate { | ||
| state = AcquireState::AfterSpin; | ||
| } |
There was a problem hiding this comment.
With the current transition if state == Immediate { state = AfterSpin }, AfterSpin can never be produced when retry_after_park is true (since state starts as AfterPark). If you want to capture “acquired after spinning” even after a previous park/spurious wake, update the state machine accordingly (e.g., track spinning separately or allow transitioning from AfterPark to AfterSpin when spinning occurs).
Add two new enums
TaskSourceandAcquireStateto represent where a task was popped from and how it was acquired byWorkerThread. Gather and expose these information throughExtrasto help us identify the characteristics of tasks with high scheduling delays.Summary by CodeRabbit
Release Notes
New Features
Improvements
Tests