buffer: Default to UTF8 in JS rather than C++ - #4010
Conversation
Sorry, something went wrong.
This will cause a minor speedup as previously with undefined values we would always cast to a string and then proceed to call .toLowerCase()
|
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. |
Sorry, something went wrong.
|
LGTM if @trevnorris is cool with it. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
|
@tomgco That is a known flakey test. No action required on your part :-) |
Sorry, something went wrong.
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>
|
@trevnorris @jasnell @cjihrig how long do you think this should live on master or 5.x before going into lts? |
Sorry, something went wrong.
|
Probably no more than the minimum (1 release?). This is just a performance tweak. |
Sorry, something went wrong.
|
+1 one release then give it a week |
Sorry, something went wrong.
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>
|
as this was included in 5.2.0 I'm going to land this in 4.x-staging this week |
Sorry, something went wrong.
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>
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>
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>
This will cause a minor speedup as previously with
undefined values we would always cast to a string and
then proceed to call .toLowerCase()
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.