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

Chore: improve performance of `indent` rule #8905

Merged
merged 7 commits into from Jul 12, 2017

Conversation

Projects
None yet
5 participants
@not-an-aardvark
Copy link
Member

commented Jul 9, 2017

What is the purpose of this pull request? (put an "X" next to item)

[x] Other, please explain:

What changes did you make? (Give an overview)

This updates the indent rule to be more performant by storing information on ranges of tokens rather than performing operations on many individual tokens one-at-a-time.

This results in a 19% end-to-end performance increase (!) according to our npm run perf benchmark.

End-to-end benchmark

master branch:

Single File:
  CPU Speed is 3100 with multiplier 13000000
  Performance Run #1:  6601.645335ms
  Performance Run #2:  6537.859522ms
  Performance Run #3:  6469.447266ms
  Performance Run #4:  6777.504465ms
  Performance Run #5:  6431.605835ms

  Performance budget exceeded: 6537.859522ms (limit: 4193.548387096775ms)

indent-performance branch:

Single File:
  CPU Speed is 3100 with multiplier 13000000
  Performance Run #1:  5287.6544109999995ms
  Performance Run #2:  5284.148783ms
  Performance Run #3:  5326.879172ms
  Performance Run #4:  5338.387146ms
  Performance Run #5:  5340.651033ms

  Performance budget exceeded: 5326.879172ms (limit: 4193.548387096775ms)
Rule-specific benchmark on ESLint codebase

master branch:

$ TIMING=1 eslint lib/ conf/ bin/ tests/ --no-eslintrc --rule 'indent: [error, 4, {SwitchCase: 1}]' --env=es6 --no-inline-config
Rule   | Time (ms) | Relative
:------|----------:|--------:
indent |  5448.737 |   100.0%

indent-performance branch:

$ TIMING=1 eslint lib/ conf/ bin/ tests/ --no-eslintrc --rule 'indent: [error, 4, {SwitchCase: 1}]' --env=es6 --no-inline-config
Rule   | Time (ms) | Relative
:------|----------:|--------:
indent |  4610.891 |   100.0%

Is there anything you'd like reviewers to focus on?

I tried to write descriptive comments to explain what's going on. Is there anything that's unclear or could be improved?

Chore: improve performance of `indent` rule
This updates the `indent` rule to be more performant by storing information ranges of tokens rather than performing operations on many individual tokens one-at-a-time.
@eslintbot

This comment has been minimized.

Copy link

commented Jul 9, 2017

LGTM

@mention-bot

This comment has been minimized.

Copy link

commented Jul 9, 2017

@not-an-aardvark, thanks for your PR! By analyzing the history of the files in this pull request, we identified @vitorbal, @gyandeeps and @Trott to be potential reviewers.

@@ -46,6 +46,7 @@
"estraverse": "^4.2.0",
"esutils": "^2.0.2",
"file-entry-cache": "^2.0.0",
"functional-red-black-tree": "^1.0.1",

This comment has been minimized.

Copy link
@not-an-aardvark

not-an-aardvark Jul 9, 2017

Author Member

I marked this PR as WIP because this module appears to be unmaintained. Its readme also mentions that the immutable approach ends up hurting performance for consumers that don't need immutability. We might be able to get an even larger performance boost if we implement our own balanced BST (or use a different maintained module, but I had trouble finding another balanced BST module that has the features we need).

This comment has been minimized.

Copy link
@not-an-aardvark

not-an-aardvark Jul 10, 2017

Author Member

I decided to unmark this as WIP because I looked for more balanced BST libraries and didn't find any good alternatives at the moment. I also tried implementing one myself for awhile, but balanced BSTs are relatively complicated and difficult to get right, so it's probably not worth adding an implementation to the repo if this module is working for us right now.

@not-an-aardvark not-an-aardvark changed the title WIP: Chore: improve performance of `indent` rule Chore: improve performance of `indent` rule Jul 10, 2017

@eslintbot

This comment has been minimized.

Copy link

commented Jul 10, 2017

LGTM

@eslintbot

This comment has been minimized.

Copy link

commented Jul 11, 2017

LGTM

@ilyavolodin
Copy link
Member

left a comment

Very nice perf boost.

not-an-aardvark added some commits Jul 12, 2017

@eslintbot

This comment has been minimized.

Copy link

commented Jul 12, 2017

LGTM

@not-an-aardvark

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2017

I've made a few small improvements -- now this improves end-to-end performance by 20%.

Benchmark

master:

Single File:
  CPU Speed is 3100 with multiplier 13000000
  Performance Run #1:  5877.433702ms
  Performance Run #2:  5805.863323ms
  Performance Run #3:  5738.451947ms
  Performance Run #4:  5712.380049ms
  Performance Run #5:  5754.480289ms

  Performance budget exceeded: 5754.480289ms (limit: 4193.548387096775ms)

indent-performance:

Single File:
  CPU Speed is 3100 with multiplier 13000000
  Performance Run #1:  4577.530063ms
  Performance Run #2:  4641.629504ms
  Performance Run #3:  4603.666983ms
  Performance Run #4:  4684.530936ms
  Performance Run #5:  4614.500893ms

  Performance budget exceeded: 4614.500893ms (limit: 4193.548387096775ms)

(I think my laptop has a different battery level than when I ran the benchmark in the original PR, so these two runs are comparable to each other, but they're not comparable to the original two runs in terms of absolute time.)

@not-an-aardvark not-an-aardvark merged commit a8a8350 into master Jul 12, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@not-an-aardvark not-an-aardvark deleted the indent-performance branch Jul 12, 2017

@eslint eslint bot locked and limited conversation to collaborators Feb 6, 2018

@eslint eslint bot added the archived due to age label Feb 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.