buffer: add swap16() and swap32() methods

[jasnell]

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:

constbuf=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

[mscdex]

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

Just how common are these anyway?

[jasnell]

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.

[jasnell]

Methods renamed to swap16/swap32

[trevnorris]

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

[jasnell]

@trevnorris ... ok, updated! PTAL

[jasnell]

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

[Fishrock123]

Does this belong in core?

[trevnorris]

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

[trevnorris]

@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 fileconstfile=fs.readFileSync(path).swap16().toString('utf16le');// Write it back to diskfs.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.

[jasnell]

@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.

[jasnell]

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 

[williamkapke]

Maybe avoid using apply inside the bench start/end?

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

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

[jasnell]

@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.

[williamkapke]

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

[jasnell]

doh! missed that one. good catch :-)

[williamkapke]

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=functionswap(a,b){varx=this[a];this[a]=this[b];this[b]=x;returnthis;};Buffer.prototype.swap32=functionswap32(){for(vari=0;i<buf.length;i+=2){this.swap(i++,i+1);this.swap(i++,i+1);}returnthis;};Buffer.prototype.swap16=functionswap16(){for(vari=0;i<buf.length;i++){this.swap(i,++i);}returnthis;};constbuf=Buffer([0x1,0x2,0x3,0x4,0x5,0x6,0x7,0x8]);console.log(buf.swap(1,2));//orconsole.log(buf.swap16());//orconsole.log(buf.swap32());

[jasnell]

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

Nits addressed!

[williamkapke]

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

LGTM!

[trevnorris]

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.

[jasnell]

will do

[jasnell]

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

[ronkorving]

Missing closing period.

[ofrobots]

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.

[jasnell]

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

[trevnorris]

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

[jasnell]

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

[trevnorris]

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

[jasnell]

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

[trevnorris]

@jasnell Thanks much. LGTM

[ronkorving]

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.

[jasnell]

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

[jasnell]

Landed in 7d73e60. Thanks all!