Skip to content
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

feat: Lint files in parallel if many files exist #42

Closed
wants to merge 16 commits into from
305 changes: 305 additions & 0 deletions designs/2019-worker-threads/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,305 @@
- Start Date: 2019-09-29
- RFC PR: https://github.com/eslint/rfcs/pull/42
- Authors: Toru Nagashima ([@mysticatea](https://github.com/mysticatea))

# Lint files in parallel if many files exist

## Summary

This RFC adds the supports for parallel linting by `worker_threads`/`child_process` modules.

## Motivation

Linting logic uses CPU heavily, so parallel linting by threads may reduce spent time.

## Detailed Design

**PoC**: [eslint/eslint#very-rough-worker-threads-poc](https://github.com/eslint/eslint/tree/very-rough-worker-threads-poc/lib/eslint)

This RFC has two steps.

**The step 1 (semver-minor):**

- Adds `--concurrency` CLI option to specify the number of worker threads. Defaults to `1`.
- Adds `concurrency` constructor option to `ESLint` class to specify the number of worker threads. Defaults to `1`.
- Adds `disallowWorkerThreads` option to plugins. Defaults to `false`.
- Changes the behavior of `ESLint#executeOnFiles(patterns)` method.

**The step 2 (semver-major):**

- Changes the default of the `--concurrency` CLI option to `auto`.
mysticatea marked this conversation as resolved.
Show resolved Hide resolved
- Changes the default of the `concurrency` constructor option to `"auto"`.

### [STEP 1] New `--concurrency` CLI option

The `--concurrency` has an argument. The argument should be a positive integer or `auto`. Defaults to `1`.

- If `1` is present, ESLint does linting in the main thread.
- If another integer is present, ESLint creates this number of worker threads then the worker threads do linting.

If `auto` is present, ESLint estimates the best value from the number of target files. It's the number of target files divided by the constant [128](#constants), but `os.cpus().length` at most.
mysticatea marked this conversation as resolved.
Show resolved Hide resolved

```js
concurrency = Math.min(os.cpus().length, Math.ceil(targetFiles.length / 128))
Copy link

@Gaafar Gaafar Nov 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

os.cpus().length returns virtual cpu count. In my experience with the PoC eslint/eslint#12191 it was more performant to use 1 worker per physical core, anything more was causing more overhead than improvement. The node module physical-cpu-count seemed to work fine to get the physical cpu count as there is no native way to get that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In your case, how many files did each worker lint? If a worker lints a few files, it will be slow as expected. This proposal calculates the proper number of workers by the number of target files.

```

This means that ESLint does linting in the main thread if the number of target files is less than [128](#constants) in order to avoid the overhead of multithreading. But ESLint does linting with using worker threads automatically if target files are many.

If `--concurrency` option is present along with the following options, ESLint throws a fatal error.

- `--stdin`
- `--stdin-filename`
- `--init`
- `-h`, `--help`
- `-v`, `--version`
- `--print-config`
- `--env-info`

### [STEP 1] New `concurrency` constructor option of `ESLint` class

`ESLint` class is the new API that is discussed in [RFC 40](https://github.com/eslint/rfcs/pull/40).

The `concurrency` option corresponds to `--concurrency` CLI option. Defaults to `1`. If `"auto"` is present, `ESLint` class estimates the best value with the way the previous section described.

This RFC doesn't change `CLIEngine` class because this requires asynchronous API to expose.

### [STEP 1] New `disallowWorkerThreads` top-level property of plugins

This RFC adds `disallowWorkerThreads` top-level property to plugins.

If a loaded plugin has this property with `true`,

- When the `concurrency` option was `"auto"`, ESLint enforces the estimated value to `1`.
- When the `concurrency` option was `2` or larger, ESLint throws a fatal error.
mysticatea marked this conversation as resolved.
Show resolved Hide resolved

Therefore, plugins that don't work fine in workers can tell ESLint to disallow workers.

### [STEP 1] New behavior of `ESLint#executeOnFiles(patterns)` method

The overview is:

![Overview](diagrams/overview.svg)

1. Master enumerates the lint target files.
1. Master checks if the files exist in the lint result cache and it extracts only the files which are not cached.
1. Master estimates the proper `concurrency` from the number of the extracted files if `"auto"` is present.
1. Lint the extracted files:
- If `concurrency` is `1`, master [verifies files][verify files] directly. It doesn't create any worker.
- Otherwise, master creates workers then the workers [verify files]. The number of the workers is `concurrency`.
1. Master sends the initial data to each worker. The initial data contains the constructor options of `CLIEngine` and several files.
1. Workers [verify files]. It reports the lint result to master, and master may send the next file to the worker. The worker verifies the received file.
1. Workers exit with `0` after it reported the result of all files.
1. Master collects the lint result from workers then write it to the lint result cache.
1. Master returns the merged result from cache and workers.

#### Worker's implementation

Worker can be `require("worker_threads").Worker` or `require("child_process").ChildProcess`.
mysticatea marked this conversation as resolved.
Show resolved Hide resolved

If `v12.11.0` or newer version of Node.js runs ESLint, it's `require("worker_threads").Worker`. Otherwise it's `require("child_process").ChildProcess`.

#### About `--cache` option

We can use parallel linting along with `--cache` option because worker threads don't touch the cache. Master gives worker threads only files which are not cached (or cache was obsolete) and updates the cache with the collected result.

#### About `--fix` option

We can use parallel linting along with `--fix` option. ESLint modifies files after all linting completed.

#### About `--debug` option

We can use parallel linting along with `--debug` option, but ESLint cannot guarantee the order of the debug log. The order of the logs that are received from worker threads is messed.

#### About Error Handling

If exceptions happen in worker threads, the worker transfers the error information to the main thread if possible. The transferred information contains the properties of `message`, `stack`, `messageTemplate`, and `messageData`.

Therefore, people can see the error message as-is even if it happened in worker threads.
For example:

```
$ eslint lib
Error: FAILED!!!
Occurred while linting C:\Users\t-nagashima.AD\dev\eslint\lib\rules\no-mixed-operators.js:229
at BinaryExpression (C:\Users\t-nagashima.AD\dev\eslint\lib\rules\eqeqeq.js:152:27)
at C:\Users\t-nagashima.AD\dev\eslint\lib\linter\safe-emitter.js:45:58
at Array.forEach (<anonymous>)
at Object.emit (C:\Users\t-nagashima.AD\dev\eslint\lib\linter\safe-emitter.js:45:38)
at NodeEventGenerator.applySelector (C:\Users\t-nagashima.AD\dev\eslint\lib\linter\node-event-generator.js:253:26)
at NodeEventGenerator.applySelectors (C:\Users\t-nagashima.AD\dev\eslint\lib\linter\node-event-generator.js:282:22)
at NodeEventGenerator.enterNode (C:\Users\t-nagashima.AD\dev\eslint\lib\linter\node-event-generator.js:296:14)
at CodePathAnalyzer.enterNode (C:\Users\t-nagashima.AD\dev\eslint\lib\linter\code-path-analysis\code-path-analyzer.js:646:23)
at C:\Users\t-nagashima.AD\dev\eslint\lib\linter\linter.js:936:32
at Array.forEach (<anonymous>)
```

As a side note, master terminates the other workers if a worker reported an error.

#### Locally Concurrency

Now ESLint does linting in parallel by thread workers if target files are many.

Apart from that, each worker does linting concurrently by non-blocking IO to use CPU resource efficiently. This is a simple read-lint-report loop with `Promise.all()`:

```js
// Wait for all loops finish.
await Promise.all(
// Run the loop concurrently.
initialFiles.map(async initialFilePath => {
let filePath = initialFilePath

do {
// Read file content by non-blocking IO.
const text = await readFile(filePath, "utf8")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the PoC eslint/eslint#12191
it seemed that readFileSync was much faster than the promisified version (using utils.promisify), but I only tested it on my local machine (MacOs). So it might be worth benchmarking.

Copy link
Sponsor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the comparison would be between readFile promisfied and not, not readFileSync.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This proposal parallelize linting by non-blocking IO even if it doesn't use workers. Would you provide the comparison of linting in series with readFileSync and linting in parallel by readFile? If the former is faster, maybe we should update this proposal.

// Lint the file.
const result = verifyText(engine, filePath, text)
// Send the lint result and get the next file path. This is also non-blocking async op.
filePath = await pushResultAndGetNextFile(result)
} while (filePath)
}),
)
```

This logic is used in two places:

1. Each worker use it.
1. Master uses it in the main thread if target files are not many.

Therefore, if `concurrency` is `1` then:

```
Main thread
├ 🔁 Read-lint-report loop
├ 🔁 Read-lint-report loop
├ ...
└ 🔁 Read-lint-report loop
```

And if `concurrency` is greater than `1` then:

```
Main thread
├ ⏩ Worker thread
│ ├ 🔁 Read-lint-report loop
│ ├ ...
│ └ 🔁 Read-lint-report loop
├ ⏩ Worker thread
│ ├ 🔁 Read-lint-report loop
│ ├ ...
│ └ 🔁 Read-lint-report loop
├ ...
└ ⏩ Worker thread
├ 🔁 Read-lint-report loop
├ ...
└ 🔁 Read-lint-report loop
```

#### Loading Config

On the other hand, this RFC doesn't change the logic that loads config files. It's still synchronous. Because the logic is large and the configs are cached in most cases.

We can improve config loading by async in the future, but it's out of the scope of this RFC.

#### Constants

This RFC contains two constants.

- `128` ... the denominator to estimate the proper `concurrency` option value.
- `16` ... concurrency for non-blocking IO.
mysticatea marked this conversation as resolved.
Show resolved Hide resolved

The current values are improvised values. We may find more proper values by measuring performance.

#### Performance

Please try the PoC.

```
npm install github:eslint/eslint#very-rough-worker-threads-poc
```

This section is an instance of performance measurement. The worker threads make ESLint over 2x faster if there are many files.

- Processor: Intel Core i7-8700 CPU (12 processors)
- RAM: 16.0 GB
- OS: Windows 10 Pro 64bit
- Node.js: 12.11.0

```
$ eslint lib tests/lib --concurrency 1
Number of files: 697
Lint files in the main thread.
Linting complete in: 19636ms

$ eslint lib tests/lib --concurrency auto
Number of files: 697
Lint files in 6 worker threads.
Linting complete in: 7486ms

$ eslint lib --concurrency 1
Number of files: 368
Lint files in the main thread.
Linting complete in: 8442ms

$ eslint lib --concurrency auto
Number of files: 368
Lint files in 3 worker threads.
Linting complete in: 4672ms
```

If forced to use workers with a few files, it's slower.

```
$ eslint lib/cli-engine --concurrency auto
Number of files: 28
Lint files in the main thread.
Linting complete in: 1521ms

$ eslint lib/cli-engine --concurrency 2
Number of files: 28
Lint files in 2 worker threads.
Linting complete in: 2054ms
```

### [STEP 2] Changing the default of `concurrency` option to `"auto"`

This change needs a major release.

ESLint gets using worker threads by default if target files are many.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the fact that in some cases the linting will happen in the main thread and in other in worker threads create a scenario where some plugins work well in just one of the two cases?

To mitigate that, would it maybe be preferable to always use workers in Step 2, even when just a single worker is required?

Copy link
Member Author

@mysticatea mysticatea Oct 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because worker_threads got stable recently, this RFC uses child_process if Node.js is older. Spawning child processes spent time (about 1 sec) on Windows. Anyway, ESLint has to find config files, parse it, and verify it in all of every worker and the main thread, and the cost is not small.

Therefore, I don't think good if ESLint always uses workers.


## Documentation

- The "[Command Line Interface](https://eslint.org/docs/user-guide/command-line-interface)" page should describe new `--concurrency` option.
- The "[Node.js API](https://eslint.org/docs/developer-guide/nodejs-api)" page should describe new `concurrency` option.
- When step 2, it needs an entry of the migration guide because of a breaking change.

## Drawbacks

This feature introduces complexity for parallelism. It will be an additional cost to maintain our codebase.

This feature might not fit `eslint-plugin-import` and `@typescript-eslint`. Those have to parse the entire codebase to check types, but workers cannot share the parsed information. Every worker has to parse the entire codebase individually. But linting in parallel is faster than single-thread in spite of the cost of duplicate works.
mysticatea marked this conversation as resolved.
Show resolved Hide resolved

## Backwards Compatibility Analysis

Step 1 is not a breaking change. It just adds a new option.

Step 2 is a breaking change technically.

- If a parser/plugin shares state for multiple files and the state can changes the parser/plugin behavior, the workflow is broken. Because every worker thread cannot share state each other.
- If a parser/plugin consumes OS resources exclusively (e.g., bind a TCP port), the workflow is broken. Because it runs multiple times at the same time by worker threads.
- If a parser/plugin writes a file which has a constant name (e.g., common cache file), the workflow is broken as a race condition.

However, I think that the above is enough rare cases and the impact is limited.

## Alternatives

- [RFC 4](https://github.com/eslint/rfcs/pull/4) and [RFC 11](https://github.com/eslint/rfcs/pull/11) implement this feature behind a flag. On the other hand, this RFC does lint files in parallel by default if files are many, and people can enforce to not use workers by `--concurrency 1` option.

## Related Discussions

- https://github.com/eslint/eslint/issues/3565 - Lint multiple files in parallel [$500]
- https://github.com/eslint/eslint/issues/10606 - Make CLIEngine and supporting functions async via Promises
- https://github.com/eslint/eslint/pull/12191 - New: Allow parallel linting in child processes
- https://github.com/eslint/rfcs/pull/4 - New: added draft for async/parallel proposal
- https://github.com/eslint/rfcs/pull/11 - New: Lint files in parallel

[verify files]: #locally-concurrency
91 changes: 91 additions & 0 deletions designs/2019-worker-threads/diagrams.plantuml
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
@startuml overview
skinparam monochrome true
skinparam backgroundColor #ffffff
skinparam sequenceGroupBodyBackGroundColor transparent
skinparam boxPadding 10

box "Master"
participant "ESLint" as engine1
participant "LintResultCache" as cache
participant "fs" as fs1
end box

box "Workers"
participant "A worker" as worker
participant "CLIEngine" as engine2
participant "fs" as fs2
end box

[-> engine1 : executeOnFiles
activate engine1

engine1 -> engine1 : enumerate files

engine1 -> cache : check files if cached or not
activate cache
cache --> engine1
deactivate cache

engine1 -> engine1 : extract non-cached files

alt concurrency = 1
group in 16 concurrent
loop for each file
engine1 -> fs1 : read file content
deactivate engine1
activate fs1
fs1 -> engine1
deactivate fs1
activate engine1
engine1 -> engine1 : verify files
end
end
|||
else concurrency >= 2
loop 'concurrency' times
create worker
engine1 ->> worker : create worker
engine1 ->> worker : send options and initial files
deactivate engine1
activate worker
create engine2
worker -> engine2 : create
group in 16 concurrent
loop until no more file
worker -> engine2 : read config
activate engine2
engine2 --> worker
deactivate engine2
worker ->> fs2 : read file content
deactivate worker
activate fs2
fs2 ->> worker
deactivate fs2
activate worker
worker -> engine2 : verify file
activate engine2
engine2 --> worker
deactivate engine2
worker ->> engine1 : report lint result
deactivate worker
activate engine1
engine1 ->> worker : send next file if exist
deactivate engine1
activate worker
end
end
worker -->> engine1
destroy worker
activate engine1
end
note over engine1: wait for all workers exit
end

engine1 -> cache : update cache
activate cache
cache --> engine1
deactivate cache

[<-- engine1
deactivate engine1
@enduml