query
query copied to clipboard
Performance issue in @tanstack/query-async-storage-persister (and possibly #updateStaleTimeout)
Describe the bug
It looks like delayFunc in the packages/query-async-storage-persister/src/asyncThrottle.ts
has some performance problem.
const delayFunc = async () => {
clearTimeout(timeout)
timeout = setTimeout(() => {
if (running) {
delayFunc() // Will come here when 'func' execution time is greater than the interval.
} else {
execFunc()
}
}, interval)
}
this is the 4x slow-down profile, each refetch
call causes persistQueryClientSave
to be called, and then delayFunc
is the slowest function in the tree. All small yellow bars are setTimeout
or clearTimeout
calls.
My throttle timeout is the default (1e3), therefore it cannot be caused by execFunc
(all those delayFunc
calls happen within 1 second).
I try to patch my node_modules and replace the asyncThrottle
with this:
function asyncThrottle2(func, { interval = 1e3, onError = noop } = {}) {
if (typeof func !== 'function') throw new Error('argument is not function.')
let currentArgs = null
setInterval(() => {
if (!currentArgs) return
Promise.resolve(func(...currentArgs)).catch(onError)
currentArgs = null
}, interval)
return (...args) => {
currentArgs = args
}
}
export { asyncThrottle2 as asyncThrottle }
This implementation is incorrect but fast.
After the patch, persistQueryClientSave
now only costs 1 ms instead of 57 ms.
I think it looks like calling setTimeout
or clearTimeout
is costly. I think a better async throttle implementation can avoid calling them so often.
btw I also observed that #updateStaleTimeout
method has a similar behavior.
also, this method behaves like delayFunc
, simple but costs a lot of time (cancel & create timer).
Your minimal, reproducible example
n/a
Steps to reproduce
Sorry I didn't provide one, but my patch works so I think my analysis of the performance problem is correct.
I'm happy to try any new asyncThrottle implementation (by patch node_modules, so maintainers don't have to release a new version for this) in our project to see if that fixes the performance problem. Thanks!
Expected behavior
No performance problem
How often does this bug happen?
Every time
Screenshots or Videos
No response
Platform
Chrome 119
@tanstack/query-async-storage-persister 5.8.7
Tanstack Query adapter
None
TanStack Query version
5.8.7
TypeScript version
No response
Additional context
No response
I'm happy to try any new asyncThrottle implementation
if you have a better implementation that is also correct, please do contribute 🙏. The asyncThrottle
function does have a decent test coverage.
Maybe we can also get inspired by other, already existing implementations?
- https://github.com/sindresorhus/p-throttle
@Jack-Works would you consider contributing your patch back to the persister plugin?
- https://github.com/DimensionDev/Maskbook/pull/11227/files
@Jack-Works would you consider contributing your patch back to the persister plugin?
* [DimensionDev/Maskbook#11227 (files)](https://github.com/DimensionDev/Maskbook/pull/11227/files)
Hi! I think our patch is obviously imperfect (it is over simplified and maybe even incorrect). I hope your team can find another throttle implementation and I'd like to try it.
Maybe we can also get inspired by other, already existing implementations?
[sindresorhus/p-throttle](https://github.com/sindresorhus/p-throttle)
@TkDodo Genuine question: why don't you use p-throttle? :) I'm just trying to understand what makes asyncThrottle
special.
Hello @jhnns,
I was also wondering why not just use p-throttle
, so I looked into the two implementations and they don't actually behave the same.
Take the following example:
const start = performance.now();
const fn = (a, b) => {
const time = Math.floor(performance.now() - start);
const result = a + b;
console.log({ time, result });
};
const throttledFn = asyncThrottle(fn, { interval: 500 });
throttledFn(1, 2); // prints: { time: 0, result: 3 }
throttledFn(1, 3); // ignored
throttledFn(1, 4); // ignored
throttledFn(1, 5); // ignored
throttledFn(1, 6); // prints: { time: 500, result: 7 }
So to my understanding, asyncThrottle
only keeps the last call waiting and ignores all the other calls.
While p-throttle
keeps all the calls waiting and executes them in order:
import pThrottle from "p-throttle";
const start = performance.now();
const fn = (a, b) => {
const time = Math.floor(performance.now() - start);
const result = a + b;
console.log({ time, result });
};
const throttle = pThrottle({
limit: 1,
interval: 500,
});
const throttledFn = throttle(fn, { interval: 500 });
throttledFn(1, 2); // prints: { time: 0, result: 3 }
throttledFn(1, 3); // prints: { time: 500, result: 4 }
throttledFn(1, 4); // prints: { time: 1000, result: 5 }
throttledFn(1, 5); // prints: { time: 1500, result: 6 }
throttledFn(1, 6); // prints: { time: 2000, result: 7 }
I didn't find a way to tell p-throttle
to only keep the last call, so I don't think it can be used as a drop-in replacement for asyncThrottle
.
@Jack-Works I made the PR https://github.com/TanStack/query/pull/7224 that suggests a new implementation. Can you please try it on your project and let us know if it fixes the issue?
fixed by:
- #7224