[HiCache] Support host memory size smaller than device memory size - #16909
stmatengss wants to merge 7 commits into
Conversation
Summary of ChangesHello @stmatengss, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the HiCache system's flexibility by allowing the host memory size to be smaller than the device memory size. This is achieved through refined eviction policies that handle host memory limitations more robustly, including graceful fallback for backup failures and enabling host memory reclamation for device-resident nodes. Additionally, the prefetch capacity limit is now dynamically calculated to adapt to different host and device memory configurations, ensuring efficient memory utilization across varied hardware setups. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
/tag-and-rerun-ci |
There was a problem hiding this comment.
Code Review
This pull request introduces support for host memory being smaller than device memory in HiCache. The changes are logical and well-implemented. In cache_controller.py, the logic for calculating prefetch_capacity_limit is correctly adapted for this new scenario. In hiradix_cache.py, the evict method is improved for robustness, and the evict_host method is updated to allow evicting host memory for nodes still on the device, which is a key part of this feature. Finally, a now-invalid assertion in memory_pool_host.py is addressed. I have a couple of minor suggestions to improve code quality.
| if self.mem_pool_host.size > self.mem_pool_device.size: | ||
| self.prefetch_capacity_limit = int( | ||
| 0.8 * (self.mem_pool_host.size - self.mem_pool_device.size) | ||
| ) | ||
| else: | ||
| self.prefetch_capacity_limit = int(0.5 * self.mem_pool_host.size) |
There was a problem hiding this comment.
| # assert ( | ||
| # self.size > device_pool.size | ||
| # ), "The host memory should be larger than the device memory with the current protocol" |
There was a problem hiding this comment.
let's only skip this check when write back is selected, or when user specified smaller CPU memory size, fall back to write back policy
There was a problem hiding this comment.
Sure, we need a check here
|
/rerun-failed-ci |
|
please take a look at this PR as well: |
Resolved conflicts in: - python/sglang/srt/managers/cache_controller.py: Applied L2 < L1 fix to the new attach_storage_backend method - python/sglang/srt/mem_cache/hiradix_cache.py: Merged evictable_host_leaves tracking with conditional eviction logic Changes preserved from this branch: 1. Support for host memory (L2) smaller than device memory (L1) 2. Conditional prefetch_capacity_limit calculation based on memory sizes 3. Fallback to regular eviction when write_backup fails (returns 0) 4. Conditional tree cleanup only when node is evicted from device 5. Commented out assertion requiring host_size > device_size Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
stmatengss
left a comment
There was a problem hiding this comment.
Review: [HiCache] Support host memory size smaller than device memory size
Key Issue: evict_host new logic for non-evicted nodes is unreachable
evict_host builds its heap from self.evictable_host_leaves (line 844). But _update_host_leaf_status (line 759-772) only adds nodes where node.evicted is True — non-evicted nodes are excluded by the early return at line 760:
def _update_host_leaf_status(self, node: TreeNode):
if not node.evicted or node.lock_ref > 0:
if node in self.evictable_host_leaves:
self.evictable_host_leaves.remove(node)
returnThis means backed-up nodes still on device will never enter the eviction heap. The new branches in evict_host that handle x.evicted is False (keeping the node in the tree, only clearing host_value) are dead code.
For the PR's goal to work — freeing redundant host copies of on-device nodes when host < device — _update_host_leaf_status needs to be updated to also consider non-evicted nodes that have host_value is not None.
Minor items
-
Commented-out assertion in
memory_pool_host.py: If the constraint is intentionally lifted, delete the assertion entirely rather than commenting it out. -
Magic number
0.5inprefetch_capacity_limitfallback (cache_controller.py:456): Worth a brief comment explaining the rationale for reserving half of host memory for prefetch when host <= device. -
Tree invariant is subtle but correct: After
evict_host, a node reachable in the tree withevicted=Truewill always havehost_value != None, becauseevict_hostremoves evicted nodes from the tree when clearing theirhost_value. This invariant protectsmatch_prefix_return_kv(line 1156-1157) from hittinglen(None). A comment documenting this invariant would help future readers.
|
/rerun-failed-ci |
1 similar comment
|
/rerun-failed-ci |
|
/rerun-failed-ci |
2 similar comments
|
/rerun-failed-ci |
|
/rerun-failed-ci |
| num_evicted += self.cache_controller.evict_host(x.host_value) | ||
| x.host_value = None | ||
|
|
||
| if x.evicted: |
There was a problem hiding this comment.
does this mean we can't evict the host if the data is present on GPU?
There was a problem hiding this comment.
Data in L2 should be consistent with the GPU.
|
#20535 should be implemented more clearly. This PR might not cover complex cases. @xiezhq-hermann |
|
After discussing with @vladnosiv, I totally agree with a new |
Motivation
To support host memory size can be smaller than device memory size:
Modifications
Accuracy Tests
Benchmarking and Profiling
Checklist
Review Process
/tag-run-ci-label,/rerun-failed-ci,/tag-and-rerun-ci