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: Default to UTF8 in JS rather than C++ by tomgco · Pull Request #4010 · nodejs/node

Conversation

@tomgco

@tomgco tomgco commented Nov 24, 2015

Copy link
Copy Markdown
Contributor

This will cause a minor speedup as previously with
undefined values we would always cast to a string and
then proceed to call .toLowerCase()

screenshot from 2015-11-24 22-37-39
screenshot from 2015-11-24 22-38-04

First flame-graph is before, second is after; calls to ToLowerCase() are highlighted in purple.

Tests seems to pass, but wanted to make sure that this is a safe change.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@tomgco
buffer: Default to UTF8 in JS rather than C++
b664a41 
This will cause a minor speedup as previously with
undefined values we would always cast to a string and
then proceed to call .toLowerCase()
@tomgco

tomgco commented Nov 24, 2015

Copy link
Copy Markdown
Contributor Author

As a note, I haven't normalized the two flamegraphs, so a discrepancy in the % call counts is observed, instead calls to ToLowerCase are shown to disappear in the LazyCompile~byteLength buffer.js:258 stack frames.
All reactions

Sorry, something went wrong.

Uh oh!

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

cjihrig commented Nov 24, 2015

Copy link
Copy Markdown
Contributor

LGTM if @trevnorris is cool with it.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mscdex mscdex added the buffer Issues and PRs related to the buffer subsystem. label Nov 24, 2015
@trevnorris

trevnorris commented Nov 24, 2015

Copy link
Copy Markdown
Contributor

LGTM. This also brought to my attention that we have some discrepancy in encoding handling. For example toString() allows any falsy value to be passed, whereas here it's an explicit check for undefined. Though it appears toString() is the exception to the other checks.

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

Sorry, something went wrong.

Uh oh!

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

tomgco commented Nov 25, 2015

Copy link
Copy Markdown
Contributor Author

It seems that the CI is failing on windows server 2012r2 for: https://ci.nodejs.org/job/node-test-binary-windows/128/RUN_SUBSET=3,VS_VERSION=vs2015,label=win2012r2/tapTestReport/test.tap-12/ is this a CI issue or something I can look into?
All reactions

Sorry, something went wrong.

Uh oh!

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

cjihrig commented Nov 25, 2015

Copy link
Copy Markdown
Contributor

@tomgco That is a known flakey test. No action required on your part :-)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
cjihrig pushed a commit that referenced this pull request Nov 25, 2015
@tomgco @cjihrig
buffer: default to UTF8 in byteLength()
93739f4 
If an undefined encoding is passed to byteLength(),
assume that it is UTF8 immediately. This yields a
small speedup, as it prevents string operations on
the encoding argument.

PR-URL: #4010
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
@cjihrig

cjihrig commented Nov 25, 2015

Copy link
Copy Markdown
Contributor

Thanks! Landed in 93739f4
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig cjihrig closed this Nov 25, 2015
@Fishrock123 Fishrock123 mentioned this pull request Nov 26, 2015
deps: upgrade npm to 3.5.0 #4032
Closed
@jasnell jasnell added the lts-watch-v4.x label Nov 30, 2015
@MylesBorins

MylesBorins commented Dec 1, 2015

Copy link
Copy Markdown
Contributor

@trevnorris @jasnell @cjihrig how long do you think this should live on master or 5.x before going into lts?
All reactions

Sorry, something went wrong.

Uh oh!

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

cjihrig commented Dec 1, 2015

Copy link
Copy Markdown
Contributor

Probably no more than the minimum (1 release?). This is just a performance tweak.
All reactions

Sorry, something went wrong.

Uh oh!

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

jasnell commented Dec 1, 2015

Copy link
Copy Markdown
Member

+1 one release then give it a week
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
rvagg pushed a commit that referenced this pull request Dec 5, 2015
@tomgco @rvagg
buffer: default to UTF8 in byteLength()
ab5b529 
If an undefined encoding is passed to byteLength(),
assume that it is UTF8 immediately. This yields a
small speedup, as it prevents string operations on
the encoding argument.

PR-URL: #4010
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
This was referenced Dec 8, 2015
Release proposal: v5.2.0 #4180
Closed
Release proposal: v5.2.0 (Stable) #4181
Merged
@MylesBorins

MylesBorins commented Dec 15, 2015

Copy link
Copy Markdown
Contributor

as this was included in 5.2.0 I'm going to land this in 4.x-staging this week
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@rvagg rvagg mentioned this pull request Dec 17, 2015
Release 4.2.4 Planning #4321
Closed
MylesBorins pushed a commit that referenced this pull request Dec 29, 2015
@tomgco Myles Borins
buffer: default to UTF8 in byteLength()
30b306c 
If an undefined encoding is passed to byteLength(),
assume that it is UTF8 immediately. This yields a
small speedup, as it prevents string operations on
the encoding argument.

PR-URL: #4010
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
@MylesBorins MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Dec 29, 2015
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
@tomgco Myles Borins
buffer: default to UTF8 in byteLength()
a320045 
If an undefined encoding is passed to byteLength(),
assume that it is UTF8 immediately. This yields a
small speedup, as it prevents string operations on
the encoding argument.

PR-URL: #4010
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 19, 2016
V4.2.5 proposal #4768
Merged
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@tomgco Michael Scovetta
buffer: default to UTF8 in byteLength()
38cb40e 
If an undefined encoding is passed to byteLength(),
assume that it is UTF8 immediately. This yields a
small speedup, as it prevents string operations on
the encoding argument.

PR-URL: nodejs#4010
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
@FireBurn FireBurn mentioned this pull request May 7, 2024
Compile failure in simdjson using GCC 14 or clang 18.1 (Node 20.12.1 - 22.1.0) #52876
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

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.

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.

6 participants

@tomgco @cjihrig @trevnorris @MylesBorins @jasnell @mscdex
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.