GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — buffer: optimize byteLength for short strings by ronag · Pull Request #54345 · nodejs/node

Conversation

@ronag

@ronag ronag commented Aug 12, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

M2

buffers/buffer-bytelength-string.js n=4000000 repeat=1 encoding='utf8' type='four_bytes'                  5.08 %      ±27.63% ±43.80%  ±75.96%
buffers/buffer-bytelength-string.js n=4000000 repeat=1 encoding='utf8' type='one_byte'                    9.92 %      ±43.25% ±67.27% ±112.98%
buffers/buffer-bytelength-string.js n=4000000 repeat=1 encoding='utf8' type='three_bytes'                12.47 %      ±31.61% ±49.92%  ±86.02%
buffers/buffer-bytelength-string.js n=4000000 repeat=1 encoding='utf8' type='two_bytes'                  34.02 %      ±52.38% ±76.67% ±116.30%
buffers/buffer-bytelength-string.js n=4000000 repeat=2 encoding='utf8' type='four_bytes'          **     82.80 %      ±52.56% ±76.65% ±115.59%
buffers/buffer-bytelength-string.js n=4000000 repeat=2 encoding='utf8' type='one_byte'                    5.31 %      ±48.31% ±70.43% ±106.17%
buffers/buffer-bytelength-string.js n=4000000 repeat=2 encoding='utf8' type='three_bytes'         **     60.17 %      ±34.55% ±50.81%  ±77.68%
buffers/buffer-bytelength-string.js n=4000000 repeat=2 encoding='utf8' type='two_bytes'           **     68.52 %      ±32.32% ±48.53%  ±76.85%

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with rocket emoji
All reactions
  • 🚀 1 reaction
@ronag

ronag commented Aug 12, 2024

Copy link
Copy Markdown
Member Author

@lemire
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag
ronag requested a review from lemire August 12, 2024 21:10
@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Aug 12, 2024
@ronag
ronag force-pushed the bytelength-int64 branch 5 times, most recently from 55227cf to 6506c2a Compare August 12, 2024 21:21
@ronag

ronag commented Aug 12, 2024

Copy link
Copy Markdown
Member Author

@lemire couldn't utf8_length_from_latin1 do this?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag

ronag commented Aug 12, 2024

Copy link
Copy Markdown
Member Author

https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1602/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@codecov

codecov Bot commented Aug 12, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.09%. Comparing base (298ff4f) to head (e204e9e).
Report is 436 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #54345      +/-   ##
==========================================
- Coverage   87.09%   87.09%   -0.01%     
==========================================
  Files         647      648       +1     
  Lines      181816   182234     +418     
  Branches    34884    34964      +80     
==========================================
+ Hits       158360   158717     +357     
- Misses      16764    16791      +27     
- Partials     6692     6726      +34
Files with missing lines Coverage Δ
src/node_buffer.cc 75.55% <100.00%> (+0.36%) ⬆️

... and 53 files with indirect coverage changes
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lemire lemire mentioned this pull request Aug 13, 2024
optimize utf8_length_from_latin1 for short strings simdutf/simdutf#499
Merged
@lemire

lemire commented Aug 13, 2024

Copy link
Copy Markdown
Member

@ronag

couldn't utf8_length_from_latin1 do this?

Fair question.

Please see simdutf/simdutf#499
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lemire
lemire reviewed Aug 13, 2024
View reviewed changes
Comment thread src/node_buffer.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
lemire
lemire approved these changes Aug 13, 2024
View reviewed changes
@lemire

lemire commented Aug 13, 2024

Copy link
Copy Markdown
Member

Long term, it could pay to make simdutf faster in these cases, because we have the runtime dispatching in place. But this PR, as is, should be fine.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
anonrig
anonrig approved these changes Aug 13, 2024
View reviewed changes
@ronag
ronag force-pushed the bytelength-int64 branch from 6506c2a to a608ae1 Compare August 13, 2024 06:14
@ronag
ronag force-pushed the bytelength-int64 branch from a608ae1 to 110e88b Compare August 13, 2024 06:17
@ronag
ronag force-pushed the bytelength-int64 branch from 110e88b to b97e1f1 Compare August 13, 2024 06:20
@ronag
ronag force-pushed the bytelength-int64 branch from b97e1f1 to 0fedfdb Compare August 13, 2024 06:21
@ronag
ronag force-pushed the bytelength-int64 branch from 0fedfdb to 1d66f25 Compare August 13, 2024 06:23
@ronag
ronag force-pushed the bytelength-int64 branch from 1d66f25 to 6e3ab46 Compare August 13, 2024 06:23
@ronag

ronag commented Aug 13, 2024

Copy link
Copy Markdown
Member Author

https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1604/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
anonrig
anonrig approved these changes Aug 13, 2024
View reviewed changes
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 13, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 13, 2024
@nodejs-github-bot

nodejs-github-bot commented Aug 13, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/61078/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag
ronag force-pushed the bytelength-int64 branch from a9325a3 to dc6d0a4 Compare August 13, 2024 13:18
@ronag
ronag requested a review from benjamingr August 13, 2024 13:19
lemire
lemire approved these changes Aug 13, 2024
View reviewed changes
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 13, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 13, 2024
@nodejs-github-bot

nodejs-github-bot commented Aug 13, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/61079/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
BridgeAR
BridgeAR reviewed Aug 13, 2024
View reviewed changes
Comment thread src/node_buffer.cc Outdated
Comment on lines 769 to 779

@BridgeAR BridgeAR Aug 13, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Is this really improving the performance? I am surprised that unrolling the loop is beneficial compared to just the lines right afterwards. I would expect the compiler to optimize things like that.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ronag ronag Aug 13, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I just took from simdutf @lemire wdyt?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot

nodejs-github-bot commented Aug 13, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/61082/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Aug 14, 2024 •
edited by ronag
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

✅ CI: https://ci.nodejs.org/job/node-test-pull-request/61094/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag ronag added the commit-queue PRs queued for automated landing through the Commit Queue. label Aug 14, 2024
@ronag
buffer: optimize byteLength for short strings
e204e9e 
PR-URL: nodejs#54345
@ronag
ronag force-pushed the bytelength-int64 branch from dc6d0a4 to e204e9e Compare August 14, 2024 08:37
@nodejs-github-bot

nodejs-github-bot commented Aug 14, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/61098/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Aug 14, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/61100/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Aug 14, 2024 •
edited by ronag
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

✅ CI: https://ci.nodejs.org/job/node-test-pull-request/61102/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag
ronag requested a review from mcollina August 14, 2024 14:16
mcollina
mcollina approved these changes Aug 14, 2024
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Aug 14, 2024
@nodejs-github-bot
nodejs-github-bot merged commit 75d25bc into nodejs:main Aug 14, 2024
@nodejs-github-bot

nodejs-github-bot commented Aug 14, 2024

Copy link
Copy Markdown
Collaborator

Landed in 75d25bc
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
RafaelGSS pushed a commit that referenced this pull request Aug 19, 2024
@ronag @RafaelGSS
buffer: optimize byteLength for short strings
15c5f87 
PR-URL: #54345
Reviewed-By: Daniel Lemire <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Aug 19, 2024
v22.7.0 proposal #54452
Merged
RafaelGSS pushed a commit that referenced this pull request Aug 21, 2024
@ronag @RafaelGSS
buffer: optimize byteLength for short strings
7b641bc 
PR-URL: #54345
Reviewed-By: Daniel Lemire <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@targos targos added the dont-land-on-v20.x label Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BridgeAR BridgeAR BridgeAR left review comments
@mcollina mcollina mcollina approved these changes
@lemire lemire lemire approved these changes
@anonrig anonrig anonrig approved these changes
@benjamingr benjamingr Awaiting requested review from benjamingr

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@ronag @lemire @targos @nodejs-github-bot @mcollina @anonrig @BridgeAR
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.