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: only create regex once #49

Merged
merged 7 commits into from May 28, 2023
Merged

Conversation

gtm-nayan
Copy link
Contributor

With chalk/wrap-ansi#51, a large portion of the time in wrap-ansi is now spent in ansiRegex because strip-ansi creates a new regex on every call and it's called for every word in wrap-ansi.

Before:
image

After:
image

@sindresorhus
Copy link
Member

You need to reset the .lastIndex and also benchmark with that to ensure it improves performance still.

@gtm-nayan
Copy link
Contributor Author

image

Huh, weird, resetting the last index does actually make this function 2x slower, still a lot faster than before tho.

Good news is, I think the lastIndex doesn't have to be reset when using replace particularly, it does it automatically

image

This reverts commit 0078ce6.
@sindresorhus
Copy link
Member

That is quite subtle. Found some info about it:

Because @@replace would keep calling exec() until it returns null, and exec() would automatically reset the regex's lastIndex to 0 when the last match fails, @@replace would typically not have side effects when it exits. However, when the regex is sticky but not global, lastIndex would not be reset. In this case, each call to replace() may return a different result.

@sindresorhus
Copy link
Member

Can you add a short code comment that states why we don't need to reset .lastIndex. Otherwise, I fear it will be added in the future accidentally.

@sindresorhus sindresorhus merged commit 840a5ea into chalk:main May 28, 2023
1 check passed
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.

None yet

2 participants