buffer: Default to UTF8 in JS rather than C++#4010
Uh oh!
There was an error while loading. Please reload this page.
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()
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 |
cjihrig commented Nov 24, 2015
LGTM if @trevnorris is cool with it. |
trevnorris commented Nov 24, 2015
LGTM. This also brought to my attention that we have some discrepancy in encoding handling. For example |
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 commented Nov 25, 2015
@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 <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
cjihrig commented Nov 25, 2015
Thanks! Landed in 93739f4 |
MylesBorins commented Dec 1, 2015
@trevnorris@jasnell@cjihrig how long do you think this should live on master or 5.x before going into lts? |
cjihrig commented Dec 1, 2015
Probably no more than the minimum (1 release?). This is just a performance tweak. |
jasnell commented Dec 1, 2015
+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 <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
MylesBorins commented Dec 15, 2015
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 <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
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]>
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]>
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.