websocket: use dataview

[KhafraDev]

We use a dataview here for performance reasons. I should have addressed this in #2106 but I did not.

Refs: nodejs/performance#2

[codecov-commenter]

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.04 ⚠️

Comparison is base (4688da2) 85.89% compared to head (f487e5a) 85.86%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2107      +/-   ##
==========================================
- Coverage   85.89%   85.86%   -0.04%     
==========================================
  Files          76       76              
  Lines        6608     6608              
==========================================
- Hits         5676     5674       -2     
- Misses        932      934       +2     

Impacted Files	Coverage Δ
lib/websocket/frame.js	100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[jawj]

Interesting: I assumed it would be quicker the way I changed it, but I didn't benchmark, so that's on me!

[KhafraDev]

@jawj don't worry about it at all, I should have explained why we used DataViews since you asked in the PR, but I didn't. There aren't currently benchmarks so there's no easy way of benchmarking this either 😆.

[jawj]

@KhafraDev Thanks! Though this got me curious, so I did a bit of benchmarking ...

let b, t0;
const trials = 1_000_000;
const bufferSize = 256;
const maxUint16Plus1 = 65_536;

t0 = performance.now();
for (let i = 0; i < trials; i ++) {
  const value = i % maxUint16Plus1;
  b = Buffer.allocUnsafe(bufferSize);
  new DataView(b.buffer).setUint16(2, value);
}
console.log('buggy DataView', performance.now() - t0, 'ms');

t0 = performance.now();
for (let i = 0; i < trials; i ++) {
  const value = i % maxUint16Plus1;
  b = Buffer.allocUnsafe(bufferSize);
  new DataView(b.buffer, b.byteOffset, b.byteLength).setUint16(2, value)
}
console.log('fixed DataView', performance.now() - t0, 'ms');

t0 = performance.now();
for (let i = 0; i < trials; i ++) {
  const value = i % maxUint16Plus1;
  b = Buffer.allocUnsafe(bufferSize);
  b.writeUInt16BE(value, 2);
}
console.log('writeUInt16BE ', performance.now() - t0, 'ms');

t0 = performance.now();
for (let i = 0; i < trials; i ++) {
  const value = i % maxUint16Plus1;
  b = Buffer.allocUnsafe(bufferSize);
  b[2] = value >>> 8;
  b[3] = value & 0xff;
}
console.log('bit-twiddling ', performance.now() - t0, 'ms');

Which run on Node 18.10 and a 2020 Intel MacBook Pro gives:

buggy DataView 215.71186208724976 ms
fixed DataView 210.01300191879272 ms
writeUInt16BE  107.3242199420929 ms
bit-twiddling  90.78455805778503 ms

DataView is the slowest! To be honest, I don't much mind which approach is used, but I would really like a fix to be released to npm soon, because WebSockets are totally broken until this is done.

[aapoalas]

I think the reason for DataView coming out as slowest is that you're mostly measuring object creation and allocation performance. In this sense it's not much of a surprise that DataView comes out on the bottom since using it requires creating secondary object while the rest get to go with just the one Buffer.

That being said, ... it looks like this is very much what the code itself is doing as well so the performance measurement is apt for comparing what the options are for this PR.

[ronag]

@KhafraDev is this mergable once conflicts are done?

[jawj]

I don't think this should be merged. It's slower, because it has the overhead of creating a new DataView for a single write.