Skip to content

Large performance regression in v2.0 #649

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

Closed
schmod opened this issue Nov 2, 2016 · 4 comments · Fixed by #654
Closed

Large performance regression in v2.0 #649

schmod opened this issue Nov 2, 2016 · 4 comments · Fixed by #654

Comments

@schmod
Copy link
Contributor

schmod commented Nov 2, 2016

I'm observing a very large performance hit between v1.x and v2.0.1. Linting my project used to take approximately 13 seconds with v1.x, and now takes about 25 minutes with v2.0.1.

I'm using ESLint v3.9.1, eslint-plugin-import v2.0.1, eslint-import-resolver-webpack v0.6.0, and babel-eslint v7.1.0.

I've tried reverting to earlier versions each of those packages, and I only see a noticeable performance regression with eslint-plugin-import v2.0.x.


My .eslintrc.js is as follows:

module.exports = {
  rules: {
    "import/default": 0,
  },
  parser: "babel-eslint",
  env: {
    browser: true,
    es6: true
  },
  globals: {
    console: false,
    require: false
  },
  extends: [
      "../.eslintrc.js",
      "plugin:import/errors",
      "plugin:import/warnings"
  ],
  plugins: [
      "import"
  ],
  settings: {
      "import/resolver": "webpack",
      "import/extensions": ['.js']
  }
}

I can't release the source code of my project, but would be happy to help track down this bug by some other means. I've already thrown a profiler on the process, and unfortunately didn't see anything particularly interesting...

@benmosher
Copy link
Member

Dang, yeah, something seems to not be properly detecting + rejecting CommonJS deps before parsing (which is what eats all the time).

See discussion on #615 for a workaround using import/ignore to always skip parsing node_modules:

  settings: {
      "import/resolver": "webpack",
      "import/extensions": ['.js'],
      "import/ignore": ['node_modules']
  }

Beware, though, that it will not lint imported names/defaults for your node_modules thereafter. no-unresolved and others will still check that the imported files exist, it just won't parse them.

When I add debug logging to start digging into this, I may ping you and ask for some logs, if you're up for that. The v2 logic is not substantially different, so it should not be anywhere near that much slower.

@benmosher benmosher added this to the v3 - import/order internal milestone Nov 3, 2016
@benmosher benmosher modified the milestones: unambiguous regex false positives, v3 - import/order internal Nov 3, 2016
@schmod
Copy link
Contributor Author

schmod commented Nov 3, 2016

Sure. Happy to help. I've downgraded to v1.x, which should work fine for me in the short-term!

@schmod
Copy link
Contributor Author

schmod commented Nov 3, 2016

BTW, running ESLint with the --debug flag does not reveal anything particularly interesting.

Watching the logs scroll by, each file is taking a long time to process, but the "timed" tasks listed in the log all claim to be very fast, despite an obvious 2-3 second pause between files.

  eslint:cli-engine Processing file1.js +3s
  eslint:cli-engine Linting file1.js +2ms
  eslint:config Constructing config for file1.js +0ms
  eslint:config Using config from cache +0ms

*** [long pause here]***

  eslint:cli-engine Processing file2.js +2s
  eslint:cli-engine Linting file2.js +2ms
  eslint:config Constructing config for file2.js +0ms
  eslint:config Using config from cache +0ms

benmosher added a commit that referenced this issue Nov 5, 2016
benmosher added a commit that referenced this issue Nov 7, 2016
* fix logical gaffs preventing unambiguous regex from functioning properly.

fixes #615
fixes #634
fixes #649

* fixed typo 😅
@schmod
Copy link
Contributor Author

schmod commented Nov 7, 2016

MUCH better. Thanks for the quick fix!

v1.16.0

real    0m3.460s
user    0m2.973s
sys 0m0.307s

v2.0.1:

real    0m47.238s
user    0m39.430s
sys 0m1.533s

v2.2.0

real    0m3.726s
user    0m3.190s
sys 0m0.321s

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

Successfully merging a pull request may close this issue.

2 participants