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: Apply memoization to config creation within glob utils #9944

Merged
merged 4 commits into from Feb 16, 2018

Conversation

@brokentone
Copy link
Contributor

brokentone commented Feb 5, 2018

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)

I attempted a brief review of eslint with performance in mind and I found (just) a few quick wins.

Running the repo's lint task with --debug I first noticed that the ignore file processing occurred multiple times in a row without significant changes.

2018-02-05T02:52:54.126Z eslint:cli Running on files
2018-02-05T02:52:54.131Z eslint:glob-util Creating list of files to process.
2018-02-05T02:52:54.132Z eslint:ignored-paths Looking for ignore file in ~/Projects/eslint
2018-02-05T02:52:54.132Z eslint:ignored-paths Loaded ignore file /Users/kjacobse/Projects/eslint/.eslintignore
2018-02-05T02:52:54.132Z eslint:ignored-paths Adding ~/Projects/eslint/.eslintignore
2018-02-05T02:52:54.213Z eslint:ignored-paths Looking for ignore file in ~/Projects/eslint
2018-02-05T02:52:54.213Z eslint:ignored-paths Loaded ignore file /Users/kjacobse/Projects/eslint/.eslintignore
2018-02-05T02:52:54.213Z eslint:ignored-paths Adding ~/Projects/eslint/.eslintignore
2018-02-05T02:52:54.215Z eslint:ignored-paths Looking for ignore file in~/Projects/eslint
2018-02-05T02:52:54.215Z eslint:ignored-paths Loaded ignore file /Users/kjacobse/Projects/eslint/.eslintignore
2018-02-05T02:52:54.215Z eslint:ignored-paths Adding ~/Projects/eslint/.eslintignore
2018-02-05T02:52:54.216Z eslint:ignored-paths Looking for ignore file in ~/Projects/eslint
2018-02-05T02:52:54.216Z eslint:ignored-paths Loaded ignore file /Users/kjacobse/Projects/eslint/.eslintignore
2018-02-05T02:52:54.216Z eslint:ignored-paths Adding ~/Projects/eslint/.eslintignore
2018-02-05T02:52:54.219Z eslint:cli-engine Processing ~/Projects/eslint/lib/api.js
2018-02-05T02:52:54.219Z eslint:cli-engine Linting ~/Projects/eslint/lib/api.js
2018-02-05T02:52:54.219Z eslint:config Constructing config file hierarchy for ~/Projects/eslint/lib
...

In this branch, the above ignored-paths steps only occur once.

Anecdotally, I have been seeing pretty consistent gains in the perf execution, but even bumping the runs up a lot, it's simply too noisy to post and stand behind. I can attempt some more work on this if desired.

That all said, what I have done is prevent a new object creation (due to its cost and loss of referential equality) when it's unnecessary.

Then once that has been done, since I noticed these executions are more likely to be similar than different, I included two small, memoized functions to help create this object, and get a new instance of the IgnoredPaths class.

Is there anything you'd like reviewers to focus on?
I need help testing my assumptions:

  • I am assuming that downstream consumers do not need unique copies of IgnoredPaths class. I feel pretty good about the immediate consumer addFile, but I don't necessarily have enough context for the rest.
  • I am also assuming that this eslint runtime has a finite shelflife -- if not this memoization in the module scope is a classic memory leak (though there also may be finite enough inputs that it also may not matter), but nonetheless, worth the discussion.
@jsf-clabot

This comment has been minimized.

Copy link

jsf-clabot commented Feb 5, 2018

CLA assistant check
All committers have signed the CLA.

Copy link
Member

platinumazure left a comment

Thanks for the PR! Just one small "house rule" style change request, this otherwise looks promising. Thanks!

@@ -8,7 +8,8 @@
// Requirements
//------------------------------------------------------------------------------

const fs = require("fs"),
const _ = require("lodash"),

This comment has been minimized.

Copy link
@platinumazure

platinumazure Feb 5, 2018

Member

We prefer just using lodash variable in the rest of the codebase, would you mind switching that up here? Thanks!

@@ -88,6 +89,18 @@ function resolveFileGlobPatterns(patterns, options) {
return patterns.filter(p => p.length).map(processPathExtensions);
}

const dotfilesPattern = /(?:(?:^\.)|(?:[/\\]\.))[^/\\.].*/;

const memoGetIgnorePaths = _.memoize(

This comment has been minimized.

Copy link
@platinumazure

platinumazure Feb 5, 2018

Member

I'm not convinced this function (or the one below) needs a "memo" prefix, but I won't insist on a change here.

@brokentone

This comment has been minimized.

Copy link
Contributor Author

brokentone commented Feb 5, 2018

@platinumazure changes made! Left the commit out for clarity for now, will rebase them in later.

@not-an-aardvark

This comment has been minimized.

Copy link
Member

not-an-aardvark commented Feb 5, 2018

Thanks for the PR! To clarify, does this just memoize the creation of the IgnoredPaths object, and not the results of checking the filesystem for ignored paths? (I wouldn't want to do the latter because the result could change if someone adds a file and runs ESLint again in the same process, e.g. as part of an editor integration.)

@brokentone

This comment has been minimized.

Copy link
Contributor Author

brokentone commented Feb 5, 2018

@not-an-aardvark that is correct.

To your point, I'm not sure how some of the editor integrations happen, but if they're long-running, and if it's possible that this options object gets mutated upstream, we might provide an old response due to a false positive on referential equality. I'm imagining this is not the case though.

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Feb 6, 2018

As far as I remember, there are a few projects that use long-running eslint through APIs. I think VSC uses eslint-server which keeps eslint process running in memory. There's also eslint_d project that does the same. So if this might result in memory leak for long-running instances, this might cause problems.

@brokentone

This comment has been minimized.

Copy link
Contributor Author

brokentone commented Feb 6, 2018

Alright, I've updated -- now the memozation only runs for the duration of the listFilesToProcess function which should prevent memory leaks, while possibly still improving performance. I also had to back out the Object.assign memoization as lodash's (and most) memoization only acts on a single argument, which would have introduced issues... at which point a function to wrap an Object.assign felt like unnecessary indirection. Unfortunately I'm no longer able to prove benefits (the 4x runs I noted in the description are back).

Theoretically the code is improved and could be merged as such with minimal risk, or I can continue looking into this.

Copy link
Member

kaicataldo left a comment

LGTM. Thanks for contributing to ESLint!

@kaicataldo kaicataldo merged commit 70f22f3 into eslint:master Feb 16, 2018
5 checks passed
5 checks passed
commit-message PR title follows commit message guidelines
Details
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
release-monitor No patch release is pending
Details
@renovate renovate bot mentioned this pull request Feb 16, 2018
This was referenced Mar 22, 2018
@renovate renovate bot mentioned this pull request Mar 23, 2018
@eslint eslint bot locked and limited conversation to collaborators Aug 16, 2018
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

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