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.

Sign up for GitHub

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

Jump to bottom

draft: implement bufferToString #45562

Closed
wants to merge 9 commits into from
Closed

draft: implement bufferToString #45562

wants to merge 9 commits into from

Conversation

kiliczsh
Copy link

Trying to implement suggested performance improvement in nodejs/performance#16.

@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 Nov 21, 2022
@kiliczsh kiliczsh changed the title src: implement bufferToString draft: implement bufferToString Nov 21, 2022
anonrig
anonrig reviewed Nov 21, 2022
View reviewed changes
src/node_buffer.cc Outdated Show resolved Hide resolved
src/node_buffer.cc Outdated Show resolved Hide resolved
src/node_buffer.cc Outdated Show resolved Hide resolved
src/node_buffer.cc Outdated Show resolved Hide resolved
src/node_buffer.cc Outdated Show resolved Hide resolved
@mscdex
Copy link
Contributor

mscdex commented Nov 21, 2022

Did you see the linked comment in the code comment for this function? I think we shouldn't be spending effort on optimizing APIs that exist solely for compatibility with older code.

@anonrig
Copy link
Member

anonrig commented Nov 21, 2022

Did you see the linked comment in the code comment for this function? I think we shouldn't be spending effort on optimizing APIs that exist solely for compatibility with older code.

@mscdex This change also applies to several other places like _http_client.js and random.js (Referencing nodejs/performance#16)

@mscdex
Copy link
Contributor

mscdex commented Nov 21, 2022

This change also applies to several other places like _http_client.js and random.js

That's fine, I'm referring specifically to btoa() (and atob() for that matter), which is what this PR is currently changing.

anonrig
anonrig reviewed Nov 21, 2022
View reviewed changes
src/node_buffer.cc Outdated Show resolved Hide resolved
@kiliczsh kiliczsh requested a review from anonrig November 24, 2022 16:31
@kiliczsh
Copy link
Author

It is not applicable anymore due to nodejs/performance#16

@kiliczsh kiliczsh closed this Jan 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anonrig anonrig Awaiting requested review from anonrig

Assignees
No one assigned
Labels
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.
4 participants
@kiliczsh @mscdex @anonrig @nodejs-github-bot