[None][chore] Dead code elimination, we no longer record/fetch through WindowBlockManager:: mContextBlocksByHash #6249

eopXD · 2025-07-22T07:21:47Z

Description

WindowBlockManager::mCachedBlocksRoot is now who is responsible for the bookkeeping of the KVCacheBlock, and the mNextBlocks is now the actual hash map that fetches the block.

The mEnableHashKey knob and related hashing is removed.

No functional change is intended in this MR.

Summary by CodeRabbit

Refactor
- Removed hash-based block lookup and management functionality from cache components, streamlining block handling.
- Updated cache manager initialization to use dynamic configuration for partial reuse settings.
Tests
- Removed unit tests related to hash key and block reuse features in cache management.

Test Coverage

The following existing test coverages under kvCacheManagerTest.cpp are removed because we are no longer using the utility.

KVCacheManagerHashKeyTest
KVCacheManagerHashKeyWithReuseTest

coderabbitai · 2025-07-22T07:21:54Z

📝 Walkthrough

Walkthrough

All code and tests related to hash-based block lookup and management have been removed from the KV cache manager. This includes the deletion of hash map member variables, associated methods for adding/removing blocks by hash, and all related test cases and utilities. Additionally, a constructor argument for block reuse configuration was updated to use the actual config value instead of a hardcoded false. No other logic or control flow was changed.

Changes

File(s)	Change Summary
Header cleanup `cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h`	Removed all hash-based block lookup methods, variables, and related interface declarations.
Implementation cleanup `cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp`	Deleted all hash map management logic and all calls to hash-based block add/remove methods; updated constructor calls to remove `enableHashKey` parameter.
Test removal `cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp`	Removed all tests and helpers related to hash key and block reuse functionality.
Constructor argument update `cpp/tensorrt_llm/batch_manager/trtGptModelInflightBatching.cpp`	Changed hardcoded `false` argument in `KVCacheManager` constructor call to use `kvCacheConfig.getEnablePartialReuse()`.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Suggested reviewers

zhengd-nv
Funatiq

Poem

A hash map once lived in our code,
For blocks it would swiftly decode.
But now it is gone,
Its duties withdrawn,
Our cache takes a simpler new road.

🐇✨

Note

🔌 MCP (Model Context Protocol) integration is now available in Early Access!

Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context.

📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e56ce75 and db957fc.

📒 Files selected for processing (4)

cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h (4 hunks)
cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp (7 hunks)
cpp/tensorrt_llm/batch_manager/trtGptModelInflightBatching.cpp (1 hunks)
cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp (0 hunks)

💤 Files with no reviewable changes (1)

cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp

🚧 Files skipped from review as they are similar to previous changes (3)

cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h
cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp
cpp/tensorrt_llm/batch_manager/trtGptModelInflightBatching.cpp

✨ Finishing Touches

📝 Generate Docstrings

🧪 Generate unit tests

Create PR with unit tests
Post copyable unit tests in a comment

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.

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Explain this complex logic.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai explain this code block.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and explain its main purpose.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai generate docstrings to generate docstrings for this PR.
@coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
@coderabbitai generate unit tests to generate unit tests for this PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

eopXD · 2025-07-22T07:24:31Z

@thorjohnsen I was introduced by @symphonylyh that you are the one to consult and add as a reviewer.

Tracing through the code, this structure looks dead to me, we should stop maintaining this utility (adding addBlockToHashMap/removeBlockFromHashMap) to reduce complexity of the code base.

thorjohnsen · 2025-07-24T15:36:45Z

Please sign off your commit, we won't be able to merge without.

thorjohnsen

WindowBlockManager::mCachedBlocksRoot is now who is responsible
for the bookkeeping of the KVCacheBlock, and the mNextBlocks is now
the actual hash map that fetches the block.

Has this change been merged into main already?

eopXD · 2025-07-28T14:59:34Z

WindowBlockManager::mCachedBlocksRoot is now who is responsible
for the bookkeeping of the KVCacheBlock, and the mNextBlocks is now
the actual hash map that fetches the block.

Has this change been merged into main already?

Yes, it is already merged into main.

https://github.com/NVIDIA/TensorRT-LLM/blob/main/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp#L1037
https://github.com/NVIDIA/TensorRT-LLM/blob/main/cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp#L1072

eopXD · 2025-07-28T15:04:28Z

Please sign off your commit, we won't be able to merge without.

Updated commit. Thank you for the reminder.

Funatiq · 2025-07-29T13:50:50Z

TBH I never really understood why we need this hashmap, it was introduced in MR 7247 as a preliminary for TRTLLM-1619 as a part of disaggregated serving. @zhengd-nv could you comment please?

zhengd-nv · 2025-07-30T01:42:27Z

This is a preliminary step of transmitting arbitrary kv cache blocks between workers and the hash id is used to identify which blocks to transfer. The feature is delayed due to pytorch integration. @Tabrizian is now working on the design and could you comment if such lookup table is still required.

eopXD · 2025-07-30T03:14:42Z

This is a preliminary step of transmitting arbitrary kv cache blocks between workers and the hash id is used to identify which blocks to transfer. The feature is delayed due to pytorch integration. @Tabrizian is now working on the design and could you comment if such lookup table is still required.

Why is code added with no real responsibility, and not along the feature to introduce it?
@Tabrizian Please share design doc and roadmap on this.

eopXD · 2025-08-05T02:25:48Z

@Tabrizian @thorjohnsen A gentle ping, can we merge this?

Tabrizian · 2025-08-05T16:27:05Z

I think it is ok to remove it for now. We can add it back once the feature is completed.

eopXD · 2025-08-07T08:20:07Z

/bot run

tensorrt-cicd · 2025-08-07T08:25:40Z

PR_Github #14427 [ run ] triggered by Bot

tensorrt-cicd · 2025-08-07T08:41:49Z

PR_Github #14427 [ run ] completed with state FAILURE
/LLM/main/L0_MergeRequest_PR pipeline #10906 completed with status: 'FAILURE'

eopXD · 2025-08-07T09:15:44Z

/bot run

tensorrt-cicd · 2025-08-07T09:20:54Z

PR_Github #14437 [ run ] triggered by Bot

tensorrt-cicd · 2025-08-07T09:39:02Z

PR_Github #14437 [ run ] completed with state FAILURE
/LLM/main/L0_MergeRequest_PR pipeline #10912 completed with status: 'FAILURE'

eopXD · 2025-08-07T10:08:34Z

/bot run

tensorrt-cicd · 2025-08-07T10:13:57Z

PR_Github #14455 [ run ] triggered by Bot

tensorrt-cicd · 2025-08-07T10:29:02Z

PR_Github #14455 [ run ] completed with state FAILURE
/LLM/main/L0_MergeRequest_PR pipeline #10923 completed with status: 'FAILURE'

eopXD · 2025-08-07T10:41:57Z

/bot run

tensorrt-cicd · 2025-08-07T10:48:14Z

PR_Github #14460 [ run ] triggered by Bot

eopXD · 2025-08-07T10:51:35Z

/bot run

tensorrt-cicd · 2025-08-07T10:57:26Z

PR_Github #14464 [ run ] triggered by Bot

tensorrt-cicd · 2025-08-07T10:57:28Z

PR_Github #14460 [ run ] completed with state ABORTED

tensorrt-cicd · 2025-08-07T13:23:09Z

PR_Github #14464 [ run ] completed with state SUCCESS
/LLM/main/L0_MergeRequest_PR pipeline #10929 completed with status: 'FAILURE'

eopXD · 2025-08-07T14:10:40Z

/bot run

tensorrt-cicd · 2025-08-07T14:16:40Z

PR_Github #14481 [ run ] triggered by Bot

tensorrt-cicd · 2025-08-07T14:58:56Z

PR_Github #14481 [ run ] completed with state SUCCESS
/LLM/main/L0_MergeRequest_PR pipeline #10939 completed with status: 'FAILURE'

eopXD · 2025-08-08T06:19:47Z

/bot run --disable-fail-fast

tensorrt-cicd · 2025-08-08T06:31:18Z

PR_Github #14574 [ run ] triggered by Bot

tensorrt-cicd · 2025-08-08T14:20:54Z

PR_Github #14574 [ run ] completed with state SUCCESS
/LLM/main/L0_MergeRequest_PR pipeline #11010 completed with status: 'FAILURE'

…he blocks with hashmap WindowBlockManager::mCachedBlocksRoot is responsible for the bookkeeping of the KVCacheBlock, and the mNextBlocks is now the actual hash map that fetches the block. Signed-off-by: eopXD <[email protected]>

eopXD · 2025-08-10T06:47:35Z

/bot run --disable-fail-fast

tensorrt-cicd · 2025-08-10T06:52:47Z

PR_Github #14700 [ run ] triggered by Bot

tensorrt-cicd · 2025-08-10T13:09:56Z

PR_Github #14700 [ run ] completed with state SUCCESS
/LLM/main/L0_MergeRequest_PR pipeline #11096 completed with status: 'SUCCESS'

svc-trtllm-gh-bot added the Community want to contribute PRs initiated from Community label Jul 22, 2025

symphonylyh requested review from Funatiq and thorjohnsen July 22, 2025 19:14

symphonylyh added Generic Runtime General operational aspects of TRTLLM execution not in other categories. and removed Community want to contribute PRs initiated from Community labels Jul 22, 2025

svc-trtllm-gh-bot added the Community want to contribute PRs initiated from Community label Jul 22, 2025

thorjohnsen reviewed Jul 24, 2025

View reviewed changes

eopXD force-pushed the kv-cache-dce branch from c4b2392 to 7f27857 Compare July 28, 2025 14:50

eopXD force-pushed the kv-cache-dce branch from 7f27857 to 19d4026 Compare July 28, 2025 15:00

Funatiq requested a review from zhengd-nv July 29, 2025 13:50

zhengd-nv requested a review from Tabrizian July 30, 2025 01:42

Tabrizian force-pushed the kv-cache-dce branch from 19d4026 to 5b27f3c Compare August 5, 2025 16:27

Tabrizian approved these changes Aug 5, 2025

View reviewed changes

eopXD changed the title ~~[KV Cache Manager] Dead code elimination, we no longer record/fetch through WindowBlockManager:: mContextBlocksByHash~~ [None][chore][kv cache manager] Dead code elimination, we no longer record/fetch through WindowBlockManager:: mContextBlocksByHash Aug 6, 2025

eopXD changed the title ~~[None][chore][kv cache manager] Dead code elimination, we no longer record/fetch through WindowBlockManager:: mContextBlocksByHash~~ [None][chore] Dead code elimination, we no longer record/fetch through WindowBlockManager:: mContextBlocksByHash Aug 6, 2025

eopXD force-pushed the kv-cache-dce branch from f238031 to 1cd1082 Compare August 6, 2025 08:08

eopXD force-pushed the kv-cache-dce branch from 1cd1082 to ba694eb Compare August 7, 2025 09:07

eopXD enabled auto-merge (squash) August 7, 2025 09:18

eopXD force-pushed the kv-cache-dce branch from ba694eb to 5499de9 Compare August 7, 2025 10:06

eopXD disabled auto-merge August 7, 2025 10:13

eopXD enabled auto-merge (squash) August 7, 2025 10:14

eopXD force-pushed the kv-cache-dce branch from 590af89 to e56ce75 Compare August 7, 2025 10:50

eopXD force-pushed the kv-cache-dce branch from e56ce75 to db957fc Compare August 10, 2025 06:47

eopXD merged commit 199f306 into NVIDIA:main Aug 10, 2025
4 checks passed

coderabbitai bot mentioned this pull request Aug 12, 2025

feat: Add support for KVCache transfer from KVCache reuse path #6348

Draft

[None][chore] Dead code elimination, we no longer record/fetch through WindowBlockManager:: mContextBlocksByHash #6249

[None][chore] Dead code elimination, we no longer record/fetch through WindowBlockManager:: mContextBlocksByHash #6249

Uh oh!

Conversation

eopXD commented Jul 22, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Summary by CodeRabbit

Test Coverage

Uh oh!

coderabbitai bot commented Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Suggested reviewers

Poem

Chat

Support

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

Documentation and Community

Uh oh!

eopXD commented Jul 22, 2025

Uh oh!

thorjohnsen commented Jul 24, 2025

Uh oh!

thorjohnsen left a comment

Choose a reason for hiding this comment

Uh oh!

eopXD commented Jul 28, 2025

Uh oh!

eopXD commented Jul 28, 2025

Uh oh!

Funatiq commented Jul 29, 2025

Uh oh!

zhengd-nv commented Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eopXD commented Jul 30, 2025

Uh oh!

eopXD commented Aug 5, 2025

Uh oh!

Tabrizian commented Aug 5, 2025

Uh oh!

eopXD commented Aug 7, 2025

Uh oh!

tensorrt-cicd commented Aug 7, 2025

Uh oh!

tensorrt-cicd commented Aug 7, 2025

Uh oh!

eopXD commented Aug 7, 2025

Uh oh!

tensorrt-cicd commented Aug 7, 2025

Uh oh!

tensorrt-cicd commented Aug 7, 2025

Uh oh!

eopXD commented Aug 7, 2025

Uh oh!

tensorrt-cicd commented Aug 7, 2025

Uh oh!

tensorrt-cicd commented Aug 7, 2025

Uh oh!

eopXD commented Aug 7, 2025

Uh oh!

tensorrt-cicd commented Aug 7, 2025

Uh oh!

eopXD commented Aug 7, 2025

Uh oh!

tensorrt-cicd commented Aug 7, 2025

Uh oh!

tensorrt-cicd commented Aug 7, 2025

Uh oh!

tensorrt-cicd commented Aug 7, 2025

Uh oh!

eopXD commented Aug 7, 2025

Uh oh!

tensorrt-cicd commented Aug 7, 2025

Uh oh!

tensorrt-cicd commented Aug 7, 2025

eopXD commented Jul 22, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Jul 22, 2025 •

edited

Loading

zhengd-nv commented Jul 30, 2025 •

edited

Loading