Fix: Improves perf of globbing by inheriting glob.GlobSync (fixes #6710)

[kaelzhang]

What issue does this pull request address?
As #6710 discussed, I create a subclass of glob.GlobSync to skip certain glob process according to .gitignore rules.

What changes did you make? (Give an overview)

• inherits glob.GlobSync and overrides _readdir, special thanks to the good idea of @mysticatea. But there are several detail things we should aware of, see the inline comments of the commit.
• change ignoredPaths.getIgnoredFoldersGlobPatterns() -> ignoredPaths.getIgnoredFoldersGlobChecker(), and now it will return a function.

Is there anything you'd like reviewers to focus on?
Do I need to create a specific test cases file for lib/glob.js? Or just testing it with glob-util.js is ok? I did create a test file, because I thought there was much duplication with tests/lib/glob-util.js

If it is necessary, I will add one.

[eslintbot]

LGTM

[mention-bot]

@kaelzhang, thanks for your PR! By analyzing the annotation information on this pull request, we identified @alberto, @IanVS and @gronke to be potential reviewers

[jquerybot]

Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA).

📝 Please visit http://contribute.jquery.org/CLA/ to sign.

After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know.

---

If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check.

[mysticatea]

Node.js supports symbols since 0.12. ESLint requires >=Node 4, so we can use symbols always.

http://node.green/#Symbol

[ilyavolodin]

This looks a lot better then previous attempt, @kaelzhang could you rebase please? @eslint/eslint-team Can you review this, since this is a pretty impactful change, I'd like to have as many eyes on this as we can? Especially @IanVS and @alberto since you've been dealing with this part of the code lately.

[IanVS]

It looks like both times that DEFAULT_IGNORE_DIRS is used we are adding the * to the end, so how about just changing DEFAULT_IGNORE_DIRS? Also, the other usage of DEFAULT_IGNORE_DIRS adds a leading slash, so maybe the final result should be:

 var DEFAULT_IGNORE_DIRS = [
     "/node_modules/*",
     "/bower_components/*"
 ];

I do like the note explaining why the * is needed, though!

[IanVS]

I tried to rebase and got bogged down, so I'm going to wait for @kaelzhang to rebase it. Then I would like to see how it would play together with https://github.com/eslint/eslint/pull/6844/files. I don't necessarily anticipate any problems, but it's all pretty related so it would be good to check.

From what I can tell just looking over the code and the tests, this all looks good (with the caveat that I don't fully understand the changes to the glob functionality).

[kaelzhang]

@IanVS Sorry that I have been away for a while. I will work on this asap.

[eslintbot]

LGTM

[kaelzhang]

@IanVS This pr will not solve #6759 which I think should be fixed separately from the current issue.

[eslintbot]

LGTM

[kaelzhang]

In the near future, we need to change this.

[kaelzhang]

@IanVS I've made some changes related to #6759 so that there won't be too much changes the day we want to fix that issue.

I also marked a few lines that should be altered later.

[kaelzhang]

Appveyor reached a timeout

Build execution time has reached the maximum allowed time for your plan (60 minutes).

[eslintbot]

LGTM

[IanVS]

It seems my previous comment #6783 (comment) was lost in the rebase. I still think it would be cleaner to just make the DEFAULT_IGNORE_DIRS the way we want them.

[IanVS]

@kaelzhang I rebased my #6844 on top of this branch and it all still works fine. The code of this PR looks great, but it looks like you need to sign the jQuery CLA before we could merge this.

Also, do you have any performance data to show that speed is improved as a result of these changes?

[eslintbot]

LGTM

[eslintbot]

LGTM

[kaelzhang]

@IanVS Seems that I can't change my full name of jQuery CLA signature, so I had to change my name in git config 😢

I'll add some performance data later.

[eslintbot]

LGTM

[eslintbot]

LGTM

[IanVS]

Responding to your note from another issue: the code LGTM, but I'm not an official reviewer. Also, I think you were going to try to gather some performance data, is that still possible? I think it would be helpful.

[kaelzhang]

I tested on an npm package, dat which has 506 dependencies (including subtle dependencies).

.eslintignore:

!node_modules/acorn

.eslintrc

{
  "root": true,
  "extends": "eslint:recommended"
}

And the script is:

const spawn = require('cross-spawn').sync

const time1 = + new Date

// local eslint(pr #6783) created with npm link
spawn('eslint', ['.'], { stdio: 'inherit' })
const time2 = + new Date

// eslint 3.3.0
spawn('./node_modules/.bin/eslint', ['.'], { stdio: 'inherit' })
const time3 = + new Date

console.log(`
eslint npm package, dat:

- eslint #6783 : ${time2 - time1} ms
- eslint 3.3.0  : ${time3 - time2} ms
`)

Output:

eslint npm package, dat:

- eslint #6783 : 1861 ms
- eslint 3.3.0 : 2709 ms

[IanVS]

So, am I reading correctly that with this branch it took longer to lint? :-/

[kaelzhang]

@IanVS I made a mistake with the last console.log(). This branch lints much faster, and the result is not excluded with the lint part (CLI-Engine -> Lint)

[mysticatea]

👍

[alberto]

LGTM, thanks @kaelzhang!

[ilyavolodin]

LGTM. Thanks @kaelzhang for your work on this!