buffer: Default to UTF8 in JS rather than C++#4010
buffer: Default to UTF8 in JS rather than C++#4010tomgco wants to merge 1 commit intonodejs:masterfrom
Conversation
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 |
|
LGTM if @trevnorris is cool with it. |
|
LGTM. This also brought to my attention that we have some discrepancy in encoding handling. For example |
|
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? |
|
@tomgco That is a known flakey test. No action required on your part :-) |
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>
|
Thanks! Landed in 93739f4 |
|
@trevnorris @jasnell @cjihrig how long do you think this should live on master or 5.x before going into lts? |
|
Probably no more than the minimum (1 release?). This is just a performance tweak. |
|
+1 one release then give it a week |
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 |
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>
6 participants
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.