3 ms·

I don't really agree, the logic around expiring values by itself is already a beneficial feature that isn't available out of the box.

jitl 4y ago
Yeah that’s true. But because it’s using the system hashmap it inherits those performance characteristics and maximum size. Map is hard limited to 2^24 = 16,777,216 keys in v8. Map can also have performance spikes that cause event loop delay when you start loading it heavily. If you expect higher load, using a two levels of map (eg Map<k1, Map<k2, V>>) can help with both issues. I went to look how the library accounted for these issues and it does not. I didn’t look to see if it uses timer coalescing or intervals, I wouldn’t want O(millions) of setTimout calls on my service either. I think it’s fine to use in the browser or a hobby server, but due to the limitations of Map I would not use it in my production server. Redis can do 2^32 = 4,294,967,296 keys (a few more), and you can do the “multi level map” by sharding your key space across multiple Redis or Memcached processes. Redis et al also has a large advantage of the cache surviving application deploy. Again, not everyone needs this but to me the main benefit of a remote cache is consistently lowering latencies, versus a in-memory cache where your latency will spike after every deploy as the cache refills.
chrismorgan 4y ago
The expiring part is very simple. For just get/set/delete and scheduled garbage collection, here’s an implementation that’s almost precisely equivalent to what KV.js offers (just with sane arguments and return types, fewer bugs, and markedly better performance due to not supporting stuff you certainly don’t need and removing unnecessary abstractions): const store = new Map(); const expiryTimes = new Map(); function get(key) { // (This works even for non-expiring keys because undefined is not less than any number.) if (expiryTimes.get(key) < Date.now()) { store.delete(key); } return store.get(key); } function set(key, value, expiry) { store.set(key, value); if (expiry) { expiryTimes.set(expiry); } } function del(key) { store.delete(key); expiryTimes.delete(key); } function flushExpired() { const now = Date.now(); for (const [key, time] of expiryTimes) { if (time < now) { del(key); } } } // Optional: automatically delete expired entries to free memory, once a second since that’s what KV.js does, but that’s probably waaaaay more often than you need. setInterval(flushExpired, 1000); Working directly with store and expiryTimes for more advanced stuff will genuinely regularly be easier than working with the KV.js abstraction.
- Etheryte 4y ago
  This comment is a good example of why "oh this is easy, I can write this myself" should not be the default approach in some cases. There's a severe bug in your set method, expiry times are not cleared when overwriting an expiring value with a value with no expiry. The library linked as the main thread does not have this bug.
  - chrismorgan 4y ago
    True, that bug exists in what I wrote. Insert an `else { expiryTimes.delete(expiry); }` at the end of set to fix it. However, I note that KV.js has similarly severe bugs (some noted in my other comment in this thread—methods that use expired values, get() mangling falsy values) and its greater complexity makes it more likely to have bugs. Very often I agree with you about the dangers of writing it yourself. But very often I also sufficiently dislike how libraries are implemented, with unnecessary bloat like excessive flexibility (all the options and such) or explicit runtime type checking rather than just letting things go wonky or blow up if given bad data. In reality, in a case like this I would actually recommend something other than a generic container. Non-expiring keys? Use variables if the keys are a finite set of known values, or a Map if the keys are in fact arbitrary. Expiring keys? Use something vaguely like this, but probably wrapping Map<K, [number, V]>, made simpler by not supporting non-expiring keys (which mixture was what caused my bug, and which mixture I was never fond of).

jitl 4y ago

Yeah that’s true. But because it’s using the system hashmap it inherits those performance characteristics and maximum size. Map is hard limited to 2^24 = 16,777,216 keys in v8. Map can also have performance spikes that cause event loop delay when you start loading it heavily. If you expect higher load, using a two levels of map (eg Map<k1, Map<k2, V>>) can help with both issues. I went to look how the library accounted for these issues and it does not. I didn’t look to see if it uses timer coalescing or intervals, I wouldn’t want O(millions) of setTimout calls on my service either. I think it’s fine to use in the browser or a hobby server, but due to the limitations of Map I would not use it in my production server. Redis can do 2^32 = 4,294,967,296 keys (a few more), and you can do the “multi level map” by sharding your key space across multiple Redis or Memcached processes. Redis et al also has a large advantage of the cache surviving application deploy. Again, not everyone needs this but to me the main benefit of a remote cache is consistently lowering latencies, versus a in-memory cache where your latency will spike after every deploy as the cache refills.

chrismorgan 4y ago

The expiring part is very simple. For just get/set/delete and scheduled garbage collection, here’s an implementation that’s almost precisely equivalent to what KV.js offers (just with sane arguments and return types, fewer bugs, and markedly better performance due to not supporting stuff you certainly don’t need and removing unnecessary abstractions): const store = new Map(); const expiryTimes = new Map(); function get(key) { // (This works even for non-expiring keys because undefined is not less than any number.) if (expiryTimes.get(key) < Date.now()) { store.delete(key); } return store.get(key); } function set(key, value, expiry) { store.set(key, value); if (expiry) { expiryTimes.set(expiry); } } function del(key) { store.delete(key); expiryTimes.delete(key); } function flushExpired() { const now = Date.now(); for (const [key, time] of expiryTimes) { if (time < now) { del(key); } } } // Optional: automatically delete expired entries to free memory, once a second since that’s what KV.js does, but that’s probably waaaaay more often than you need. setInterval(flushExpired, 1000); Working directly with store and expiryTimes for more advanced stuff will genuinely regularly be easier than working with the KV.js abstraction.

Etheryte 4y ago
This comment is a good example of why "oh this is easy, I can write this myself" should not be the default approach in some cases. There's a severe bug in your set method, expiry times are not cleared when overwriting an expiring value with a value with no expiry. The library linked as the main thread does not have this bug.
- chrismorgan 4y ago
  True, that bug exists in what I wrote. Insert an `else { expiryTimes.delete(expiry); }` at the end of set to fix it. However, I note that KV.js has similarly severe bugs (some noted in my other comment in this thread—methods that use expired values, get() mangling falsy values) and its greater complexity makes it more likely to have bugs. Very often I agree with you about the dangers of writing it yourself. But very often I also sufficiently dislike how libraries are implemented, with unnecessary bloat like excessive flexibility (all the options and such) or explicit runtime type checking rather than just letting things go wonky or blow up if given bad data. In reality, in a case like this I would actually recommend something other than a generic container. Non-expiring keys? Use variables if the keys are a finite set of known values, or a Map if the keys are in fact arbitrary. Expiring keys? Use something vaguely like this, but probably wrapping Map<K, [number, V]>, made simpler by not supporting non-expiring keys (which mixture was what caused my bug, and which mixture I was never fond of).