src: added big-endian check and support to UTF-16 encode#3410

exinfinitum · 2015-10-16T20:54:30Z

Added for loop to Encode function to support big-endian machines.
Used stack allocation instead of heap allocation to improve speed of endianness correction.
Resolves#3409

jasnell · 2015-10-17T01:39:27Z

@srl295

bnoordhuis · 2015-10-17T14:31:03Z

src/string_bytes.cc

Don't do this. buflen may be on the order of 100s of megabytes.

bnoordhuis · 2015-10-17T14:32:01Z

Instead of copy/pasting the byte-swapping code, it would be much better to move it into a small utility class.

kkoopa · 2015-10-18T14:16:00Z

src/string_bytes.cc

Would it be worth using intrinsics to ensure efficient byteswap instructions?

There's no need. gcc and clang recognize the byte swap just fine.

jasnell · 2015-10-19T19:50:35Z

agree with @bnoordhuis ... there are a few places in the code now where we do this kind of reordering. We should be refactoring it down rather than cutting and pasting it elsewhere.

jasnell · 2015-10-20T19:27:15Z

@srl295 @bnoordhuis ...

@exinfinitum ... just an fyi... we don't get notified automatically when a PR has been updated with a new push. It's helpful if you comment and mention us after the new push so we can get notified and come back to review.

bnoordhuis · 2015-10-20T20:19:45Z

src/string_bytes.cc

No whitespace changes please.

bnoordhuis · 2015-10-20T20:27:37Z

Commit log style nit: please see CONTRIBUTING.md for how it should be written.

exinfinitum · 2015-10-22T14:32:57Z

PR Updated

john-yan · 2015-10-22T16:33:30Z

src/string_bytes.cc

Please check to see if we need to include util.h here?

exinfinitum · 2015-10-22T20:45:25Z

Updated

bnoordhuis · 2015-10-22T21:43:52Z

src/string_bytes.cc

I'd declare an empty std::vector here and .resize() it inside the if block. That avoids the memory allocation on little-endian architectures.

exinfinitum · 2015-10-23T18:38:28Z

Updated

john-yan · 2015-10-23T18:45:15Z

src/util-inl.h

there is no shift operation anymore :)

Versions of Node.js after v0.12 have relocated byte-swapping away from the StringBytes::Encode function, thereby causing a nan test (which accesses this function directly) to fail on big-endian machines. This change re-introduces byte swapping in StringBytes::Encode, done via a call to a function in util-inl. Another change in NodeBuffer::StringSlice was necessary to avoid double byte swapping in big-endian function calls to StringSlice. PR-URL: #3410 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Trevor Norris <[email protected]>

Notable changes: This update to the LTS line includes a number of semver minor changes that have been staged for a number of months. This includes: * deps: backport 9da3ab6 from V8 upstream (Ali Ijaz Sheikh) - #3609 * http: handle errors on idle sockets (José F. Romaniello) - #4482 * src: add BE support to StringBytes::Encode() (Bryon Leung) - #3410 * tls: add `options` argument to createSecurePair (Коренберг Марк) - #2441 There are also quite a large number of semver patch changes including over 20 doc fixes and almost 50 test fixes. Notable semver patch changes include: * deps: upgrade to npm 2.14.18 (Kat Marchán) - #5245 * https: evict cached sessions on error (Fedor Indutny) - #4982 * process: support symbol events (cjihrig) - #4798 * querystring: improve parse() performance (Brian White) - #4675 PR-URL: #5301

In December we announced that we would be doing a minor release in order to get a number of voted on SEMVER-MINOR changes into LTS. Our ability to release this was delayed due to the unforeseen security release v4.3. We are quickly bumping to v4.4 in order to bring you the features that we had committed to releasing. Notable changes: The SEMVER-MINOR changes include: * **deps**: - An update to v8 that introduces a new flag --perf_basic_prof_only_functions (Ali Ijaz Sheikh) #3609 * **http**: - A new feature in http(s) agent that catches errors on *keep alived* connections (José F. Romaniello) #4482 * **src**: - Better support for Big-Endian systems (Bryon Leung) #3410 * **tls**: - A new feature that allows you to pass common SSL options to `tls.createSecurePair` (Коренберг Марк) #2441 Notable semver patch changes include: * **npm** - upgrade to npm 2.14.19 (Kat Marchán) #5335 * **https**: - A potential fix for #3692) HTTP/HTTPS client requests throwing EPROTO (Fedor Indutny) #4982 * **process**: - Add support for symbols in event emitters. Symbols didn't exist when it was written ¯\_(ツ)_/¯ (cjihrig) #4798 * **querystring**: - querystring.parse() is now 13-22% faster! (Brian White) #4675 PR-URL: #5301

In December we announced that we would be doing a minor release in order to get a number of voted on SEMVER-MINOR changes into LTS. Our ability to release this was delayed due to the unforeseen security release v4.3. We are quickly bumping to v4.4 in order to bring you the features that we had committed to releasing. This release also includes security updates to openssl. More information can be found [on nodejs.org](https://nodejs.org/en/blog/vulnerability/openssl-march-2016/) This release also includes over 70 fixes to our docs and over 50 fixes to tests. The SEMVER-MINOR changes include: * deps: - An update to v8 that introduces a new flag --perf_basic_prof_only_functions (Ali Ijaz Sheikh) #3609 * http: - A new feature in http(s) agent that catches errors on *keep alived* connections (José F. Romaniello) #4482 * src: - Better support for Big-Endian systems (Bryon Leung) #3410 * tls: - A new feature that allows you to pass common SSL options to `tls.createSecurePair` (Коренберг Марк) #2441 * tools - a new flag `--prof-process` which will execute the tick processor on the provided isolate files (Matt Loring) #4021 Notable semver patch changes include: * buld: - Support python path that includes spaces. This should be of particular interest to our Windows users who may have python living in `c:/Program Files` (Felix Becker) #4841 * https: - A potential fix for #3692 HTTP/HTTPS client requests throwing EPROTO (Fedor Indutny) #4982 * installer: - More readable profiling information from isolate tick logs (Matt Loring) #3032 * *npm: - upgrade to npm 2.14.20 (Kat Marchán) #5510 * *openssl: - upgrade openssl to 1.0.2g (Ben Noordhuis) #5507 * process: - Add support for symbols in event emitters. Symbols didn't exist when it was written ¯\_(ツ)_/¯ (cjihrig) #4798 * querystring: - querystring.parse() is now 13-22% faster! (Brian White) #4675 * streams: - performance improvements for moving small buffers that shows a 5% throughput gain. IoT projects have been seen to be as much as 10% faster with this change! (Matteo Collina) #4354 * tools: - eslint has been updated to version 2.1.0 (Rich Trott) #5214 PR-URL: #5301

In December we announced that we would be doing a minor release in order to get a number of voted on SEMVER-MINOR changes into LTS. Our ability to release this was delayed due to the unforeseen security release v4.3. We are quickly bumping to v4.4 in order to bring you the features that we had committed to releasing. This release also includes security updates to openssl. More information can be found [on nodejs.org](https://nodejs.org/en/blog/vulnerability/openssl-march-2016/) This release also includes over 70 fixes to our docs and over 50 fixes to tests. The SEMVER-MINOR changes include: * deps: - An update to v8 that introduces a new flag --perf_basic_prof_only_functions (Ali Ijaz Sheikh) nodejs#3609 * http: - A new feature in http(s) agent that catches errors on *keep alived* connections (José F. Romaniello) nodejs#4482 * src: - Better support for Big-Endian systems (Bryon Leung) nodejs#3410 * tls: - A new feature that allows you to pass common SSL options to `tls.createSecurePair` (Коренберг Марк) nodejs#2441 * tools - a new flag `--prof-process` which will execute the tick processor on the provided isolate files (Matt Loring) nodejs#4021 Notable semver patch changes include: * buld: - Support python path that includes spaces. This should be of particular interest to our Windows users who may have python living in `c:/Program Files` (Felix Becker) nodejs#4841 * https: - A potential fix for nodejs#3692 HTTP/HTTPS client requests throwing EPROTO (Fedor Indutny) nodejs#4982 * installer: - More readable profiling information from isolate tick logs (Matt Loring) nodejs#3032 * *npm: - upgrade to npm 2.14.20 (Kat Marchán) nodejs#5510 * *openssl: - upgrade openssl to 1.0.2g (Ben Noordhuis) nodejs#5507 * process: - Add support for symbols in event emitters. Symbols didn't exist when it was written ¯\_(ツ)_/¯ (cjihrig) nodejs#4798 * querystring: - querystring.parse() is now 13-22% faster! (Brian White) nodejs#4675 * streams: - performance improvements for moving small buffers that shows a 5% throughput gain. IoT projects have been seen to be as much as 10% faster with this change! (Matteo Collina) nodejs#4354 * tools: - eslint has been updated to version 2.1.0 (Rich Trott) nodejs#5214 PR-URL: nodejs#5301

Versions of Node.js after v0.12 have relocated byte-swapping away from the StringBytes::Encode function, thereby causing a nan test (which accesses this function directly) to fail on big-endian machines. This change re-introduces byte swapping in StringBytes::Encode, done via a call to a function in util-inl. Another change in NodeBuffer::StringSlice was necessary to avoid double byte swapping in big-endian function calls to StringSlice. PR-URL: #3410 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Trevor Norris <[email protected]>

In December we announced that we would be doing a minor release in order to get a number of voted on SEMVER-MINOR changes into LTS. Our ability to release this was delayed due to the unforeseen security release v4.3. We are quickly bumping to v4.4 in order to bring you the features that we had committed to releasing. This release also includes over 70 fixes to our docs and over 50 fixes to tests. The SEMVER-MINOR changes include: * deps: - An update to v8 that introduces a new flag --perf_basic_prof_only_functions (Ali Ijaz Sheikh) #3609 * http: - A new feature in http(s) agent that catches errors on *keep alived* connections (José F. Romaniello) #4482 * src: - Better support for Big-Endian systems (Bryon Leung) #3410 * tls: - A new feature that allows you to pass common SSL options to `tls.createSecurePair` (Коренберг Марк) #2441 * tools - a new flag `--prof-process` which will execute the tick processor on the provided isolate files (Matt Loring) #4021 Notable semver patch changes include: * buld: - Support python path that includes spaces. This should be of particular interest to our Windows users who may have python living in `c:/Program Files` (Felix Becker) #4841 * https: - A potential fix for #3692 HTTP/HTTPS client requests throwing EPROTO (Fedor Indutny) #4982 * installer: - More readable profiling information from isolate tick logs (Matt Loring) #3032 * *npm: - upgrade to npm 2.14.20 (Kat Marchán) #5510 * process: - Add support for symbols in event emitters. Symbols didn't exist when it was written ¯\_(ツ)_/¯ (cjihrig) #4798 * querystring: - querystring.parse() is now 13-22% faster! (Brian White) #4675 * streams: - performance improvements for moving small buffers that shows a 5% throughput gain. IoT projects have been seen to be as much as 10% faster with this change! (Matteo Collina) #4354 * tools: - eslint has been updated to version 2.1.0 (Rich Trott) #5214 PR-URL: #5301

bnoordhuis · 2016-09-23T08:59:15Z

src/string_bytes.cc

-for (size_t i = 0; i < nchars; i++)
- dst[i] = dst[i] << 8 | dst[i] >> 8;
-break;
+SwapBytes(dst, dst, nchars);


It was pointed out in #7645 that this change introduces a bug: the break statement should have been retained because now the bytes are swapped back by the fallback path below.

Thanks for pointing out! Does anyone already develop a fix? if not, I can do the favour.

#7645 should fix this when it lands but I've made a note in case that PR stalls.

Between nodejs#3410 and nodejs#7645, bytes were swapped twice on bigendian platforms if buffer was not two-byte aligned. See comment in nodejs#7645.

Removes use of builtins that are unavailable for older clang. Per benchmarks, only uses builtins on Windows, where speedup is significant. Also adds test for unaligned ucs2 buffer write. Between #3410 and #7645, bytes were swapped twice on bigendian platforms if buffer was not two-byte aligned. See comment in #7645. PR-URL: #7645Fixes: #7618 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

Removes use of builtins that are unavailable for older clang. Per benchmarks, only uses builtins on Windows, where speedup is significant. Also adds test for unaligned ucs2 buffer write. Between #3410 and #7645, bytes were swapped twice on bigendian platforms if buffer was not two-byte aligned. See comment in #7645. PR-URL: #7645Fixes: #7618 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Conflicts: src/node_buffer.cc

exinfinitum mentioned this pull request Oct 16, 2015
Endianness check/bit shifting needed for UCS-2 write operation in versions 4.x+ #3409
Closed

mscdex added the string_decoder Issues and PRs related to the string_decoder subsystem. label Oct 16, 2015

bnoordhuis reviewed Oct 17, 2015
View reviewed changes

src/string_bytes.cc Outdated
Copy link
Member
bnoordhuisOct 17, 2015
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't do this. buflen may be on the order of 100s of megabytes.

kkoopa reviewed Oct 18, 2015
View reviewed changes

brendanashworth added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 19, 2015

exinfinitum force-pushed the community-master branch from 4564f5e to d4380a1Compare October 20, 2015 19:05

bnoordhuis reviewed Oct 20, 2015
View reviewed changes

src/string_bytes.cc Outdated
Copy link
Member
bnoordhuisOct 20, 2015
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No whitespace changes please.

john-yan reviewed Oct 22, 2015
View reviewed changes

src/string_bytes.cc Outdated
Copy link
john-yanOct 22, 2015
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check to see if we need to include util.h here?

bnoordhuis reviewed Oct 22, 2015
View reviewed changes

exinfinitum force-pushed the community-master branch from 9e0881f to cb4874cCompare October 23, 2015 18:37

john-yan reviewed Oct 23, 2015
View reviewed changes

src/util-inl.h Outdated
Copy link
john-yanOct 23, 2015
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is no shift operation anymore :)

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Feb 17, 2016

MylesBorins mentioned this pull request Feb 18, 2016
V4.4.0 proposal #5301
Merged

bnoordhuis mentioned this pull request Sep 23, 2016
src: fix build failure for clang 3.2; consolidate byte swapping code; fix buffer writes for unaligned ucs2 strings #7645
Closed

bnoordhuis reviewed Sep 23, 2016
View reviewed changes

zbjornson added a commit to zbjornson/node that referenced this pull request Sep 30, 2016
test: add test for unaligned ucs2 buffer write
2b69933
Between nodejs#3410 and nodejs#7645, bytes were swapped twice on bigendian platforms if buffer was not two-byte aligned. See comment in nodejs#7645.

Uh oh!

src: added big-endian check and support to UTF-16 encode#3410

src: added big-endian check and support to UTF-16 encode #3410

Uh oh!

Conversation

exinfinitum commented Oct 16, 2015

Uh oh!

jasnell commented Oct 17, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Oct 17, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Oct 19, 2015

Uh oh!

jasnell commented Oct 20, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Oct 20, 2015

Uh oh!

exinfinitum commented Oct 22, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

exinfinitum commented Oct 22, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

exinfinitum commented Oct 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants