revert the highwatermak increase

[mcollina]

In v22, we increased the highWaterMark to 64k. But it has worsen the performance of socket operations on my mac and linux boxes.

Consider the following example:

import stream from 'stream';
import { Readable, Writable, Transform } from 'stream';
import { createServer, connect } from 'net';

// const num = 128 * 1024
// stream.setDefaultHighWaterMark(false, num);

let count = 0

const readableStream = new Readable({
  read(size) {
    this.push('Hello');
    if (count++ === 10000000) {
      this.push(null);
    }
  }
});

const server = createServer((socket) => {
  readableStream.pipe(socket);
}).listen(8000, () => {
  console.time('benchmark')
  const socket = connect(8000);

  let chunks = 0
  const writableStream = new Writable({
    write(chunk, encoding, callback) {
      console.log(chunk.length)
      chunks++
      callback();
    }
  });

  socket.pipe(writableStream);

  writableStream.on('finish', () => {
    console.log('Chunks:', chunks);
    console.timeEnd('benchmark')
    console.log('All data has been written');
    socket.destroy();
    server.close()
  });
})

Node v22 on Mac:

Chunks: 997
benchmark: 8.404s
All data has been written

Node v20 on Mac:

Chunks: 782
benchmark: 8.147s
All data has been written

Node v22 on Linux:

Chunks: 774
benchmark: 14.953s
All data has been written

Node v20 on Linux:

Chunks: 764
benchmark: 13.810s
All data has been written

[mcollina]

cc @ronag @nodejs/tsc

[mcollina]

Alternatively, we might want to set it to 16KB for sockets and keep the default higher for the in-memory processing.

[rluvaton]

Does changing the default high water mark fixes it? because as you know we have other changes in that release like v8 updates

[mcollina]

Does changing the default high water mark fixes it? because as you know we have other changes in that release like v8 updates

Yes.

[lpinca]

I'm fine with reverting as I don't like the increased memory usage, but did you try with two different processes? One for the server and one for the client? Also, console.time('benchmark') should be inside the 'connect' event.

[mcollina]

Note that there isn't any memory increases in this example, as the chunks arrives with length 65535 even on Node 20.

My theory is that the outgoing buffer is smaller than 65535, causing some data to be resent over libuv, effectively slowing things down.

[ronag]

I think we should fix this in the specific case not the general. If someone sets setDefaultHighwaterMark they should not suffer for specific cases.

[lpinca]

Note that there isn't any memory increases in this example, as the chunks arrives with length 65535 even on Node 20

Yes, everything is written synchronously and the consumer is faster than the producer. There is a single socket and not much is buffered. Chunks of 65535 bytes are read from a socket even on older versions of Node.js. That is why I am a bit skeptical about this regression, at least in the form of the benchmark in the issue description.

[mcollina]

The regression is that actually we are not reading chunks of 65535 from the other side, but smaller.

[ronag]

Is the regression in the reading or the writing side?

[mcollina]

Writing side, setting the following "fixes" it:

  socket._writableState.highWaterMark = 16 * 1024

[ronag]

Writing side, setting the following "fixes" it:

  socket._writableState.highWaterMark = 16 * 1024

I propose we do that then with a comment.

[ronag]

Would still be interesting to understand why it's slower.

[ronag]

@mcollina: Could you try with #52815?

[mcollina]

Unfortunately it's not enough, the issue is also somewhat on reading side. The same benchmark completes in roughly 7.7s with 65535 (with the highwaterMark overridden in the socket) set on vs 7.3 with a default of 16384.

I'll try your patch.

[lpinca]

FWIW I can't reproduce the issue:

// server.mjs
import stream from 'stream';
import net from 'net';

// stream.setDefaultHighWaterMark(false, num);

let count = 0;

const readableStream = new stream.Readable({
  read() {
    this.push('Hello');
    if (count++ === 10000000) {
      this.push(null);
    }
  }
});

const server = net.createServer(function (socket) {
  readableStream.pipe(socket);
})

server.listen(8000, function () {
  console.log('Server listening on port 8000');
});

// client.mjs
import stream from 'stream';
import net from 'net';

// stream.setDefaultHighWaterMark(false, num);

let chunks = 0;
const writableStream = new stream.Writable({
  write(chunk, encoding, callback) {
    // console.log(chunk.length);
    chunks++;
    callback();
  }
});

writableStream.on('finish', () => {
  console.log('Chunks:', chunks);
  console.timeEnd('benchmark');
  console.log('All data has been written');
  socket.end();
});

const socket = net.createConnection(8000);

socket.on('connect', function () {
  console.time('benchmark');
});

socket.pipe(writableStream);

# Node.js 20.12.2
$ node client.mjs
Chunks: 2407765
benchmark: 10.010s
All data has been written

# Node.js 22.1.0
$ node client.mjs
Chunks: 2357301
benchmark: 10.623s
All data has been written

[RomainLanz]

On Windows, here is the result I have with your code @lpinca:

# Node.js 20.12.2
$ node client.mjs
Chunks: 8949801
benchmark: 51.945s
All data has been written

# Node.js 22.1.0
$ node client.mjs
Chunks: 9068320
benchmark: 51.602s
All data has been written

[mcollina]

Very interestingly, then this seems to affect only the case of client and server running on the same process.

I think we can close this then.

[ronag]

@mcollina did my patch do anything?

[mcollina]

@mcollina did my patch do anything?

Nope

---

I'm going to close this. Sorry for the false alarm, however it was good to investigate.

[rluvaton]

Very interestingly, then this seems to affect only the case of client and server running on the same process.

I think we can close this then.

Which is the case for some tests but still worth it IMO

[mcollina]

should we reopen?

[RafaelGSS]

I think we should have a more comprehensive benchmark if we want to revert it. Also, note if we revert, it will be flagged as semver-major anyway.