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

perf(compression): use regex exec instead of match #210

Merged
merged 1 commit into from
Jul 27, 2024

Conversation

bluwy
Copy link

@bluwy bluwy commented Jul 26, 2024

I was running eslint-plugin-regexp in polka and found we can refactor the str.match(re) to re.exec(str) for perf.

/Users/bjorn/Work/oss/vite/node_modules/.pnpm/@[email protected]/node_modules/@polka/compression/build.js
  30:32  error  Use the `RegExp#exec()` method instead  regexp/prefer-regexp-exec
  30:68  error  Use the `RegExp#exec()` method instead  regexp/prefer-regexp-exec

I made some tests and can confirm there's a sizable perf improvement with exec. (perf.link test)

@kurtextrem
Copy link

might make sense to hoist the regexps too?

@bluwy
Copy link
Author

bluwy commented Jul 27, 2024

The perf improvement from hoisting regex varies quite a bit in my experience and often doesn't yield noticable returns. I think engines are able to optimize them well enough these days. Maybe those with g flags could still be useful to hoist as they're more stateful.

@lukeed lukeed merged commit 822c80a into lukeed:next Jul 27, 2024
@lukeed
Copy link
Owner

lukeed commented Jul 27, 2024

released as @polka/[email protected], ty @bluwy

@bluwy bluwy deleted the use-regex-exec branch July 27, 2024 14:43
@kurtextrem
Copy link

@bluwy Last time we benched it for valibot, it was ~2ms faster to hoist (even non global regexps)

@bluwy
Copy link
Author

bluwy commented Jul 27, 2024

Yeah it'll depend but it's often a very small amount that I've come to prefer not bugging libraries to hoist them unless really needed 😅

@lukeed
Copy link
Owner

lukeed commented Jul 27, 2024

It generally has a significant impact in repeat synchronous usage (eg loops), so seeing it in valibot makes sense. In an async-like callback (like here) it won’t matter much, especially with simple patterns

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants