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

fix: performance improvement #53

Merged
merged 4 commits into from Sep 29, 2021
Merged

fix: performance improvement #53

merged 4 commits into from Sep 29, 2021

Conversation

@Trott
Copy link
Contributor

@Trott Trott commented Sep 29, 2021

The performance measuring here is confounded a bit by an is-glob performance gotcha that is fixed in micromatch/is-glob#15. Here's what I'm seeing running time node -e "require('./')('/('.repeat(500000) + ')')" on my 2013 Macbook:

  • main branch with is-glob 4.0.2: No idea. I sent SIGINT after 12 minutes so more than that, and probably a lot more than that.
  • this PR with is-glob 4.0.2: 6.52 seconds
  • this PR with aforementioned is-glob PR: 80 milliseconds

(The added test will likely fail without the is-glob PR, depending on how performant the test machine is.)

@Trott
Copy link
Contributor Author

@Trott Trott commented Sep 29, 2021

The tests are failing because performance with the newly-added test is not good enough without the additional help of the is-glob update in micromatch/is-glob#15.

if (/[^\\][{[]/.test(str)) {
return true;
}
return isGlob(str);
Copy link
Member

@phated phated Sep 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anything weird (different?) by changing the order of the isGlob check and "globby" regexp test?

Copy link
Contributor Author

@Trott Trott Sep 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because they were OR'ed, I don't think it makes any difference logically. I'm happy to flip it back assuming that doesn't introduce performance regressions.

@phated
Copy link
Member

@phated phated commented Sep 29, 2021

@Trott can you update the package.json to require is-glob 4.0.3? 🍻

@Trott
Copy link
Contributor Author

@Trott Trott commented Sep 29, 2021

@Trott can you update the package.json to require is-glob 4.0.3? 🍻

Done.

@Trott
Copy link
Contributor Author

@Trott Trott commented Sep 29, 2021

All green!

package.json Outdated Show resolved Hide resolved
@phated phated merged commit 843f8de into gulpjs:main Sep 29, 2021
21 checks passed
@phated
Copy link
Member

@phated phated commented Sep 29, 2021

Published as 6.0.2 - thanks a ton @Trott!

@Trott Trott deleted the perf branch Sep 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants