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 — draft: implement bufferToString by kiliczsh · Pull Request #45562 · nodejs/node

Conversation

@kiliczsh

@kiliczsh kiliczsh commented Nov 21, 2022

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with eyes emoji
All reactions
  • 👀 1 reaction
@kiliczsh
src: implement bufferToString
7106dc3
@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
src: update bufferToString
ab2c5b4
@kiliczsh kiliczsh changed the title src: implement bufferToString draft: implement bufferToString Nov 21, 2022
anonrig
anonrig reviewed Nov 21, 2022
View reviewed changes
Comment thread src/node_buffer.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/node_buffer.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/node_buffer.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/node_buffer.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/node_buffer.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
kiliczsh added 3 commits November 21, 2022 20:22
@kiliczsh
src: bufferToString argument checks
48798ef
@kiliczsh
src: add string case for bufferToString
698f5e2
@kiliczsh
src: fix errors for bufferToString
0be4196
@mscdex

mscdex commented Nov 21, 2022

Copy link
Copy Markdown
Contributor

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.
addaleax reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

anonrig commented Nov 21, 2022

Copy link
Copy Markdown
Member

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)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kiliczsh
src: add string case
d84a7d4
@mscdex

mscdex commented Nov 21, 2022

Copy link
Copy Markdown
Contributor

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 and addaleax reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
anonrig
anonrig reviewed Nov 21, 2022
View reviewed changes
Comment thread src/node_buffer.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
kiliczsh added 3 commits November 22, 2022 18:01
@kiliczsh
src: fix check error and add string case
bb557ae
@kiliczsh
src: add IsSharedArrayBuffer
778ec39
@kiliczsh
src: fix for string
b8cd296
@kiliczsh
kiliczsh requested a review from anonrig November 24, 2022 16:31
@kiliczsh

kiliczsh commented Jan 16, 2023

Copy link
Copy Markdown
Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

Uh oh!

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

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.

Uh oh!

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

4 participants

@kiliczsh @mscdex @anonrig @nodejs-github-bot
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.