undici icon indicating copy to clipboard operation
undici copied to clipboard

websocket: improve frame parsing

Open tsctx opened this issue 1 year ago • 7 comments

Adjust to avoid creating a temporary buffer

tsctx avatar Aug 12 '24 13:08 tsctx

I'd prefer if there was a benchmark to measure the change.

KhafraDev avatar Aug 12 '24 14:08 KhafraDev

As an example, the 256Kib receive benchmark is 1.2x faster.

tsctx avatar Aug 12 '24 21:08 tsctx

Benchmarking websockets is hard and there is no benchmark to verify.

KhafraDev avatar Aug 13 '24 01:08 KhafraDev

Benchmarking websockets is hard and there is no benchmark to verify.

It is a simple benchmark using console.time. For a correct benchmark, it should be a Parser-only benchmark.

tsctx avatar Aug 13 '24 13:08 tsctx

This is a simple benchmark of the current Parser.

  • main
benchmark                  time (avg)             (min … max)       p75       p99      p999
------------------------------------------------------------- -----------------------------
parsing                   239 µs/iter  (29'500 ns … 2'398 µs)    242 µs  1'189 µs  2'202 µs
parsing (arraybuffer)     243 µs/iter  (25'400 ns … 9'173 µs)    227 µs  1'381 µs  5'795 µs
parsing (string)          459 µs/iter     (219 µs … 3'083 µs)    418 µs  1'990 µs  2'757 µs
  • this PR
benchmark                  time (avg)             (min … max)       p75       p99      p999
------------------------------------------------------------- -----------------------------
parsing                   906 ns/iter     (727 ns … 4'198 ns)    916 ns  3'199 ns  4'198 ns
parsing (arraybuffer)     211 µs/iter  (20'800 ns … 3'202 µs)    190 µs  1'239 µs  2'894 µs
parsing (string)          264 µs/iter     (166 µs … 4'089 µs)    231 µs  1'650 µs  3'126 µs
Script
import { bench, run } from "mitata";
import { opcodes } from "../../lib/web/websocket/constants.js";
import { toArrayBuffer, utf8Decode } from "../../lib/web/websocket/util.js";
import { ByteParser } from "../../lib/web/websocket/receiver.js";

function createFrame(opcode, data) {
  const length = data.length;

  let payloadLength = length;
  let offset = 2;

  if (length > 65535) {
    offset += 8;
    payloadLength = 127;
  } else if (length > 125) {
    offset += 2;
    payloadLength = 126;
  }

  const frame = Buffer.allocUnsafeSlow(length + offset);

  frame[0] = 0x80 | opcode;

  frame[1] = payloadLength;

  if (payloadLength === 126) {
    frame.writeUInt16BE(length, 2);
  } else if (payloadLength === 127) {
    frame[2] = frame[3] = 0;
    frame.writeUIntBE(length, 4, 6);
  }

  if (length !== 0) {
    frame.set(data, offset);
  }

  return frame;
}

const requestBody = createFrame(opcodes.BINARY, Buffer.from('a'.repeat(256 * 1024), 'utf8'));

let _resolve, _reject;

const parser = new ByteParser({
  onFail: (reason) => {
    _reject(reason);
  },
  onMessage: (opcode, data) => {
    _resolve(data);
  },
});

bench("parsing", () => {
  return new Promise((resolve, reject) => {
    _resolve = resolve;
    _reject = reject;
    parser.write(requestBody);
  });
});

bench("parsing (arraybuffer)", () => {
  return new Promise((resolve, reject) => {
    _resolve = (data) => resolve(toArrayBuffer(data));
    _reject = reject;
    parser.write(requestBody);
  });
});

bench("parsing (string)", () => {
  return new Promise((resolve, reject) => {
    _resolve = (data) => resolve(utf8Decode(data));
    _reject = reject;
    parser.write(requestBody);
  });
});

await run();

tsctx avatar Aug 13 '24 13:08 tsctx

I'd prefer a benchmark that uses WebSocket, not websocket internals. If #3203 could be completed, and then this PR benchmarked against that, I wouldn't have any complaints.

KhafraDev avatar Aug 14 '24 18:08 KhafraDev

@KhafraDev Would you be able to revisit this? This is the result of the #3203's of benchmarks.

before / after

> $ node ./benchmarks/websocket-benchmark.mjs
(node:11148) [UNDICI-WSS] Warning: WebSocketStream is experimental! Expect it to change at any time.
(Use `node --trace-warnings ...` to show where the warning was created)
undici [binary]: transferred 95.78MiB Bytes/s
undici [string]: transferred 92.98MiB Bytes/s
undici - stream [binary]: transferred 95.93MiB Bytes/s
undici - stream [string]: transferred 97.00MiB Bytes/s
ws [binary]: transferred 125.82MiB Bytes/s
ws [string]: transferred 117.20MiB Bytes/s
> $ node ./benchmarks/websocket-benchmark.mjs
(node:3924) [UNDICI-WSS] Warning: WebSocketStream is experimental! Expect it to change at any time.
(Use `node --trace-warnings ...` to show where the warning was created)
undici [binary]: transferred 113.48MiB Bytes/s
undici [string]: transferred 112.11MiB Bytes/s
undici - stream [binary]: transferred 106.25MiB Bytes/s
undici - stream [string]: transferred 91.41MiB Bytes/s
ws [binary]: transferred 122.96MiB Bytes/s
ws [string]: transferred 114.50MiB Bytes/s

tsctx avatar Oct 15 '24 13:10 tsctx

@KhafraDev ping

mcollina avatar Dec 18 '24 09:12 mcollina