Merged
Conversation
The intrinsic linked list change (61f5066) introduced a bug where items with next==nil && prev==nil are incorrectly detected as "not in list". This affects the first/only item in the cache: - Promotion treats it as new, double-counting size - Deletion skips size decrement and onDelete callback
The intrinsic linked list change (61f5066) introduced a bug where items with next==nil && prev==nil are incorrectly detected as "not in list". This affects the first/only item in the cache: - Promotion treats it as new, double-counting size - Deletion skips size decrement and onDelete callback
The intrinsic linked list change used next==nil && prev==nil to detect items not in the list. This fails for the first/only item, which has both pointers nil while legitimately being in the list. Add an inList boolean to Item, set by Insert/Remove, and use it in doPromote/doDelete instead of checking pointer state. Fixes size double-counting on promotion and missing cleanup on delete. The item struct already uses two cache-lines, adding the inList field does not have any significant performance impact.
Owner
|
Thank you. |
Contributor
Author
|
Feedback after 4 days in production, and ~30 deploys: the memory leak is gone. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes a bug introduced in v3.0.7 where the first/only item in the cache is incorrectly handled during promotion and deletion.
Bug
The v3.0.7 release changed from external
Nodepointers to intrinsic linked list pointers (next/prevembedded inItem).The code uses
next == nil && prev == nilto detect if an item is not in the list.However, when inserting into an empty list,
Insert()returns early without settingnextorprev, leaving both asnil. This means the first/only item in the cache has both pointersnilwhile legitimately being in the list.This causes two bugs:
doPromote): A single item is incorrectly treated as "new", causingsizeto be incremented againdoDelete): The cleanup path is skipped entirely,sizeis not decremented andonDeletecallback is never called.Changes
inListboolean field toIteminListtotrueinInsert()andfalseinRemove().next/prevnil checks withinListin bothCacheandLayeredCache.Performance
The inList boolean adds a small memory overhead (~1-8 bytes per item including padding).
On amd64 the Item struct already uses 2 cache-lines, the new field does not change that.
Negligible CPU cost reduction: from two pointer comparisons to a single boolean check.