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(node:util): fast path for extractedSplitNewLines #15838

Merged
merged 10 commits into from
Dec 20, 2024

Conversation

DonIsaac
Copy link
Contributor

@DonIsaac DonIsaac commented Dec 18, 2024

extractedSplitNewLines uses a regex with a positive lookahead to spit strings by newlines. Benchmarks show that this causes inspect to be ~2-3x slower than node when this codepath is hit. This PR adds a fast path when the object being split is known to be a string.

We cannot run this fast path in all cases since splitting via a regexp respects Symbol.split.

@robobun
Copy link

robobun commented Dec 18, 2024

Updated 3:42 PM PT - Dec 19th, 2024

✅ Your commit 65e9b70 has passed in #8263! 🎉


🧪   try this PR locally:

bunx bun-pr 15838

@DonIsaac DonIsaac added the performance An issue with performance label Dec 19, 2024
@DonIsaac DonIsaac requested a review from paperclover December 19, 2024 01:18
Copy link
Member

@paperclover paperclover left a comment

Choose a reason for hiding this comment

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

i think on my end, i should make the bindings generator pass WTFString instead of bun.String, since it will always be of the WTFStringImpl tag. this is significant because the input to this function is always latin1 or utf16, and never utf8. checking for the utf8 encoding in this function shouldn't be done since it wont ever be present.

src/bun.js/node/node_util_binding.zig Outdated Show resolved Hide resolved
src/bun.js/node/node_util_binding.zig Show resolved Hide resolved
src/bun.js/node/node_util_binding.zig Outdated Show resolved Hide resolved
@DonIsaac DonIsaac requested review from paperclover and a team December 19, 2024 04:16
@DonIsaac DonIsaac enabled auto-merge December 19, 2024 19:02
@DonIsaac DonIsaac added this pull request to the merge queue Dec 19, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Dec 19, 2024
@DonIsaac DonIsaac added this pull request to the merge queue Dec 19, 2024
@DonIsaac DonIsaac removed this pull request from the merge queue due to a manual request Dec 19, 2024
@DonIsaac DonIsaac enabled auto-merge December 19, 2024 21:31
@DonIsaac DonIsaac added this pull request to the merge queue Dec 19, 2024
github-merge-queue bot pushed a commit that referenced this pull request Dec 19, 2024
Co-authored-by: Don Isaac <don@bun.sh>
Co-authored-by: DonIsaac <DonIsaac@users.noreply.github.com>
auto-merge was automatically disabled December 19, 2024 23:58

Pull Request is not mergeable

Merged via the queue into main with commit 960b2b2 Dec 20, 2024
67 checks passed
@DonIsaac DonIsaac deleted the don/util/perf/extracted-split-fast-path branch December 20, 2024 01:07
probably-neb pushed a commit to probably-neb/bun that referenced this pull request Jan 7, 2025
Co-authored-by: Don Isaac <don@bun.sh>
Co-authored-by: DonIsaac <DonIsaac@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
node:util performance An issue with performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants