Correct prefix-caching ref-count accounting - #6047
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Signed-off-by: Teodor-Dumitru Ene <teodord.ene@gmail.com>
071d064 to
6f0dd71
Compare
|
@lmcafee-nvidia, we missed this in our testing. |
sidsingh-nvidia
left a comment
There was a problem hiding this comment.
Great find, thanks!
|
/claude strict-review |
There was a problem hiding this comment.
Strict review passed — no significant issues found. LGTM
Verified the fix addresses a real ref-count leak: self.block_ref_counts[blocks] -= 1 only decrements once per unique index under advanced indexing, so a prefix block shared by multiple requests finishing in the same batched release was under-decremented and leaked. scatter_add_ correctly accumulates per occurrence (the deterministic-accumulate op).
Also confirmed:
- The added
torch.uniqueon both the REF_ZERO deregister path and the LRU pool-return path fixes a companion double-free (a duplicated block ID reaching ref 0 would otherwise be returned to the pool / deregistered twice). - dtype/device are consistent: int32 self + int32 source + int64 index on CPU, matching
scatter_add_requirements; no new device constraint over the prior indexing. -1sentinels are filtered by the caller before reaching the scatter, so no out-of-range index.
The new unit test is thorough — staged multi-owner release, in-batch duplication with a private block mixed in, and both eviction policies, including a set-uniqueness assertion that guards against double-returned IDs.
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/30383126962 |
What does this PR do?
Issue tracking
For PRs from open-source community contributors:
Linked issue:
Contribution process
Pre-checks
Code review
Feel free to message or comment @NVIDIA/mcore-oncall to help accelerate your merge into main. The less complex your PR is, the faster it will be approved and merged!
All PRs start as draft. If you open a non-draft PR, it will be automatically converted to draft.
Step 1: Mark PR as "Ready for Review"
.github/CODEOWNERS.Final Review might get declined if these requirements are not fulfilled.
Step 2: Final Review
For PRs that change
megatron/core, once all expert reviewers have approved, theFinal Reviewlabel is applied automatically and final reviewers are assigned.For PRs outside
megatron/core, this step is skipped.Step 3: Approved
Once all required reviewers have approved, the
Approvedlabel is applied automatically.Merge
Any member of mcore-engineers will be able to merge your PR.