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: cache results in runtime-info #12320

Merged
merged 1 commit into from Sep 28, 2019
Merged

Conversation

@kaicataldo
Copy link
Member

kaicataldo commented Sep 26, 2019

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

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain:

What changes did you make? (Give an overview)

This change improves performance by caching the results of the shell commands the RuntimeInfo module executes. In practice, this only improves performance when ESLint is installed locally and globally (which we're actively discouraging these days), but it's something!

In the code path described above, I'm seeing these results on my local machine (average over 5 runs with each branch):

master: ~3.072s
runtime-info-cache-results: ~2.826s

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

Nothing in particular. This PR is a lot easier to review by hiding whitespace changes.

@eslint eslint bot added the triage label Sep 26, 2019
@kaicataldo kaicataldo force-pushed the runtime-info-cache-results branch from 12689e4 to 67e3db4 Sep 26, 2019
@kaicataldo kaicataldo changed the title Chore: cache reuslts in runtime-info Chore: cache results in runtime-info Sep 26, 2019
@kaicataldo kaicataldo added chore and removed triage labels Sep 26, 2019
@kaicataldo kaicataldo force-pushed the runtime-info-cache-results branch from 67e3db4 to c01e5a1 Sep 26, 2019
Copy link
Member

platinumazure left a comment

LGTM, thanks!

Copy link
Member

mysticatea left a comment

LGTM, thank you!

@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Sep 27, 2019

As a side note, we may be able to reduce spawn by using the packages in @npmcli.

@kaicataldo kaicataldo merged commit 29c12f1 into master Sep 28, 2019
16 checks passed
16 checks passed
Verify Files
Details
Test (ubuntu-latest, 8.x)
Details
Test (ubuntu-latest, 10.x)
Details
Test (ubuntu-latest, 12.x)
Details
Test (windows-latest, 12.x)
Details
Test (macOS-latest, 12.x)
Details
Browser Test
Details
commit-message Commit message follows guidelines
Details
continuous-integration Build #20190926.13 succeeded
Details
continuous-integration (Test on Node.js 10 (Linux)) Test on Node.js 10 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Linux)) Test on Node.js 12 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Windows)) Test on Node.js 12 (Windows) succeeded
Details
continuous-integration (Test on Node.js 12 (macOS)) Test on Node.js 12 (macOS) succeeded
Details
continuous-integration (Test on Node.js 8 (Linux)) Test on Node.js 8 (Linux) succeeded
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@kaicataldo kaicataldo deleted the runtime-info-cache-results branch Sep 28, 2019
@eslint eslint bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.