3 ms·
Here is somewhat of a technical review. I have attempted to be objective, and also to provide some useful insights in the middle of it, as well as pointing out
by chrismorgan 4y ago
Here is somewhat of a technical review. I have attempted to be objective, and also to provide some useful insights in the middle of it, as well as pointing out a few bugs here and there.
Not to beat about the bush: I would strongly recommend against using (or designing) something like this unless you’re actively aiming for usage compatibility with Redis (I don’t know how actually compatible it might be; I don’t use Redis; as for whether aiming for that is worthwhile, my opinion is generally not). If you aren’t explicitly aiming for that, the code is significant bloat around very simple patterns and you would do much better to use those underlying patterns.
The original source code is 100KB, and 3023 lines long (including 1021 comment and 346 blank lines). Minified, it’s 24KB, and gzipped 5.3KB. Except it also depends on glob, so add on another 75KB minified/23KB gzipped for it and its transitive dependencies, for this functionality that you probably don’t use but which probably won’t be eliminated from your build.
The typical user of something like this should really just use a Map directly, with maybe a second Map and auxiliary function for expiring keys, if they need that. Much less code, much faster, much simpler. If you need any of the more exotic functionality, code it yourself, it’s simple enough; then you can also choose more meaningful names, too.
> fast
Well, I took a look at the first method after the constructor, set, and found stuff like this:
for (const opt of options) {
switch (opt.toUpperCase()) {
case 'NX':
nx = true;
break;
…
case 'EX':
ex = Number.parseInt(options[options.indexOf(opt) + 1], 10);
Ugh. Gratuitous and unnecessary case-insensitivity, use of an array instead of an object, unwise indexOf because you used a values iterator rather than retaining the index… I suppose quite a bit of this is bound up in a probably-misguided (in my opinion) mirroring of Redis’s API. But in general, this direction of design is bad for JavaScript.
But to go deeper:
The thing to realise about this set method is that despite being 83 lines long, the most common case of passing no options makes it precisely equivalent to `this.store.set(key, value);`—just slower and heavier.
As time goes by I become more and more strongly opposed to options objects because they harm performance, code size and complexity, unless mitigated at build time by partial evaluation, which is not something done in JavaScript-land (Facebook tried it a few years ago with Prepack, but quickly gave up, and I’m not sure quite why).
Many of these options are either unwise variation (EX/PX/EXAT/PXAT) or to make it do materially different things (GET—I note it makes a lie of the return type declared in the JSDoc comment); my typical recommendation would be splitting things out into different methods, because tooling can cut away unused methods (… though it mostly won’t actually do so—unused functions, sure, but tooling is more leery about culling unused methods), and it helps the code to run fast. I won’t give examples because the method is messy enough in its diversity that it’d be painful.
But even retaining the current behaviour, there is one technique that I would expect to significantly improve throughput: add a little more code, a fast path for the no-options case.
set(key, value, options) {
if (!options) {
// Fast path.
this.store.set(key, value);
} else {
// Slow path.
… the 81 lines of current method body …
}
}
—⁂—
get’s few instructions show just how simplistic and imitable the expiry feature is:
get(key) {
const isExpired = this._checkAndRemoveExpiredKey(key);
if (isExpired) {
return null;
}
return this.store.get(key) || null;
}
Pretty sure that return statement is buggy, `|| null` will mangle falsy values (e.g. false, 0, "") to null.
—⁂—
Skimming through much of the rest, there are quite a few weird and/or inefficient and/or wrong things. Examples: incrby() uses expired values (looks like that might happen fairly commonly); incrby() happily mangles types; decr() has a superfluous try-catch-rethrow; decrby(key, decrement) isn’t just incrby(key, -decrement); keys() uses a .entries() iterator for no reason; also produces an array rather than an iterator (mildly more subjective); mset()… eww array rather than object; various list methods: wow these are verbose and often very bad fit for JavaScript, but I’ll limit myself to just remarking that in `list === undefined || !Array.isArray(list)` the `list === undefined ||` is superfluous; zdiffstore: clearly not tested, this.ZDIFF is undefined; blocking list methods like brpop(): um, you know JavaScript is single-threaded, right? You’re busy-waiting until the timeout for something that cannot possibly change. (And no, there’s no SharedArrayBuffer or such here which could be multi-threaded, because it checks Array.isArray.)
—⁂—
I think a lot of this would be better as free-standing functions that operate on a simple Map. It seems to me that expiry is really the only reason you can’t obviously and completely do this. For that reason, the direction I’d suggest would instead be making a new class that exposes the Map interface with only the minimum changes required to support expiry (only working with timestamps in milliseconds, no relative values or seconds, both of which are just begging for trouble), and then have all the rest of the more exotic functionality sit on top of that, perhaps then just as standalone functions.
- CyberDildonics 4y agotl;dr ?
- vidalee 4y agoNot the author here, but thank you for taking the time to do this review.
- chrismorgan 4y agoYou’re welcome. It’s a trap I often fall into. I invariably end up spending longer on it than I intended to!
- chrismorgan 4y agoAmendment due to https://news.ycombinator.com/item?id=35619757 https://news.ycombinator.com/item?id=35619757: the fast path for set would need to also include `this.expireTimes.delete(key);`.