Skip to content
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

feat: adds remove functionality to LRUCache and LRUMap #154

Closed
wants to merge 13 commits into from
Closed

feat: adds remove functionality to LRUCache and LRUMap #154

wants to merge 13 commits into from

Conversation

trivikr
Copy link
Contributor

@trivikr trivikr commented May 5, 2021

Fixes: #143

This PR adds remove functionality to LRUCache and LRUMap.

Changes done:

  • Added remove functionality to LRUCache and LRUMap.
  • Added tests.
  • Updates types.

@trivikr trivikr mentioned this pull request May 5, 2021
@Yomguithereal
Copy link
Owner

Hello @trivikr. Thanks for your PR. The reason why I advocated for an ObliviousLRUCache class in issue #143 is because this lib's LRUCache is performance-critical and is used in industrial code that needs to be the fastest possible all while consuming the least amount of memory possible. Here the fact is adding #.remove (by the way, I think this method should be named #.delete to remain consistent with ES6 Map as well as returning a boolean indicating if anything was deleted, what do you think?) adds cruft to the #.set method (which is one of the most critical) as well as adding O(n) memory, as you mention in #143. I'll elaborate a bit on #160 but the fact is, what you see as being redundant code is often redundant because abstraction have a cost that was found as having a significant performance cost when benchmarked.

This said, I am bothered, as you were, by the redundancy of the code because it makes all of this more difficult to maintain. But the fact is I am not sold on using runtime abstraction to solve the issue at the cost of performance and maybe some macro/preprocessor tool can help?

@trivikr
Copy link
Contributor Author

trivikr commented May 6, 2021

The reason why I advocated for an ObliviousLRUCache class in issue #143 is because this lib's LRUCache is performance-critical and is used in industrial code that needs to be the fastest possible all while consuming the least amount of memory possible.

Created new PR to add delete functionality in ObliviousLRUCache class at #162

About reusing code, we can continue discussing on relevant issues #159 and #160

@trivikr trivikr closed this May 6, 2021
@trivikr trivikr deleted the lru-cache-remove branch May 6, 2021 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LRU remove by key
2 participants