Change Request: revert the deprecation of no-return-await

[SukkaW]

ESLint version

Since v8.46.0

What problem do you want to solve?

ESLint deprecated the no-return-await in #17417 and claims that return 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 than return promise by a lot, see the benchmark here: https://replit.com/@isukkaw/return-await-is-slow

What do you think is the correct solution?

ESLint should revert the deprecation of no-return-await and wait for v8 to actually make await faster.

Participation

• I am willing to submit a pull request for this change.

Additional comments

cc @clshortfuse @mdjermanovic

[clshortfuse]

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.

[SukkaW]

There's a problem with microbenchmarking like this because you're awaiting a fully resolved promise which doesn't actually need the microtask.

Is this written in the ECMAScript spec or is V8/JavaScriptCore/SpiderMonkey implemented in this way?

Basically you're short-circuiting the "does this function need a microtasks at all" check.

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 awaits, it needs to wait at least a tick (microtask) before moving on, whether the value might or might not be a promise-like object, whether has been resolved or not.

That's to say, even await 1 still needs to wait for a tick, there is no so-called shortcut.

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.

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.

[clshortfuse]

I did write a rule to attempt to avoid useless await. Basically await <literal> is bad.

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.

[SukkaW]

For real word performance, it's based on V8 and I would suggest seeing this blog post:

https://v8.dev/blog/fast-async

The real-world performance data shown in the blog post only demonstrates how await is faster than before, it never says it is just as fast as without await.
Most of the real-world applications don't use return await, so those data can't justify if return await is as fast as return promise.

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.

IMHO, unless you are doing try...catch, you still SHOULD NEVER write return await.

No matter how engines/runtimes optimize, an extra await will always make the execution wait for at least an extra tick, there is no exception. It is written in the ES spec, and any engine that doesn't wait for the tick means it is not ES spec-compliant.

[clshortfuse]

No matter how engines/runtimes optimize, an extra await will always make the execution wait for at least an extra tick, there is no exception. It is written in the ES spec, and any engine that doesn't wait for the tick means it is not ES spec-compliant.

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.

#17345

[clshortfuse]

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.

[SukkaW]

Here's the code to prove otherwise:
[Redacted]
Unless the nature of Promise.resolve has changed, and on the latest Chrome stable it hasn't, then an await is one tick faster.

But your test case actually didn't prove your point. Your test case only demonstrates why we need ESLint require-await (https://eslint.org/docs/latest/rules/require-await) rule: await (async () => Promise.resolve(42))() costs more ticks than await (() => Promise.resolve(42))(), not because it doesn't return await, but because it has useless async!

And your test case also demonstrates that await (() => Promise.resolve(42))() costs one less tick than await (async () => await Promise.resolve(42))(), which exactly proves my point!

---

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.

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?

[clshortfuse]

And your test case also demonstrates that await (() => Promise.resolve(42))() costs one less tick than await (async () => await Promise.resolve(42))(), which exactly proves my point!

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:

async function test(name, fn) {
  let tick = 0;
  const tock = () => tick++;
  Promise.resolve().then(tock).then(tock).then(tock).then(tock).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 await await 42),
  test('promise-sync', () => Promise.resolve().then(() => Promise.resolve()).then(() => Promise.resolve(42))),
  test('promise-async', async () => Promise.resolve().then(() => Promise.resolve()).then(() => Promise.resolve(42))),
  test('promise-async-await', async () => await await await Promise.resolve(42)),
]);

setTimeout(() => {}, 100);

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:

https://perf.link/#eyJpZCI6Im82Ym1jbjVzaHM0IiwidGl0bGUiOiJSZXR1cm4gYXdhaXQgUHJvbWlzZSB2cyByZXR1cm4gUHJvbWlzZSIsImJlZm9yZSI6IiIsInRlc3RzIjpbeyJuYW1lIjoiVGVzdCBDYXNlIiwiY29kZSI6ImF3YWl0IChhc3luYyAoKSA9PiB7XG4gYXdhaXQgMDtcbiByZXR1cm4gUHJvbWlzZS5yZXNvbHZlKDQyKTtcbn0pKClcbmdsb2JhbFRoaXM7IC8vIERvIHNvbWV0aGluZyBhZnRlciIsInJ1bnMiOls4OTAwMCwyNzYwMDAsODIwMDAsNzkyMDAwLDEyMDAwLDc4MzAwMCwyMTgwMDAsNzAwMCwxMzQwMDAsMTQ3MDAwLDg3MDAwLDE4MjAwMCw4OTAwMCw4NDkwMDAsNjAwMCw2MTAwMCwxMDIwMDAsMjQ1MDAwLDIzOTAwMCw1MjAwMCwxNDUwMDAsMTEwMDAsODg2MDAwLDc5MDAwLDE0NTAwMCwyMjAwMDAsMTA1OTAwMCwyMTEwMDAsNzc5MDAwLDIwMTAwMCw3NDAwMCw3ODYwMDAsNzYzMDAwLDQ3MDAwLDEwNzAwMCw3OTEwMDAsNDM2MDAwLDE4NzAwMCwxNjAwMCwxOTIwMDAsMzUwMDAsMTEwMDAsNTAwMDAsMzUxMDAwLDIwNDAwMCw4NzAwMCwyMTIwMDAsMTM1MDAwLDExMTIwMDAsNzAwMDAsMzczMDAwLDk2MDAwMCw1NjgwMDAsNDAwMCwxNzAwMDAsNzUwMDAsMTMwMDAsMTEwMDAsNDEwMDAsMTE2MDAwLDE1MTAwMCw5MDAwLDkxMDAwLDQwMDAsNjcwMDAsMjc3MDAwLDQwODAwMCwzNDAwMCw5MDAwLDQ5MzAwMCwyODAwMCwyNzAwMCwyOTcwMDAsOTAwMCw0ODAwMCwyNTgwMDAsNjc3MDAwLDE4NjAwMCw0NjYwMDAsMTIwMDAsOTUwMDAsNzAwMDAsMjI5MDAwLDEwMjYwMDAsNjU5MDAwLDM0NTAwMCw5NjEwMDAsNzQ3MDAwLDI4NjAwMCw4MzEwMDAsMTAwMCwxNDMwMDAsMjA5MDAwLDU3MDAwMCwxNzIwMDAsMjMwMDAsNDU5MDAwLDExMDAwLDI5NzAwMCw4OTIwMDBdLCJvcHMiOjI3NzYyMH0seyJuYW1lIjoiVGVzdCBDYXNlIiwiY29kZSI6ImF3YWl0IChhc3luYyAoKSA9PiB7XG4gYXdhaXQgMDtcbiByZXR1cm4gYXdhaXQgUHJvbWlzZS5yZXNvbHZlKDQyKTtcbn0pKClcbmdsb2JhbFRoaXM7IC8vIERvIHNvbWV0aGluZyBhZnRlciIsInJ1bnMiOlszMzcwMDAsNDI5MDAwLDgwMDAwLDEwMTAwMCwxNjQwMDAsMzg0MDAwLDEwMDAsNzM4MDAwLDY1MjAwMCwxOTIwMDAsMzcwMDAsNDgwMDAsODQ3MDAwLDk4MDAwLDEwMDAsMzk0MDAwLDEyMDAwMCwxMTMwMDAsNDc1MDAwLDY3MTAwMCwzODMwMDAsNzE0MDAwLDg0NzAwMCw3NDAwMCwxMzEwMDAsMzAwMCwyNDAwMDAsNjEwMDAsMzAwMCw3MDEwMDAsMTQwMDAwLDEwOTAwMCwyMzEwMDAsNzI3MDAwLDc3NzAwMCwxMzIwMDAsODgwMDAsNTg3MDAwLDE1MDAwLDEzNzAwMCw2NTQwMDAsNjgzMDAwLDkwMDAsMzAxMDAwLDIwNjAwMCwxNjAwMCwxOTYwMDAsMzgwMDAsMjMwMDAsNTA4MDAwLDQyNzAwMCwzMzAwMCwxMTIwMDAsMTIxMjAwMCw0MzYwMDAsNzcwMDAsOTI2MDAwLDEyMDAwLDcxMDAwMCwyMTAwMDAsMTEwMDAwLDMwMzAwMCw4MjAwMCwzMjIwMDAsOTMwMDAsMzAxMDAwLDExMDAwLDMwMjAwMCwzMDAwLDQxMDAwMCwxMTQwMDAsMjA3MDAwLDQ1MDAwMCwyMDMwMDAsMjI4MDAwLDcwMjAwMCwxNDcwMDAsMzAwMCw3NDcwMDAsNzAwMDAsMTkyMDAwLDU1MjAwMCwxNzYwMDAsMTYxMDAwLDMzNTAwMCwzNjAwMCw3MDkwMDAsMTMyMjAwMCwyNzIwMDAsODA0MDAwLDc4NjAwMCwzMDAwLDM1OTAwMCw4NjAwMCw4NjAwMCw1NTAwMDAsNDAwMDAwLDU0MjAwMCw2MDEwMDAsODA0MDAwXSwib3BzIjozMjM1NTB9XSwidXBkYXRlZCI6IjIwMjMtMDktMjlUMTI6MDI6NTUuNzMzWiJ9

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."

[clshortfuse]

To show you I'm not pulling your leg on this, here's two back to back benchmarks

And believe me, I'd wager I'm probably more performance obsessed than you. If there were something faster, I'd jump on it.

[nzakas]

Thanks for the report @SukkaW and for chiming in @clshortfuse. The no-return-await rule was added early on when async functions were first released and was based largely on how engines were implementing it at that time. Since then, there have been a lot of changes, performance and otherwise, so we don't feel like it makes sense to warn people against using return await.

As a reminder, deprecating a rule doesn't mean it's removed. You can still use it if you'd like.