You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
The text was updated successfully, but these errors were encountered:
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:#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 (ifmax
limited, ornumber[]
otherwise) containing1
if the fetch has been returned, or0
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 ornull
on the object itself.__staleWhileFetching
, just use#valList[index]
instead.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.fetchMethod
is read-only and can only be set in the constructor, we don't have to bother setting them up unless afetchMethod
is provided.this.#abortController?.[index]?.abort(byeBye)
any time we write to#valList
, vs a#isBackgroundFetch
method 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.The text was updated successfully, but these errors were encountered: