isaacs / isaacs/node-lru-cache

possible perf improvements around fetch() and BackgroundFetch objects

Open
#287 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
JavaScript
Stars
5.9k
Forks
374
PR merge metrics
No merged PRs in 30d

Description

Right now, the #valList can contain both value types, and BackgroundFetch promises to value types.

This was a cute way to implement things initially, but as the feature set around fetch() has increased, it's meant a lot of very careful applications of isBackgroundFetch() pretty much everywhere values are handled (which, being a cache, is kind of a lot of places.) Leading to bugs like #285, and also plenty of other cases where those objects have leaked in the past.

Also, it screws up what should be a very fast operation, because instead of having an array filled entirely with one type plus undefined, it's got this arbitrarily (if consistently) decorated Promise object along with the value, and v8 usually doesn't smile upon such mixing of types.

Most of the time, what we care about is the __staleWhileFetching value on the BackgroundFetch, which is sort of it's "effective value" while it awaits resolution in the #valList. Here's an approach that would probably make things a bit faster, and definitely a whole lot cleaner:

  • Add these internal fields:
    • #fetchList - array of plain old Promise objects and undefined, side by side with #valList, so this.#fetchList[index] will replace this.#valList[index] once it resolves.
    • #abortController - array of AbortControllers, where #abortController[index] corresponds to #fetchList[index]
    • #returnedFetch - Uint8Array (if max limited, or number[] otherwise) containing 1 if the fetch has been returned, or 0 otherwise. The nice thing here is that we only have to set the number when creating the promise, and potentially when returning it, and only ever have to read it when/if it blows up, so it should be a bit faster than keeping around a circular reference or null on the object itself.
  • All the places that we would use __staleWhileFetching, just use #valList[index] instead.
  • Remove all instances of this.#isBackgroundFetch, since methods that don't concern themselves with fetch() (ie, pretty much everything except fetch() itself) can just be oblivious and act like they're a cache that doesn't do fetching.
  • Because the fetchMethod is read-only and can only be set in the constructor, we don't have to bother setting them up unless a fetchMethod is provided.
  • We do still need to ensure when evicting/deleting/updating an item that if there's a fetch in progress, it should be canceled, but that's just a matter of doing this.#abortController?.[index]?.abort(byeBye) any time we write to #valList, vs a #isBackgroundFetch method and then v.__abortController.abort().

The real memory footprint cost is going to be in the 2 added arrays, but I don't think it'll be that bad, tbh. They'll stay mostly empty most of the time, after all, so the net impact is just 2 array references per cache, and 2 undefined values + 1 uint byte per possible entry.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start at the cache's fetch() entry point and trace the existing BackgroundFetch, #valList, and #isBackgroundFetch() handling. Done means fetch state is separated from cached values, in-progress fetches are canceled when entries change, and the cache no longer needs BackgroundFetch checks outside fetch().

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, node.js
Domain
backend
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.