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

Performance regression when updating from 2.9.0 to 2.10.0 #1058

Closed
klimashkin opened this issue Apr 2, 2018 · 18 comments
Closed

Performance regression when updating from 2.9.0 to 2.10.0 #1058

klimashkin opened this issue Apr 2, 2018 · 18 comments
Assignees

Comments

@klimashkin
Copy link

klimashkin commented Apr 2, 2018

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
Copy link
Contributor

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
Copy link
Member

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 benmosher mentioned this issue Apr 9, 2018
1 task
@benmosher
Copy link
Member

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
Copy link
Author

klimashkin commented Apr 9, 2018

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member

benmosher commented Apr 10, 2018

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
Copy link

danez commented Apr 10, 2018

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
Copy link
Member

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
Copy link
Member

benmosher commented Apr 11, 2018

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
Copy link
Member

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

@lukeapage
Copy link
Contributor

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

@klimashkin
Copy link
Author

Fixed, thank you!

@JakubJagoda
Copy link

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
Copy link
Member

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

@JakubJagoda
Copy link

@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
Copy link
Member

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

@soryy708
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

6 participants