Oblivious LRU with delete -- addressed pull request comments, added LRUMapWithDelete #173
Conversation
|
One other change from 162 -- at the cost of one extra array reference op, the delete method returns the dead item. If that cost is meaningful I can add a ¿deletePop? method to parallel set / setPop. |
lru-cache-with-delete.js
Outdated
| // list of "holes" in the pointer array. On insert, if there is a hole | ||
| // the new pointer slots in to fill the hole; otherwise, it is | ||
| // appended as usual. (Note: we are only talking here about the | ||
| // internal pointer list. set'ing or get'ing an item promotes it |
There was a problem hiding this comment.
There is a typo on set'ing and get'ing.
lru-cache-with-delete.js
Outdated
| } | ||
|
|
||
| LRUCacheWithDelete.prototype = Object.create(LRUCache.prototype); | ||
| LRUCacheWithDelete.prototype.constructor = LRUCacheWithDelete; |
There was a problem hiding this comment.
I am not sure but I think .constructor is not enumerable, which means that the following:
for (var k in LRUCache.prototype)
LRUCacheWithDelete.prototype[k] = LRUCache.prototype[k];would to the trick less "magically"?
lru-cache-with-delete.js
Outdated
| * @return {undefined} | ||
| */ | ||
| LRUCacheWithDelete.prototype.set = function(key, value) { | ||
| this.setpop(key, value); |
There was a problem hiding this comment.
If you can bench using a proper #.set vs. relying on #.setpop internally this could be useful because I have a, maybe false, intuition that the need to create the object to return costs you a little performance and this method has to be performance-critical.
There was a problem hiding this comment.
Even if I benched it fine on say Node, I wouldn't trust that to extend to whatever browsers. I propose to un-DRY it as you did for the parent class.
lru-cache-with-delete.js
Outdated
| return undefined; | ||
| } | ||
|
|
||
| const dead = this.V[pointer]; |
There was a problem hiding this comment.
I don't have any issue with this particularly, and I don't think the array deref is such a perf hit but I notice that the equivalent in, say, an ES6 map, is to return a boolean on delete. I agree it looks more useful to return the value even though you won't be able to distinguish between an actual deletion or else if you use undefined as values (but if you do so, who are you? why are you doing this :) ?).
There was a problem hiding this comment.
Unless the repetition bothers you, I'll update my proposal to add the "returns its dead" method as remove, along with a delete method that returns a boolean. (edited -- my first response wouldn't have worked)
|
Thanks for your PR @mrflip. I have opened a small review with some comments. The unit tests look good to me. |
* un-DRY'ed set to match lru-map / lru-cache * made #delete return boolean (true if entry was present) * added #remove, which deletes item and returns its former value
|
Pushed a new commit addressing your comments. Remove lets you supply a singleton -- eg |
|
I merged the PR just now. Will release a new version in the evening I think (I need to batch up some SparseSet improvements along with it). |
|
v0.39 is now live |
This PR is all to the credit of @trivikr's PR #162 ; in order to help accelerate its inclusion in the proper release I've added commits that address the PR comments, and also added LRUMapWithDelete using the analogous strategy.
It wasn't clear to me what ObliviousLRUCache meant -- perhaps it's a term of art I haven't met yet -- but I took the liberty of renaming it to the homely but very clear LRUCacheWithDelete / LRUMapWithDelete. (Also, putting the adjective last grouped them together in the tests / indexes / etc where things are alphabetized). I'm glad to redo the commit history if ObliviousLRUCache is the proper name for this widget.
Last point -- in the comments on 162 @Yomguithereal recommended "a rolling test ... add 2 remove 1, in a loop of ~10 items for a cache of ~4-5 capacity." I tried to think of a way to give it a workout with a nice clean loop and still test it meaningfully -- like a fizzbuzz kind of thing -- but bringing in a pattern seemed too artificial and I think there'd be a lot of logic lying around. Instead, I just spammed in a whole bunch of inserts and deletes and asserted the state of the cache at various rest stops. To my mind this proved the same point that a loop would but more clearly demonstrated there was nothing up my sleeve mathematically. Does this do the trick for you?