[Core] Minimize number of dict lookup in _maybe_evict_cached_block #21281

Jialin · 2025-07-21T05:24:50Z

Essential Elements of an Effective PR Description Checklist

The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
The test plan, such as providing test command.
The test results, such as pasting the results comparison before and after, or e2e results
(Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.

Purpose

With #21222, we noticed that _maybe_evict_cached_block became the new bottleneck for allocate_new_block.

And we notice that, we do duplicated key lookup against the same dict 4 times, and we could easily cut it down into 2.

Test Plan

Compare trace with and without the change on top of #21222

Test Result

Quite significant improvements 0.13ms -> 0.03ms

After

Before

(Optional) Documentation Update

github-actions · 2025-07-21T05:24:57Z

👋 Hi! Thank you for contributing to the vLLM project.

💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels.

Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging.

To run CI, PR reviewers can either: Add ready label to the PR or enable auto-merge.

🚀

gemini-code-assist

Code Review

This pull request optimizes _maybe_evict_cached_block by reducing dictionary lookups, which was identified as a performance bottleneck. The changes are logically sound, achieving the intended performance improvement by using .get() and .pop() to minimize lookups. The new implementation is also more robust by handling cases where a block ID might not be present in the cache, avoiding potential KeyError exceptions. The code is correct under the assumption of single-threaded access, which is consistent with the vLLM scheduler's design. Overall, this is a solid improvement.

Jialin · 2025-07-21T07:41:46Z

Resolve #21141

njhill

Minor comment, otherwise lgtm

njhill · 2025-07-21T08:59:30Z

vllm/v1/core/block_pool.py

+        if len(blocks_by_id) == 0:
+            self.cached_block_hash_to_block.pop(block_hash)


Why not del?

Suggested change

if len(blocks_by_id) == 0:

self.cached_block_hash_to_block.pop(block_hash)

if blocks_by_id:

del cached_block_hash_to_block.pop[block_hash]

For del, it makes a lot sense, as we don't rely on the returned values;

But for the bool check, I thought len(blocks_by_id) == 0 would be alightly more efficient? I could imagine the bool stack would be bool -> len(blocks_by_id) == 0. So maybe the original approach would avoid one less function call.

However, the difference of the second bullet might be very minimum, so updating the commit per you comments.

yeah the bool check is more a style thing, only a suggestion

houseroad

Overall, looks good. Only two comments to address.

vllm/v1/core/block_pool.py

Signed-off-by: Jialin Ouyang <Jialin.Ouyang@gmail.com>

simon-mo · 2025-07-22T07:54:37Z

vllm/v1/core/block_pool.py

+            return False
+        block.reset_hash()
+        blocks_by_id.pop(block.block_id, None)
+        if blocks_by_id:


This is wrong, fixed in #21357

Sorry about this :( had meant to suggest not blocks_by_id above.

Interesting this didn’t trigger any unit test failure 🤣? I think we should fix that @Jialin

Thanks @simon-mo for the fix forward. My bad. I should be more careful when updating the logic.

There you go, at least #21400 should help capturing some basic failures.

I will take some blame as well, didn't notice the issue in the review.

No worries, thx for adding the tests.