lukeed/flru

Addl methods?

Opened this issue · 5 comments

Could add keys, values, and/or forEach methods... but the problem is whether or not to include the "stale" (prev) cache values. These are technically on their way out & not necessarily current values, but may be. 🤔

Since a LRU is really just a set and get anyway, this may fall out of scope anyway.

Could also add a remove method, but it still falls out of scope. At least it doesn't have the same stale-vs-not concern.

Extracted snippet before release. Removing methods is breaking, but adding them is not~

remove: function (key) {
  if (curr[key] !== void 0) {
    curr[key] = void 0;
  }
  if (prev[key] !== void 0) {
    prev[key] = void 0;
  }
},
keys: function () {
  var k, arr=[];
  for (k in curr) arr.push(k);
  return arr;
},
values: function () {
  var k, arr=[];
  for (k in curr) arr.push(curr[k]);
  return arr;
},
forEach: function (fn, ctx) {
  ctx = ctx || this;
  for (var k in curr) {
    fn.call(ctx, curr[k], k, curr);
  }
}

How about extending Map, then you get those for free?

Please see tmp-cache. Does exactly this. And it’s much slower than flru as a result

I checked ;), but like your short code better, how badly do you think will this refactor affect performance?

function LRU(max) {
	let num = 0;
	let curr = new Map();
	let prev = new Map();
	const limit = max || 1;

	function keep(key, value) {
		if (++num > limit) {
			prev = curr;
			reset(1);
			num++;
		}
		curr.set(key, value);
	}

	function reset(isPartial) {
		num = 0;
		curr.clear();
		isPartial || prev.clear();
	}

	reset();

	return {
		clear: () => reset(),
		has: key => curr.has(key) || prev.has(key),
		get: key => {
			let val = curr.get(key);
			if (val !== undefined) return val;
			if (prev.has(key)) {
				val = prev.get(key);
				keep(key, val);
				return val;
			}
		},
		set: (key, value) => curr.has(key) ? curr.set(key, value) : keep(key, value)
	};
}

tmp-cache is also mine :) Initializing two Maps is costly, significantly more than object dictionaries. At scale, one can’t implement the map methods more efficiently than what Map already does, which is why the setup costs as much as it does.

That aside, you’re welcome to bench it! But Map performance aside, I don’t see any reason to need Maps in this code snippet. The same API can be done & be more performant than Map backend below a certain item count threshold

Oh, no other reason than some kind of code-OCD 🤣 , I just like the "cleanness" of Map's .has .get .clear...that's all, but if it messes up performance it's not worth it...