schedule dspark slim - #652
zhangxiaolei123456 merged 4 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors disaggregation state management by encapsulating various pd_hidden attributes into a new PDHiddenReqState class and moving init_disaggregation from scheduler.py to prefill.py. Feedback highlights a critical missing import of DisaggregationMode in prefill.py that would cause a runtime error. Additionally, the reviewer suggests optimizing performance by storing PDHiddenReqState as a dynamic attribute on Req instead of using a WeakKeyDictionary, and caching the state lookup in local variables to avoid redundant lookups.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| _pd_hidden_req_states = weakref.WeakKeyDictionary() | ||
|
|
||
|
|
||
| def get_pd_hidden_req_state(req: "Req") -> PDHiddenReqState: | ||
| state = _pd_hidden_req_states.get(req) | ||
| if state is None: | ||
| state = PDHiddenReqState() | ||
| _pd_hidden_req_states[req] = state | ||
| return state |
There was a problem hiding this comment.
Using a global weakref.WeakKeyDictionary introduces significant overhead for weak reference creation, dictionary lookups, and garbage collection tracking in the hot path of request scheduling.
Instead, we can store the PDHiddenReqState as a dynamic attribute directly on the Req object (e.g., req.pd_hidden_state). This keeps the Req class definition slim (no extra attributes initialized by default) while providing much faster, thread-safe, and direct access.
def get_pd_hidden_req_state(req: "Req") -> PDHiddenReqState:
state = getattr(req, "pd_hidden_state", None)
if state is None:
state = PDHiddenReqState()
req.pd_hidden_state = state
return state| MetadataBuffers, | ||
| ReqToMetadataIdxAllocator, | ||
| TransferBackend, |
There was a problem hiding this comment.
The DisaggregationMode enum is used in init_disaggregation (e.g., self.disaggregation_mode == DisaggregationMode.DECODE), but it is not imported in this file. This will cause a NameError at runtime. Please add DisaggregationMode to the imports from sglang.srt.disaggregation.utils.
| MetadataBuffers, | |
| ReqToMetadataIdxAllocator, | |
| TransferBackend, | |
| DisaggregationMode, | |
| MetadataBuffers, | |
| ReqToMetadataIdxAllocator, | |
| TransferBackend, |
| import threading | ||
| import weakref |
| pd_hidden_state(req).meta = None | ||
| pd_hidden_state(req).src_indices = None | ||
| pd_hidden_state(req).dst_indices = None | ||
| pd_hidden_state(req).written = None | ||
| pd_hidden_state(req).capture_layer_ids = None | ||
| pd_hidden_state(req).current_src_indices = None | ||
| pd_hidden_state(req).current_start = None | ||
| pd_hidden_state(req).current_row_len = 0 | ||
| pd_hidden_state(req).current_is_last = False | ||
| pd_hidden_state(req).owner_direct_sent = False |
There was a problem hiding this comment.
To avoid multiple lookups of the request state (whether via WeakKeyDictionary or getattr), retrieve the state once into a local variable and update its attributes.
| pd_hidden_state(req).meta = None | |
| pd_hidden_state(req).src_indices = None | |
| pd_hidden_state(req).dst_indices = None | |
| pd_hidden_state(req).written = None | |
| pd_hidden_state(req).capture_layer_ids = None | |
| pd_hidden_state(req).current_src_indices = None | |
| pd_hidden_state(req).current_start = None | |
| pd_hidden_state(req).current_row_len = 0 | |
| pd_hidden_state(req).current_is_last = False | |
| pd_hidden_state(req).owner_direct_sent = False | |
| state = pd_hidden_state(req) | |
| state.meta = None | |
| state.src_indices = None | |
| state.dst_indices = None | |
| state.written = None | |
| state.capture_layer_ids = None | |
| state.current_src_indices = None | |
| state.current_start = None | |
| state.current_row_len = 0 | |
| state.current_is_last = False | |
| state.owner_direct_sent = False |
| pd_hidden_state(req).meta = dict(dspark_meta) | ||
| pd_hidden_state(req).src_indices = [] | ||
| pd_hidden_state(req).dst_indices = [] | ||
| pd_hidden_state(req).written = [] | ||
| pd_hidden_state(req).owner_direct_sent = False |
There was a problem hiding this comment.
Retrieve the request state once into a local variable to avoid multiple redundant lookups.
| pd_hidden_state(req).meta = dict(dspark_meta) | |
| pd_hidden_state(req).src_indices = [] | |
| pd_hidden_state(req).dst_indices = [] | |
| pd_hidden_state(req).written = [] | |
| pd_hidden_state(req).owner_direct_sent = False | |
| state = pd_hidden_state(req) | |
| state.meta = dict(dspark_meta) | |
| state.src_indices = [] | |
| state.dst_indices = [] | |
| state.written = [] | |
| state.owner_direct_sent = False |
| pd_hidden_state(req).capture_layer_ids = [int(x) for x in plan.local_layer_ids] | ||
| pd_hidden_state(req).meta = dict(dspark_meta) | ||
| pd_hidden_state(req).src_indices = src_indices | ||
| pd_hidden_state(req).dst_indices = plan.dst_indices | ||
| pd_hidden_state(req).written = ( | ||
| None if plan.streaming_hidden else [False] * plan.hidden_len | ||
| ) | ||
| req.pd_hidden_owner_direct_sent = False | ||
| pd_hidden_state(req).owner_direct_sent = False |
There was a problem hiding this comment.
Retrieve the request state once into a local variable to avoid multiple redundant lookups.
| pd_hidden_state(req).capture_layer_ids = [int(x) for x in plan.local_layer_ids] | |
| pd_hidden_state(req).meta = dict(dspark_meta) | |
| pd_hidden_state(req).src_indices = src_indices | |
| pd_hidden_state(req).dst_indices = plan.dst_indices | |
| pd_hidden_state(req).written = ( | |
| None if plan.streaming_hidden else [False] * plan.hidden_len | |
| ) | |
| req.pd_hidden_owner_direct_sent = False | |
| pd_hidden_state(req).owner_direct_sent = False | |
| state = pd_hidden_state(req) | |
| state.capture_layer_ids = [int(x) for x in plan.local_layer_ids] | |
| state.meta = dict(dspark_meta) | |
| state.src_indices = src_indices | |
| state.dst_indices = plan.dst_indices | |
| state.written = ( | |
| None if plan.streaming_hidden else [False] * plan.hidden_len | |
| ) | |
| state.owner_direct_sent = False |
| if streaming_hidden: | ||
| req.pd_hidden_src_indices = None | ||
| req.pd_hidden_current_src_indices = None | ||
| req.pd_hidden_current_start = None | ||
| req.pd_hidden_current_row_len = 0 | ||
| req.pd_hidden_current_is_last = False | ||
| req.pd_hidden_owner_direct_sent = True | ||
| pd_hidden_state(req).src_indices = None | ||
| pd_hidden_state(req).current_src_indices = None | ||
| pd_hidden_state(req).current_start = None | ||
| pd_hidden_state(req).current_row_len = 0 | ||
| pd_hidden_state(req).current_is_last = False | ||
| pd_hidden_state(req).owner_direct_sent = True |
There was a problem hiding this comment.
Retrieve the request state once into a local variable to avoid multiple redundant lookups.
| if streaming_hidden: | |
| req.pd_hidden_src_indices = None | |
| req.pd_hidden_current_src_indices = None | |
| req.pd_hidden_current_start = None | |
| req.pd_hidden_current_row_len = 0 | |
| req.pd_hidden_current_is_last = False | |
| req.pd_hidden_owner_direct_sent = True | |
| pd_hidden_state(req).src_indices = None | |
| pd_hidden_state(req).current_src_indices = None | |
| pd_hidden_state(req).current_start = None | |
| pd_hidden_state(req).current_row_len = 0 | |
| pd_hidden_state(req).current_is_last = False | |
| pd_hidden_state(req).owner_direct_sent = True | |
| state = pd_hidden_state(req) | |
| if streaming_hidden: | |
| state.src_indices = None | |
| state.current_src_indices = None | |
| state.current_start = None | |
| state.current_row_len = 0 | |
| state.current_is_last = False | |
| state.owner_direct_sent = True |
| pd_hidden_state(req).current_start = write_start | ||
| pd_hidden_state(req).current_row_len = rows | ||
| pd_hidden_state(req).current_src_indices = write_indices | ||
| pd_hidden_state(req).current_is_last = write_end >= hidden_start + hidden_len | ||
| written = pd_hidden_state(req).written |
There was a problem hiding this comment.
Retrieve the request state once into a local variable to avoid multiple redundant lookups.
state = pd_hidden_state(req)
state.current_start = write_start
state.current_row_len = rows
state.current_src_indices = write_indices
state.current_is_last = write_end >= hidden_start + hidden_len
written = state.written| if has_current_pd_hidden and streaming_pd_hidden: | ||
| req.pd_hidden_src_indices = None | ||
| req.pd_hidden_current_src_indices = None | ||
| req.pd_hidden_current_start = None | ||
| req.pd_hidden_current_row_len = 0 | ||
| req.pd_hidden_current_is_last = False | ||
| pd_hidden_state(req).src_indices = None | ||
| pd_hidden_state(req).current_src_indices = None | ||
| pd_hidden_state(req).current_start = None | ||
| pd_hidden_state(req).current_row_len = 0 | ||
| pd_hidden_state(req).current_is_last = False |
There was a problem hiding this comment.
Retrieve the request state once into a local variable to avoid multiple redundant lookups.
| if has_current_pd_hidden and streaming_pd_hidden: | |
| req.pd_hidden_src_indices = None | |
| req.pd_hidden_current_src_indices = None | |
| req.pd_hidden_current_start = None | |
| req.pd_hidden_current_row_len = 0 | |
| req.pd_hidden_current_is_last = False | |
| pd_hidden_state(req).src_indices = None | |
| pd_hidden_state(req).current_src_indices = None | |
| pd_hidden_state(req).current_start = None | |
| pd_hidden_state(req).current_row_len = 0 | |
| pd_hidden_state(req).current_is_last = False | |
| state = pd_hidden_state(req) | |
| if has_current_pd_hidden and streaming_pd_hidden: | |
| state.src_indices = None | |
| state.current_src_indices = None | |
| state.current_start = None | |
| state.current_row_len = 0 | |
| state.current_is_last = False |
32990a0
into
deepseev_v4_dpsark_pd_dev
Motivation
Modifications
Accuracy Tests
Speed Tests and Profiling
Checklist
Review and Merge Process
/tag-and-rerun-ci,/tag-run-ci-label,/rerun-failed-ciCI States
Latest PR Test (Base): ❌ Run #29811258641
Latest PR Test (Extra): ❌ Run #29811258402