1621234 - [devtools-rfc] Lint against await in loops (no-await-in-loop)

Status: NEW

(DevTools :: General, task, P3)

Description

[Julian Descottes [:jdescottes]]

Recently, we have improved performance in DevTools by parallelizing async work such as target creation. This was typically handled with the following pattern:

for (const item of someArray) {
  await item.callAsyncMethod();
}

When it is safe to run this in parallel, we can write this as follows:

const promises = someArray.map(async item => {
  await item.callAsyncMethod();
});
await Promise.all(promises);

We could start linting against such patterns using https://eslint.org/docs/rules/no-await-in-loop

Comment 1

[Patrick Brosset <:pbro>]

Does that mean that for the places where it is not safe to run things in parallel, we'd have to add an eslint comment to disable the rule temporarily? // eslint-disable-next-line

Comment 2

[Julian Descottes [:jdescottes]]

(In reply to Patrick Brosset <:pbro> from comment #1)

Does that mean that for the places where it is not safe to run things in parallel, we'd have to add an eslint comment to disable the rule temporarily? // eslint-disable-next-line

Yes and honestly I am not convinced this is the good call to make.
It might lead to unnecessary optimizations, code that is harder to read etc...

Before deciding to parallelize a loop, we need to carefully review 1/ if the functionality is preserved, and 2/ if the improvement is worth hurting the readability.

For now I just want to get an idea of the landscape in DevTools. We currently have 130 violations (full log at https://gist.github.com/juliandescottes/92faa85890ea46b48a5ba60a94070841 ). I sampled a few of them, and it's a mixed batch. In some cases I can't easily tell whether it's safe to parallelize, in other cases it looks like the async operation will only rarely happen so parallelizing will most likely be useless etc...

Try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=04fd79518fa9315d1f065e85cf513847c1fedc47

Comment 3

[Alexandre Poirot [:ochameau]]

I really appreciate the readability of for loops. But well, that's a suggestive argument.

I'm wondering if using Promise.all would:

• introduce races, between each concurrent tracks running in parallel. Well that's the price to pay to run in parallel.
• might introduce hangs by running more things all at once. I imagine this is a bad argument when you actually dispatch to multiple differents threads/process, like the TargetList case! But if the concurrent runs are all on the same thread/process, may be that's true?

While I immediately apply suggestions of using Promise.all instead of for loop in critical framework code, where it is clear that we are dispatching calls to parallel threads or processes, I'm not sure it is right to enforce rejecting for loop everywhere.

Comment 4

[Julian Descottes [:jdescottes]]

(In reply to Alexandre Poirot [:ochameau] from comment #3)

I really appreciate the readability of for loops. But well, that's a suggestive argument.

I'm wondering if using Promise.all would:

• introduce races, between each concurrent tracks running in parallel. Well that's the price to pay to run in parallel.
• might introduce hangs by running more things all at once. I imagine this is a bad argument when you actually dispatch to multiple differents threads/process, like the TargetList case! But if the concurrent runs are all on the same thread/process, may be that's true?

While I immediately apply suggestions of using Promise.all instead of for loop in critical framework code, where it is clear that we are dispatching calls to parallel threads or processes, I'm not sure it is right to enforce rejecting for loop everywhere.

I completely agree, I feel like this is a good thing to keep in mind for some critical code paths, but in general not something we should enforce.
I'll mention it one last time in devtools-central and if nothing new comes up, I will close this RFC. Thanks for sharing your input!
(ni myself to update this after next devtools-central)

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

I agree this should be decided on a case-by-case basis.
I don't know if we could have "hints" on phabricator when using await in a for...of loop, saying it could have a performance impact, but not blocking landing.

Comment 6

[Julian Descottes [:jdescottes]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #5)

I agree this should be decided on a case-by-case basis.
I don't know if we could have "hints" on phabricator when using await in a for...of loop, saying it could have a performance impact, but not blocking landing.

I didn't think about that! I guess we could have it as an eslint warning, but I'm typically not paying attention to warnings :)