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

Improve cache logic for prefetch #51

Open
puzrin opened this issue Jan 25, 2016 · 5 comments
Open

Improve cache logic for prefetch #51

puzrin opened this issue Jan 25, 2016 · 5 comments
Assignees

Comments

@puzrin
Copy link

puzrin commented Jan 25, 2016

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:

  • it still suppress parallel calls
  • it does not increase latency
@medikoo medikoo added the bug label Jan 25, 2016
@medikoo
Copy link
Owner

medikoo commented Jan 25, 2016

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
Copy link
Author

puzrin commented Jan 25, 2016

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
Copy link
Owner

medikoo commented Jan 25, 2016

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.

@medikoo medikoo self-assigned this Jan 25, 2016
@medikoo medikoo added enhancement and removed bug labels Jan 25, 2016
@ilaif
Copy link

ilaif commented Dec 12, 2016

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
Copy link
Owner

medikoo commented Dec 13, 2016

@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 :)

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

No branches or pull requests

3 participants