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

buffer: Default to UTF8 in JS rather than C++ #4010

Closed
wants to merge 1 commit into from

Conversation

tomgco
Copy link
Contributor

@tomgco tomgco commented Nov 24, 2015

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.

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
Copy link
Contributor Author

tomgco commented Nov 24, 2015

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.

@cjihrig
Copy link
Contributor

cjihrig commented Nov 24, 2015

LGTM if @trevnorris is cool with it.

@mscdex mscdex added the buffer Issues and PRs related to the buffer subsystem. label Nov 24, 2015
@trevnorris
Copy link
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/

@tomgco
Copy link
Contributor Author

tomgco commented Nov 25, 2015

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?

@cjihrig
Copy link
Contributor

cjihrig commented Nov 25, 2015

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

cjihrig pushed a commit that referenced this pull request Nov 25, 2015
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 <cjihrig@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@cjihrig
Copy link
Contributor

cjihrig commented Nov 25, 2015

Thanks! Landed in 93739f4

@MylesBorins
Copy link
Contributor

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

@cjihrig
Copy link
Contributor

cjihrig commented Dec 1, 2015

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

@jasnell
Copy link
Member

jasnell commented Dec 1, 2015

+1 one release then give it a week

rvagg pushed a commit that referenced this pull request Dec 5, 2015
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 <cjihrig@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@MylesBorins
Copy link
Contributor

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

@rvagg rvagg mentioned this pull request Dec 17, 2015
MylesBorins pushed a commit that referenced this pull request Dec 29, 2015
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 <cjihrig@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
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 <cjihrig@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 19, 2016
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
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 <cjihrig@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants