Performance regression when updating from 2.9.0 to 2.10.0

[klimashkin]

Eslint check time on whole project takes 50% more time after updating from 2.9.0 to 2.10.0 even with disabled import/no-cycle rule.

My config:

{
    /** Import Static analysis https://github.com/benmosher/eslint-plugin-import#rules **/
    // Ensure imports point to a file/module that can be resolved
    'import/no-unresolved': [2, { caseSensitive: true, commonjs: true, amd: false }],
    // Ensure named imports correspond to a named export in the remote file
    'import/named': 2,
    // Ensure a default export is present, given a default import
    'import/default': 2,
    // Ensure imported namespaces contain dereferenced properties as they are dereferenced
    'import/namespace': [2, { allowComputed: true }],
    // Restrict which files can be imported in a given folder
    'import/no-restricted-paths': [0, { zones: [
      {target: './client', from: './app'},
    ] }],
    // Forbid import of modules using absolute paths
    'import/no-absolute-path': [2, { esmodule: true, commonjs: true, amd: false }],
    // Forbid require() calls with expressions (We use it for intl, svg icons)
    'import/no-dynamic-require': 0,
    // Prevent importing the submodules of other modules
    'import/no-internal-modules': 0,
    // Forbid Webpack loader syntax in imports
    'import/no-webpack-loader-syntax': 0,
    // Forbid a module from importing itself
    'import/no-self-import': 2,
    // Forbid useless path segments
    'import/no-useless-path-segments': 2,

    /** Import Helpful warnings https://github.com/benmosher/eslint-plugin-import#rules **/
    // Report any invalid exports, i.e. re-export of the same name
    'import/export': 2,
    // Report use of exported name as identifier of default export
    'import/no-named-as-default': 0,
    // Report use of exported name as property of default export
    'import/no-named-as-default-member': 2,
    // Report imported names marked with @deprecated documentation tag
    'import/no-deprecated': 0,
    // Forbid the use of extraneous packages, allow devDependencies
    'import/no-extraneous-dependencies': [2, { devDependencies: true, packageDir: path.resolve('./') }],
    // Forbid the use of mutable exports with var or let
    'import/no-mutable-exports': 2,

    /** Import Module systems https://github.com/benmosher/eslint-plugin-import#rules **/
    // Report potentially ambiguous parse goal
    'import/unambiguous': 0,
    // Report CommonJS require calls and module.exports or exports.*
    'import/no-commonjs': 0,
    // Report AMD require and define calls
    'import/no-amd': 0,
    // No Node.js builtin modules. Allow in general, forbid in ./app
    'import/no-nodejs-modules': 0,

    /** Import Style guide https://github.com/benmosher/eslint-plugin-import#rules **/
    // Ensure all imports appear before other statements
    'import/first': 2,
    // Ensure all exports appear after other statements
    'import/exports-last': 0,
    // Report repeated import of the same module in multiple places
    'import/no-duplicates': 2,
    // Report namespace imports
    'import/no-namespace': 0,
    // Ensure consistent use of file extension within the import path. Use extensions except .js/jsx
    'import/extensions': [2, 'always', { js: 'never', jsx: 'never' }],
    // Enforce a convention in module import order. TODO: discuss import order that we want and apply
    'import/order': [0, {
      'groups': ['builtin'],
      'newlines-between': 'ignore',
    }],
    // Enforce a newline after import statements. 'padding-line-between-statements' handles this for us
    'import/newline-after-import': 0,
    // Prefer a default export if module exports a single name
    'import/prefer-default-export': 0,
    // Limit the maximum number of dependencies a module can have
    'import/max-dependencies': 0,
    // Forbid unassigned imports
    'import/no-unassigned-import': 0,
    // Forbid named default exports
    'import/no-named-default': 0,
    // Forbid default exports
    'import/no-default-export': 0,
    // Forbid anonymous values as default exports
    'import/no-anonymous-default-export': 0,
    // Prefer named exports to be grouped together in a single export declaration
    'import/group-exports': 0,
}

[lukeapage]

confirmed, same issue, though for me it goes from 2 minutes to run on our project to 10 minutes and runs out of memory. No difference in rules enabled.

memory usage 2.9 - ~ 200MB
memory usage 2.10 - ~1500MB

[benmosher]

ah, brutal. I saw similar stuff but it looked like it was no-cycle when I tested. thanks for the feedback and sorry for the trouble. will dig in ASAP.

[benmosher]

I am seeing super high memory usage (>2GB) for both 2.9 and 2.10. Do either of you have lockfiles showing what other dependency versions changed when upgrading from 2.9 to 2.10?

If not, no worries. Given the huge difference in time+space, I am assuming it is some issue with the in-memory parsed module cache, it must be generating universally distinct hash keys for some reason...

I tried explicitly downgrading eslint-module-utils to the lowest allowed version for v2.9 and wasn't able to get perf/memory to improve relative to 2.10, for my large-ish reference project. I saw this same space/time usage similarity before I published, as I recall, so I figured it had not changed.

[klimashkin]

I checked package-lock.json, only eslint-plugin-import got updated from 2.9.0 to 2.10.0. Time increased from ~66s to ~105s in my case

[lukeapage]

Same here. we use shrinkwrap and its only the core package change when upgrading. I'm going to try changing some code in the package to see if I can identify the change

[lukeapage]

If I remove this line:
ed719a3#diff-e991b36c6628220049668b5e2d2c43d8R416

thats line 416 of ExportMap

     if (n.type === 'ImportDeclaration') {
//      captureDependency(n)
       let ns

then the problem does not occur.

Is the problem that its capturing the source code of every file I am linting?

I'm using babel-eslint as the parser if that makes any difference.

[benmosher]

Ohhhhh no:

  function captureDependency(declaration) {
    if (declaration.source == null) return null

    const p = remotePath(declaration)
    if (p == null || m.imports.has(p)) return p

    const getter = () => ExportMap.for(p, context)
    m.imports.set(p, { getter, source: declaration.source })  // <== this.
    return p
  }

I bet it's this: source: declaration.source. I'm probably holding the full AST for every linted file in memory, yeah...

edit: well, tried capturing only the necessary fields from source (value + loc) and it didn't matter. to the devtools! will run some heap analysis, maybe something else is also capturing ASTs unintentionally...

[danez]

Same issue here. When I looked at the heap there where a lot of ExportMap things that used tons of memory. We have a large codebase 10k+ js files and with 2.10 eslint always runs out of memory.

edit: Also using babel-eslint

[benmosher]

Good info. I think I'm onto something but ran out of time this morning.

I'll push my branch (fix-perf) if anyone wants to look at it. I think it's no accident that I already saw terrible perf/memory in 2.9; I think the line @lukeapage identified is exacerbating it as of v2.10. Definitely looks like the ExportMap infrastructure is retaining ASTs.

[benmosher]

update: it dawned on me that I saw the same memory issues with 2.9 because my project uses the memo-parser which necessarily retains the parse trees for all linted files + dependencies.

I will re-run my tests at some point without it to see if the changes I've made so far avoid retaining them within the plugin code (specifically via ExportMap).

[benmosher]

published fix in v2.11, AFAICT. let me know if you still see issues, and I can reopen if so 🙆‍♂️

[lukeapage]

Thanks for fixing this. I've upgraded to 2.11 and its fine. Sorry I couldn't confirm earlier.

[klimashkin]

Fixed, thank you!

[JakubJagoda]

The problem still appears for me in 2.11.0 if used with --fix option. Without autofix it runs fine, with autofix there's no response for a few minutes until JS runs out of the memory
FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory

[benmosher]

@JakubJagoda are you using the first or order rules? --fixers were added to each of those rules between 2.9 and 2.11.

[JakubJagoda]

@benmosher sorry, should have mentioned - I am using the order rule, with following config (just in case):

"import/order": [
      "error",
      {
        "groups": [
          [
            "builtin",
            "external",
            "internal"
          ],
          [
            "index",
            "sibling",
            "parent"
          ]
        ],
        "newlines-between": "ignore"
      }
    ]

[benmosher]

@JakubJagoda can you open a new issue regarding order fixer perf? ideally with a link back to the fixer PR. thanks!

[soryy708]

Here's an idea for how to make import/no-cycle faster:
#2937