-
-
Notifications
You must be signed in to change notification settings - Fork 364
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 andundefined, side by side with#valList, sothis.#fetchList[index]will replacethis.#valList[index]once it resolves.#abortController- array of AbortControllers, where#abortController[index]corresponds to#fetchList[index]#returnedFetch- Uint8Array (ifmaxlimited, ornumber[]otherwise) containing1if the fetch has been returned, or0otherwise. 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 ornullon 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
fetchMethodis read-only and can only be set in the constructor, we don't have to bother setting them up unless afetchMethodis 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#isBackgroundFetchmethod and thenv.__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.