groupon / node-cached

A simple caching library for node.js, inspired by the Play cache API
BSD 3-Clause "New" or "Revised" License
94 stars 15 forks source link

Fetching stale values can be slow #33

Open fdegiuli opened 8 years ago

fdegiuli commented 8 years ago

When a stale value is fetched, because of the way promise code is scheduled (Promise.then just sticks the function on the even loop), the val() function passed to getOrElse() is executed before returning the stale value. If val() is computationally expensive, or if many stale values are queried a the same time, it can significantly delay the response.

Just stupidly delaying the content refresh solves this problem (as long as you don't make another request 500ms later):

refreshed = Bluebird.delay(300)
      .then(() => util.toPromise(val))
      .then(writeValue) // etc...

This is obviously not the correct way to do it, but it does demonstrate that this is where the problem is.

Having some kind of scheduler that delays the content refresh logic until all the cache calls have resolved seems like the way to go. (Naturally it would have to avoid starvation.) Let me know what you think, I'd be happy to look into it.

jkrems commented 8 years ago

It seems like if the computation is expensive enough to cause the event loop to stall for a non-trivial amount of time, that would cause problems anyhow. "Best case" once the app is in production and exposed to a certain level of concurrency..?

fdegiuli commented 8 years ago

Yeah, so for my case each refresh is just a single network call, but each call to the server fetches a bunch of values from the cache. (Between 20-200.) I'm using the memory backend, so it's not a huge deal, but the requests seemed to be clobbering me.

I profiled my code, and it spends most of its time in the node innards. I think issue really is that a value has to bubble its way past a bunch of thens which each get scheduled at the end of the event loop, instead of just calling its way out synchronously with callbacks. I wrote a dumb little benchmark to test this:

var i = 0;
var ptimes = new Array(5000);
function ptest() { ptimes[i] = process.hrtime(); Promise.resolve(i).then((ii) => {ptimes[ii] = process.hrtime(ptimes[ii])[1]/1000000;})}

// u = lodash
for ( i = 0; i < 50; i++) ptest()
u.mean(ptimes)
// 0.384245

for ( i = 0; i < 500; i++) ptest();
u.mean(ptimes)
// 1.9938009480000003

So the more promises are scheduled at the same time, the longer they take, since they just stack on top of the event loop. Since util.toPromise is just Promise.resolve, it sticks more promises on the stack and slows down all the other calls.

I don't expect you guys to fix this, since it obviously isn't your use case. Anyway it might not be possible without rewriting most of the cache, now that I know what the problem is. At this point I just hope you found this interesting. 😄

You're totally right though, it does work like a dream when it's under serious load. Now all I need is the users.