-
-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[Misc] Clean up and consolidate LRUCache #11339
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
👋 Hi! Thank you for contributing to the vLLM project. 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 do one of these:
🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM. Meanwhile I'd like to make sure the logic is the same when pinned items are not used. cc @alexm-neuralmagic
raise RuntimeError("All items are pinned, " | ||
"cannot remove oldest from the cache.") | ||
else: | ||
lru_key = next(iter(self.cache)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since mm hash cache doesn't need the pin logic, could we optimize this function when there's no pinned items? For example
if remove_pinned and self.pinned_items:
...
else:
lru_key = ...
Also I'm wondering whether to use .popitem(last=False) would be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this optimization is somewhat unnecessary, as the first key will be returned if pinned_items
is empty. So either way, we will check for the emptiness of pinned_items
once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC in the branch with remove_pinned is False, we iterative an entire cache anyways whatever pinned_items are empty or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The next()
statement only consumes the first item in the generator comprehension, it doesn't consume the generator fully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah you're right. Miss the point that it's a generator. Should be good then
Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
LRUDictCache
andLRUCache
are fundamentally the same, so I merged them together. I also improved the existing type annotations to be more precise.