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
Change Request: revert the deprecation of no-return-await
#17613
Comments
Return await uses less ticks (queued microtasks). There's a problem with microbenchmarking like this because you're awaiting a fully resolved promise which doesn't actually need the microtask. Basically you're short-circuiting the "does this function need a microtasks at all" check. That's not exactly indicative of real world performance since we have to gauge why the promise exists at all. And more often than not (my guess) there's an I/O. Still the rule was removed because it's not a clear cut answer and the original reasons for creating the rule no longer apply. If there was a way for esllnt to know that you're awaiting a promise that will already be resolved, then I guess it would make sense. |
Is this written in the ECMAScript spec or is V8/JavaScriptCore/SpiderMonkey implemented in this way?
Creating a new promise object in each cycle would cause more problems with micro benchmarking, hence sharing the same promise object and pre-resolving the promise before the cycle actually begins. Also, IIUC, ES spec has said that as long as it That's to say, even
IMHO the only reason you should return await is to constraint the current function stack (without it being leaked to the outer scope), which is only commonly seen in try-catch. I just can't come up with any other reason that you need to return await. |
I did write a rule to attempt to avoid useless await. Basically But whatever rule we make will always be incomplete, so it was better to deprecate. For real word performance, it's based on V8 and I would suggest seeing this blog post: https://v8.dev/blog/fast-async The deprecation of the rule doesn't translate that you MUST always use return await. It's just not longer as clear cut to say you should NEVER return await. We can draft up numerous examples where it's faster one way and faster in another, which emphasizes more that the rule should not exist as it did. |
The real-world performance data shown in the blog post only demonstrates how
IMHO, unless you are doing try...catch, you still SHOULD NEVER write return await. No matter how engines/runtimes optimize, an extra |
Here's the code to prove otherwise: async function test(name, fn) {
let tick = 0;
const tock = () => tick++;
Promise.resolve().then(tock).then(tock).then(tock);
const p = await fn();
console.assert(p === 42);
console.log(name, tick);
}
await Promise.all([
test('nonpromise-sync', () => 42),
test('nonpromise-async', async () => 42),
test('nonpromise-async-await', async () => await 42),
test('promise-sync', () => Promise.resolve(42)),
test('promise-async', async () => Promise.resolve(42)),
test('promise-async-await', async () => await Promise.resolve(42)),
]);
setTimeout(() => {}, 100); Unless the nature of Promise.resolve has changed, and on the latest Chrome stable it hasn't, then an await is one tick faster. I suggest you read the comments of the original issue for information on the decision. |
Also I lost my comment because I'm on mobile, but runtimes also take shortcuts that seem in counter of spec. For example each named function should be a constructor, but Chrome will only execute the constructor related stuff on demand, in other words, only when you try access it as a constructor. To spec, it fulfills, but it doesn't follow it 1:1 in the name of performance. I had a V8 dev educate me on this because I swore anonymous functions should be faster because there are no constructor steps. I would hope that a useless await is swallowed by the runtime (eg: no other microtask queues). And I hope that's what's happening in the sample your provided. It's smart for the runtime to do so. |
But your test case actually didn't prove your point. Your test case only demonstrates why we need ESLint And your test case also demonstrates that
So I've updated my benchmark to address your comment in #17613 (comment) . The benchmark now also tests against the newly created promise: https://replit.com/@isukkaw/return-await-is-slow Even with unresolved promises, return await is still consistently slower. Would you mind enlightening me on this? |
But it doesn't because that's a completely different reshaping of a function unless you're arguing to completely remove all async/await, which I assume you're not. One executed immediately. The other has paused execution. Given this example: async function getData(id) {
const cache = myCacheMap.get(id);
if (cache) return cache; // NonPromise returns immediately (1 tick)
// return fetch(id); // Bad: Promise returned in async (3 ticks)
return await fetch(id); // Good: Promise to NonPromise via await (2 ticks)
} You can't just strip any function of the async and target every since instance of a literal return and convert it to a Promise return. That basically calls for stripping the async syntax altogether. Also, why would you ignore the extra tick? It serves as an example that a Promises in async functions lag a bit in running the microtask. For example, given:
The calls to Promise.then balloons up to 7, whereas await goes to 4. That's because await will consume the chain the promises that are common in asynchronous coding. That was explained in the V8 blog. (This is to illustrate how ticks chain, not a benchmark.) The reality is ticks add up, and the V8 blog illustrates that reduction of ticks improves performance. It's a constant target for optimization because ticks are very important in the JS landscape. The reason we have this optimization is because it was filed by fastify's main dev who is performance obsessed and he says the performance different is transparent. fastify uses return await in their code and if there were more to eek out, I'm sure they would use something else. I'm on mobile for the moment, so repl.it is a bit rough to use. The benchmarks you have don't account for a function that never calls for await, which could bring in external optimizations (like a runtime detecting if there is no paused execution ever). I built one here: For me, on my phone, the results are back and forth. It essentially becomes negligible. I would then have to consider how that extra tick I know is happening could have an effect. According to the V8 blog, those ticks end up being measurable. In the end, it's all a bit inconclusive. We're basically measuring if ticks are worse than calling await at all, but it's so variable based on your codebase that we can't really clearly say "Do this always." |
Thanks for the report @SukkaW and for chiming in @clshortfuse. The As a reminder, deprecating a rule doesn't mean it's removed. You can still use it if you'd like. |
See https://eslint.org/docs/rules/no-return-await: > Since the return value of an async function is always wrapped in > `Promise.resolve, `return await` doesn’t actually do anything except > add extra time before the overarching `Promise` resolves or rejects. BREAKING CHANGE: "return await" is no longer allowed, just return the promise without awaiting its resolution.
ESLint version
Since v8.46.0
What problem do you want to solve?
ESLint deprecated the
no-return-await
in #17417 and claims thatreturn await
has no performance difference with return a promise.It is not.
For v8 10.2 (which is currently shipped with the latest version of Node.js),
return await promise
is still consist slower thanreturn promise
by a lot, see the benchmark here: https://replit.com/@isukkaw/return-await-is-slowWhat do you think is the correct solution?
ESLint should revert the deprecation of
no-return-await
and wait for v8 to actually makeawait
faster.Participation
Additional comments
cc @clshortfuse @mdjermanovic
The text was updated successfully, but these errors were encountered: