no-unused-modules hangs & overflows memory

[soryy708]

In version 2.4.0

Running with the following rule:

"import/no-unused-modules": [
    "error",
    {"unusedExports": true}
]

eslint hangs for a minute, and then node prints a dump:

<--- Last few GCs --->

[13612:00000273A3998820] 138421 ms: Mark-sweep 1393.8 (1425.0) -> 1393.4 (1424.0) MB, 2458.2 / 0.0 ms (average mu = 0.060, current mu = 0.008) allocation failure scavenge might not succeed
[13612:00000273A3998820] 138429 ms: Scavenge 1394.6 (1424.0) -> 1394.2 (1426.5) MB, 5.7 / 0.0 ms (average mu = 0.060, current mu = 0.008) allocation failure

<--- JS stacktrace --->

==== JS stack trace =========================================

0: ExitFrame [pc: 00000367C635C5C1]

Security context: 0x02cfc449e6e1
1: visitSingle [000001E8CE77CE21] [C:\Users\Ivan Rubinson\Documents\Software Engineering\Work\PROJECTNAME\Front\node_modules\babel-eslint\node_modules@babel\traverse\lib\context.js:~88] [pc=00000367C6C797DF](this=0x02e4eee45b41 ,node=0x02f0c5c7de69 ,key=0x02f78ec07d79 <String[2]: id>)
...

FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory
1: 00007FF79725ECE5
2: 00007FF797238196
3: 00007FF797238BA0
4: 00007FF7974C8D5E
5: 00007FF7974C8C8F
6: 00007FF797A069D4
7: 00007FF7979FD137
8: 00007FF7979FB6AC
9: 00007FF797A04627
10: 00007FF797A046A6
11: 00007FF7975A7767
12: 00007FF79763F44A
13: 00000367C635C5C1
npm ERR! code ELIFECYCLE
npm ERR! errno 134
npm ERR! base-boilerplate@2.0.0 lint: eslint ./client/src/
npm ERR! Exit status 134
npm ERR!
npm ERR! Failed at the base-boilerplate@2.0.0 lint script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR! C:\Users\Ivan Rubinson\AppData\Roaming\npm-cache_logs\2019-05-20T16_10_57_005Z-debug.log

The log from npm-cache is:

0 info it

worked if it ends with ok
1 verbose cli [ 'C:\Program Files\nodejs\node.exe',
1 verbose cli 'C:\Program Files\nodejs\node_modules\npm\bin\npm-cli.js',
1 verbose cli 'run',
1 verbose cli 'lint' ]
2 info using npm@6.4.1
3 info using node@v10.13.0
4 verbose run-script [ 'prelint', 'lint', 'postlint' ]
5 info lifecycle base-boilerplate@2.0.0prelint: base-boilerplate@2.0.0
6 info lifecycle base-boilerplate@2.0.0lint: base-boilerplate@2.0.0
7 verbose lifecycle base-boilerplate@2.0.0lint: unsafe-perm in lifecycle true
9 verbose lifecycle base-boilerplate@2.0.0lint: CWD: C:\Users\Ivan Rubinson\Documents\Software Engineering\Work\PROJECTNAME\Front
10 silly lifecycle base-boilerplate@2.0.0lint: Args: [ '/d /s /c', 'eslint ./client/src/' ]
11 silly lifecycle base-boilerplate@2.0.0lint: Returned: code: 134 signal: null
12 info lifecycle base-boilerplate@2.0.0~lint: Failed to exec lint script
13 verbose stack Error: base-boilerplate@2.0.0 lint: eslint ./client/src/
13 verbose stack Exit status 134
13 verbose stack at EventEmitter. (C:\Program Files\nodejs\node_modules\npm\node_modules\npm-lifecycle\index.js:301:16)
13 verbose stack at EventEmitter.emit (events.js:182:13)
13 verbose stack at ChildProcess. (C:\Program Files\nodejs\node_modules\npm\node_modules\npm-lifecycle\lib\spawn.js:55:14)
13 verbose stack at ChildProcess.emit (events.js:182:13)
13 verbose stack at maybeClose (internal/child_process.js:962:16)
13 verbose stack at Process.ChildProcess._handle.onexit (internal/child_process.js:251:5)
14 verbose pkgid base-boilerplate@2.0.0
15 verbose cwd C:\Users\Ivan Rubinson\Documents\Software Engineering\Work\PROJECTNAME\Front
16 verbose Windows_NT 10.0.17134
17 verbose argv "C:\Program Files\nodejs\node.exe" "C:\Program Files\nodejs\node_modules\npm\bin\npm-cli.js" "run" "lint"
18 verbose node v10.13.0
19 verbose npm v6.4.1
20 error code ELIFECYCLE
21 error errno 134
22 error base-boilerplate@2.0.0 lint: eslint ./client/src/
22 error Exit status 134
23 error Failed at the base-boilerplate@2.0.0 lint script.
23 error This is probably not a problem with npm. There is likely additional logging output above.
24 verbose exit [ 134, true ]

[fkoehler42]

Similar issue here on last released version (2.17.3). Actually eslint does work but it takes an insane amount of time to complete, more than 10min on my current react-native project while downgrading the plugin to version 2.17.2 get things back to normal (~25sec)

[rfermann]

Could you please provide a demo repo for this case?

[lukeapage]

@rfermann
It takes me over an hour to get back eslint results (its a large project, without this rule it takes 8 mins to run). It looks to me like this change:
bb686de
may have done it.
Specifically refreshing source files.. it seems to add 2-3 seconds per check for me.

(But many thanks for the great rule, its been almost impossible to find dead code before this)

[rfermann]

@lukeapage thanks for the context. I guess, reading files from the src folder multiple times (once per linted file) causes your issue. The time for this task seems to increase overproportional when having more files to lint.

I will try to find a solution for this issue within the next days.

[soryy708]

What's the current algorithm? What's its time complexity? Opening files is an expensive operation; maybe open them for reading & don't close until done linting?

[rfermann]

@soryy708 the rule is building up a list of all files within the specified src folder by reading from the file system. It does so to prevent linting files which either have to be ignored or are located outside the src folder. Linting these files may lead to a crash of the rule as these haven't been analyzed before.

Currently this list is getting refreshed on each lint, e.g for 1000 linted files, this list is refreshed 1000 times.

One solution could be to only reread the src-files, if the currently linted file hasn't been found in the list of source files first.

So, instead of using this logic:

// refresh list of source files
const srcFiles = resolveFiles(getSrc(src), ignoreExports)

// make sure file to be linted is included in source files
if (!srcFiles.has(file)) {
  return
}

we could use this logic:

// refresh list of source files
// const srcFiles = resolveFiles(getSrc(src), ignoreExports)

// make sure file to be linted is included in source files
if (!srcFiles.has(file)) {
  srcFiles = resolveFiles(getSrc(src), ignoreExports)
  if (!srcFiles.has(file)) {
    return
  }
}

Tests are still passing with this logic, so there is no change in the logic of the rule.

Benchmarking this change brings down the execution time of this rule from more than 830ms to approx. 115ms using the files of our tests.
Taking out all tests creating new files during runtime (which makes it necessary to reread the list of relevant source files), reduces the execution time to something below 100ms.

This change takes the biggest effect, if the src specified in the rule options is congruent with the src eslint is going to lint.
If the src option of this rule is only a small subset of the files being linted, the execution time might not drop that much.

@lukeapage could you implement my change in your local version of this plugin and lint your project?
Ideally you could also benchmark the lint run before and after the change. export TIMING=1 before linting your project will give you an overview about the 10 most expensive rules.

@ljharb what do you think about my suggested change?

[ljharb]

Your change sounds great.

Separately, I don't think we have to be concerned with the files changing during the same eslint run - so perhaps just reading it once is sufficient?

[rfermann]

Reading these files not only on the startup of ESLint is basically to make this rule work in code editors. When ESLint is started as a plugin in e.g. VS Code, it reads the files once and does not refresh the list of files while the plugin is running. When creating some new files, this rule will not consider the new files, possibly reporting some exports as unused although they are used in one of the new files.

[ljharb]

hmm, are you sure they don't invoke eslint fresh for each run? if they use something like eslint_d, i'd expect it to provide some kind of hook to tell plugins that it's a fresh run.

[soryy708]

@ljharb That depends on the editor, and I imagine eslint wants to support as many as possible. While a naive implementation (like you suggested) exists, I'm sure there are editors out there that will go the extra mile for a more optimal but sophisticated solution.

[ljharb]

I wonder if we could maybe shasum the file, and only refresh the file list when it’s changed, not just when it’s outside the src?

[rfermann]

I don't know how expensive this will be and if it is even more expensive than rereading the file system under some circumstances. And I never worked with checksums before, so I would need support in implementing this.

Another possibility of not refreshing the file list too often could be a second Set containing all files which have already been looked up but are not within src:

// refresh list of source files
// const srcFiles = resolveFiles(getSrc(src), ignoreExports)

// make sure file to be linted is included in source files
if (filesOutsideSource.has(file)) {
  return
}

if (!srcFiles.has(file)) {
  srcFiles = resolveFiles(getSrc(src), ignoreExports)
  if (!srcFiles.has(file)) {
    filesOutsideSource.add(file)
    return
  }
}

This way we would only refresh the file list once per file outside the source folder.

[ljharb]

I like that approach; that would work for long-running tasks as well, unless you’d changed the eslint config which generally requires an editor restart anyways.