Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

possible perf improvements around fetch() and BackgroundFetch objects #287

Open
isaacs opened this issue Mar 17, 2023 · 0 comments
Open

Comments

@isaacs
Copy link
Owner

isaacs commented Mar 17, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant