medikoo / memoizee

Complete memoize/cache solution for JavaScript
ISC License
1.73k stars 61 forks source link

Improve cache logic for prefetch #51

Open puzrin opened 8 years ago

puzrin commented 8 years ago

Current prefetch implementation drops cache value immediately after start. That's not correct behaviour for big loads. Cache should be dropped only if it's timed out, and should not be affected by internal failures.

Summary:

  1. If prefetch "in progress", memoizee should return old cache value.
  2. When prefetch complete:
    • on successs - update cache value
    • on fail - do nothing.

Potential problem:

If prefetch fails, it will be called again on next memoizee call (no throttling). That can be considered as acceptable behaviour, because:

medikoo commented 8 years ago

Current prefetch implementation drops cache value immediately after start.

I don't think it works that way. To me prefetch works exactly you want it to work. Can you prepare a simple test case which shows it's otherwise (?)

puzrin commented 8 years ago

Probably me & @Kirill89 did not understood right what happens. Could you explain this 2 lines https://github.com/medikoo/memoizee/blob/v0.3.9/ext/max-age.js#L49-L50 ?

It looks like when prefetch starts, it immedeately drops cache data with conf.delete(...).

medikoo commented 8 years ago

This is called after value being accessed, and !preFetchTimeouts[id] means that prefetchAge time has passed and value should be prefetched for next access.

To prefetch value we call again memoized function, however this can't work properly without previously deleting cached value (if we won't delete it, memoized function will returned cache value).


It's indeed as I look now, not ideal, as when we ask for a result when new value is being retrieved, then instead of returning old value, result will be postponed until refreshed value arrives.

So it's as you say, sorry first I misunderstood I thought you say in general values are dropped right after being memoized.

Right fix for that would be, to not call memoized function but underlying function, and replace result after having it. This unfortunately is not that straightforward with various extensions (like e.g. resolvers), or different method of obtaining results (e.g. async, promise), Still should be addressed.

ilaif commented 7 years ago

Hey Medikoo, Thanks for this awesome work. I'm using your package successfully and it has great performance.

I encountered the need for the enhancement stated here. Is there a roadmap for it, can I contribute in some way?

My use-case: I have dozens of requests per second. Each of them passes through authentication against a MySQL table. Since I have 1 db instance, then sometimes, during big loads I get long query times (> 10 seconds) for these trivial queries (Usually under 50 ms). Still, I don't want the client side to be affected, in such a case, I would want to define a "timeout" for fetching the data, that when expires, will return the old cache value and will continue fetching in the background, updating the value for subsequent requests.

medikoo commented 7 years ago

@ilaif it's unfortunately not easy to address with current internal design, which doesn't really allow to refresh the value without invaliding the cache upfront, and at same time make this operation not problematic and transparent to any eventual extensions to which internals try to remain agnostic.

I plan to refactor the internals so handling cache that way is possible and straightforward (should happen in Q1 of next year). otherwise, trying to implement some workaround on existing implementation, while probably possible, for sure is challenging and may rise few head-aches. Anyway I'm open for PR's :)