buffer: add swap16() and swap32() methods by jasnell · Pull Request #5724 · nodejs/node · GitHub

Pull Request check-list

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to [CONTRIBUTING.md][0]?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

Affected core subsystem(s)

buffer

Description of change

Adds Buffer.prototype.swapShort() Buffer.prototype.swap16() and
Buffer.prototype.swapLong() Buffer.prototype.swap32() methods that
mutate the Buffer instance in-place by swapping the 16-bit and 32-bit
byte-order.

Example:

const buf = Buffer([0x1, 0x2, 0x3, 0x4]);
buf.swap16();
console.log(buf);
  // prints Buffer(0x2, 0x1, 0x4, 0x3);

buf.swap32();
console.log(buf);
  // prints Buffer(0x3, 0x4, 0x1, 0x2);

Was looking at a number of use cases recently around reading in UTF16BE data and converting that to UTF8 and realized that we really didn't expose an efficient mechanism for doing a byte-swap in Buffer. This seemed like a useful API addition.

/cc @trevnorris @srl295

I think swap16() and swap32() might be better names.

Just how common are these anyway?

I can live with swap16 and swap32.

The swap16 is particularly useful when dealing with utf16 big-endian encoded buffer content that needs to be converted. The swap32 is less common but included for completeness. There are certain formats (such as TIFF) that can be big-endian or little-endian and it's common when working with those to have to swap the order to meet expectations. I wouldn't say that these are critical by any stretch, but they are useful and building the implementation directly into node_buffer is cheap.

Methods renamed to swap16/swap32

While case differs, generally the names are the same between c++ and js. Would Swap{16,32} work?

@trevnorris ... ok, updated! PTAL

CI: https://ci.nodejs.org/job/node-test-pull-request/1935/

Does this belong in core?

nit: wrap multi-line statements in conditionals with {}, and double indent with multi-line statements. another thing lint should be catching.

@Fishrock123 Asked myself the same thing. I believe it may just fit within the parameters of what functionality belongs in core. For example:

// Read in utf16be file
const file = fs.readFileSync(path).swap16().toString('utf16le');
// Write it back to disk
fs.writeFileSync(path, Buffer(file, 'utf16le').swap16());

Though additional input is welcome.

@jasnell have a nit, but LGTM otherwise. Let's leave this open though for at least a couple days for others to respond.

@Fishrock123 ... it is certainly possible to do in userland but there's a bit of a performance penalty if it's done in pure javascript... I'll post some benchmark numbers in a couple of minutes that compares the performance of swap16/swap32 to equivalent pure javascript code. I went back and forth on this one also but I landed in the same @trevnorris did... that it likely fits just inside that boundary.

Benchmarks ... the swap(16|21) is the impl in node_buffer, while the htons/htonl is a pure javascript impl... note that at the low end of the range the javascript impl's blow away the native impls, but that reverses once the buffer size grows. Given that, I may change this up a bit to do the swap in javascript if the length is below a certain threshold.

4     - swap16/htons - 15881312.97675 / 56764416.88469 - htons
64    - swap16/htons - 11189303.45557 / 18467670.86474 - htons
1024  - swap16/htons - 2019124.50116  / 1546034.86159  - swap16
2056  - swap16/htons - 1070521.29584  / 732798.39598   - swap16
4096  - swap16/htons - 591436.23889   / 383412.43533   - swap16

4     - swap32/htonl - 15827403.80753 / 78215981.07361 - htonl
64    - swap32/htonl - 11970720.57513 / 20776537.63779 - htonl
1024  - swap32/htonl - 2255585.06207  / 1886075.54128  - swap32
2056  - swap32/htonl - 1242583.71829  / 1036100.06795  - swap32
4096  - swap32/htonl - 621999.48227   / 511625.91432   - swap32

Maybe avoid using apply inside the bench start/end?

In buffer-iterate.js it just calls off the instance...

const name = conf.method;
//...
for (i = 0; i < n; i += 1)
  method[name](buf);

@trevnorris @williamkapke ... nits addressed.

I also updated the implementation such that if Buffer length is below a certain threshold, a pure javascript version of the swap will be used rather than dropping down to the native layer. Based on the benchmarks, when the Buffer is below a certain size, it's far more efficient to swap in js. We may be able to further tweak the threshold but the current limits appear to be within the margin of error.

you already have len above... you might as well use it here too! :D
(Same for swap32)

doh! missed that one. good catch :-)

If we're doing this, what about (also) including a more generic buffer.swap(a,b) that swap16 and swap32 could leverage? Maybe that would tank perf on the native side?

I think it makes the JS side cleaner...

Buffer.prototype.swap = function swap(a, b) {
  var x = this[a];
  this[a] = this[b];
  this[b] = x;
  return this;
};

Buffer.prototype.swap32 = function swap32() {
  for (var i = 0; i < buf.length; i += 2) {
    this.swap(i++, i + 1);
    this.swap(i++, i + 1);
  }
  return this;
};

Buffer.prototype.swap16 = function swap16() {
  for (var i = 0; i < buf.length; i++) {
    this.swap(i, ++i);
  }
  return this;
};

const buf = Buffer([0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0x8]);
console.log(buf.swap(1,2));
//or
console.log(buf.swap16());
//or
console.log(buf.swap32());

Having a swap function makes sense but I wouldn't expose it as part of the Buffer API.

Nits addressed!

heh- that's how I originally wrote it and then decided to put swap on the prototype ;)

LGTM!

mind throwing these (the for loop) in their own function? when profiling it's easier to look for execution of a named function that only contains the code actually being profiled.

will do

@trevnorris ... done! also squashed the commits and rebased to pick up the buffer api changes also

Missing closing period.

What are the semantics when the size of the buffer is not a multiple of 2 / 4?

EDIT: found the answer by looking at the test-case.

It's also covered in the documentation addition. A RangeError is thrown.

Why are we doing this instead of attaching the native implementation to binding.swap*()?

Just an artifact of how it was originally written. You're right, attaching directly to binding.swap*() is better.

Nice tests. Left two comments. Other than that LGTM.

@trevnorris .. thanks .. updated to address those nits!

@jasnell Thanks much. LGTM

Incredible nitpicking, but nonetheless I would opt for one of these:

Buffer size must be a multiple of 16 bits

or:

Buffer length must be a multiple of 2

I hope you understand the nuance difference I'm getting at.

Will change to Buffer size must be a multiple of 16 bits when I land.

Adds Buffer.prototype.swap16() and Buffer.prototype.swap32() methods that mutate the Buffer instance in-place by swapping the 16-bit and 32-bit byte-order. Example: ```js const buf = Buffer([0x1, 0x2, 0x3, 0x4]); buf.swap16(); console.log(buf); // prints Buffer(0x2, 0x1, 0x4, 0x3); buf.swap32(); console.log(buf); // prints Buffer(0x3, 0x4, 0x1, 0x2); ``` PR-URL: #5724 Reviewed-By: Trevor Norris <trev.norris@gmail.com>

Landed in 7d73e60. Thanks all!

Adds Buffer.prototype.swap16() and Buffer.prototype.swap32() methods that mutate the Buffer instance in-place by swapping the 16-bit and 32-bit byte-order. Example: ```js const buf = Buffer([0x1, 0x2, 0x3, 0x4]); buf.swap16(); console.log(buf); // prints Buffer(0x2, 0x1, 0x4, 0x3); buf.swap32(); console.log(buf); // prints Buffer(0x3, 0x4, 0x1, 0x2); ``` PR-URL: #5724 Reviewed-By: Trevor Norris <trev.norris@gmail.com>

Notable changes: * buffer: * make byteLength work with ArrayBuffer & DataView (Jackson Tian) [#5255](#5255) * backport --zero-fill-buffers command line option (James M Snell) [#5744](#5744) * backport new buffer constructor APIs (James M Snell) [#5763](#5763) * add swap16() and swap32() methods (James M Snell) [#5724](#5724) * fs: add the fs.mkdtemp() function. (Florian MARGAINE) [#5333](#5333) * net: emit host in lookup event (HUANG Wei) [#5598](#5598) * node: --no-browser-globals configure flag (Fedor Indutny) [#5853](#5853) * npm: Upgrade to v3.8.3. (Forrest L Norvell) * repl: support standalone blocks (Prince J Wesley) [#5581](#5581) * src: override v8 thread defaults using cli options (Tom Gallacher) [#4344](#4344)

Notable changes: * buffer: * make byteLength work with ArrayBuffer & DataView (Jackson Tian) [#5255](#5255) * backport --zero-fill-buffers command line option (James M Snell) [#5744](#5744) * backport new buffer constructor APIs (James M Snell) [#5763](#5763) * add swap16() and swap32() methods (James M Snell) [#5724](#5724) * fs: add the fs.mkdtemp() function. (Florian MARGAINE) [#5333](#5333) * net: emit host in lookup event (HUANG Wei) [#5598](#5598) * node: --no-browser-globals configure flag (Fedor Indutny) [#5853](#5853) * npm: Upgrade to v3.8.3. Fixes a security flaw in the use of authentication tokens in HTTP requests that would allow an attacker to set up a server that could collect tokens from users of the command-line interface. Authentication tokens have previously been sent with every request made by the CLI for logged-in users, regardless of the destination of the request. This update fixes this by only including those tokens for requests made against the registry or registries used for the current install. (Forrest L Norvell) [npm#6](npm#6) * repl: support standalone blocks (Prince J Wesley) [#5581](#5581) * src: override v8 thread defaults using cli options (Tom Gallacher) [#4344](#4344)

Notable changes: * buffer: * make byteLength work with ArrayBuffer & DataView (Jackson Tian) [#5255](#5255) * backport --zero-fill-buffers command line option (James M Snell) [#5744](#5744) * backport new buffer constructor APIs (James M Snell) [#5763](#5763) * add swap16() and swap32() methods (James M Snell) [#5724](#5724) * fs: add the fs.mkdtemp() function. (Florian MARGAINE) [#5333](#5333) * net: emit host in lookup event (HUANG Wei) [#5598](#5598) * node: --no-browser-globals configure flag (Fedor Indutny) [#5853](#5853) * npm: Upgrade to v3.8.3. Fixes a security flaw in the use of authentication tokens in HTTP requests that would allow an attacker to set up a server that could collect tokens from users of the command-line interface. Authentication tokens have previously been sent with every request made by the CLI for logged-in users, regardless of the destination of the request. This update fixes this by only including those tokens for requests made against the registry or registries used for the current install. (Forrest L Norvell) [npm#6](npm#6) * repl: support standalone blocks (Prince J Wesley) [#5581](#5581) * src: override v8 thread defaults using cli options (Tom Gallacher) [#4344](#4344) PR-URL: #5970

jasnell added buffer Issues and PRs related to the buffer subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels Mar 15, 2016

jasnell changed the title buffer: add swapShort() and swapLong() methods buffer: add swap16() and swap32() methods Mar 15, 2016

trevnorris reviewed Mar 15, 2016
View reviewed changes

williamkapke reviewed Mar 15, 2016
View reviewed changes

williamkapke reviewed Mar 16, 2016
View reviewed changes

trevnorris reviewed Mar 16, 2016
View reviewed changes

jasnell force-pushed the buffer-byte-swap branch 2 times, most recently from 74769f0 to 26b4ffd Compare March 17, 2016 00:46

ronkorving reviewed Mar 18, 2016
View reviewed changes

jasnell modified the milestone: 6.0.0 Mar 22, 2016

Fishrock123 removed this from the 6.0.0 milestone Mar 22, 2016

trevnorris reviewed Mar 22, 2016
View reviewed changes

Address feedback from @trevnorris

9838523

ronkorving reviewed Mar 23, 2016
View reviewed changes

jasnell closed this Mar 23, 2016

evanlucas mentioned this pull request Mar 31, 2016

v5.10.0 Release Proposal #5970

Closed

+                // dropping down to the native code is faster.
+                const len = this.length;
+                if (len % 2 !== 0)
+                  throw new RangeError('Buffer length must be a multiple of 16-bits');

               binding.setupBufferJS(Buffer.prototype, bindingObj);
+              const swap16n = Buffer.prototype.swap16;
+              const swap32n = Buffer.prototype.swap32;

buffer: add swap16() and swap32() methods by jasnell · Pull Request #5724 · nodejs/node · GitHub

Uh oh!

Uh oh!

buffer: add swap16() and swap32() methods - #5724

buffer: add swap16() and swap32() methods#5724jasnell wants to merge 2 commits intonodejs:masternodejs/node:masterfrom jasnell:buffer-byte-swapjasnell/node:buffer-byte-swapCopy head branch name to clipboard

Conversation

jasnell commented Mar 15, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

Uh oh!

mscdex commented Mar 15, 2016

Uh oh!

jasnell commented Mar 15, 2016

Uh oh!

jasnell commented Mar 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Mar 15, 2016

Uh oh!

jasnell commented Mar 15, 2016

Uh oh!

Fishrock123 commented Mar 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trevnorris commented Mar 15, 2016

Uh oh!

jasnell commented Mar 15, 2016

Uh oh!

jasnell commented Mar 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Mar 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

williamkapke commented Mar 16, 2016

Uh oh!

jasnell commented Mar 16, 2016

Uh oh!

williamkapke commented Mar 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Mar 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ofrobots commented Mar 22, 2016

Uh oh!

jasnell commented Mar 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trevnorris commented Mar 22, 2016

Uh oh!

jasnell commented Mar 22, 2016

Uh oh!

trevnorris commented Mar 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Mar 23, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

buffer: add swap16() and swap32() methods#5724
jasnell wants to merge 2 commits into
nodejs:masterfrom
jasnell:buffer-byte-swap