@mscdex build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2224/pipeline

CI lite again: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/2226/

I like the feature idea. Regarding the implementation:

• I don’t think passing the buffer along the constructors is a good idea, given the complexity that it introduces to our code. I know it’s an extra call to C++, but adding a single method would probably be much cleaner.
• It’s also kind of odd to stream_buf as a member of StreamBase, and add if/else cases everywhere that handle the cases of having/not having a custom buffer each time – it would, imo, be cleaner to create a class similar to EmitToJSStreamListener (but with custom a OnStreamAlloc method), and attach an instance of it to the stream when calling the C++ method.

I'm open to suggestions (especially specific code changes), this is very much just a proof of concept to show the potential, significant performance gains.

@mscdex Since you’re asking for specific code changes: addaleax@d8b783f is about what I’d have in mind (applied on top of this PR). It also brings the combined diff nicely down from +225/−66 to +139/−45.

@addaleax Merged, thanks.

thoughts @nodejs/collaborators ?

@mscdex The code LGTM here, but I’d also like other people to weigh in more. I assume you don’t want to do unnecessary work in case this doesn’t get merged, but I also guess for a lot of people docs and tests help; so, tl;dr on the API here:

This PR adds an option to net.Socket instances (easy to extend to e.g. HTTP/2) where instead of using the traditional stream.Readable interface, a user can provide a fixed buffer and a callback which is called when data has been read from the socket~~, with no backpressure handling or similar features~~.

Usage looks something like this:

const socket = net.connect({
  host: 'example.com',
  port: 80,
  onread: {
    buffer: Buffer.allocUnsafe(65536),
    callback(bytesRead, buffer) {
      // Do something with the first `bytesRead` bytes of `buffer`.
      // The `buffer` will always be the same object, so all data
      // needs to be handled synchronously here.
    }
  }

There has been some talk of creating a node API layer that is closer to uv, so more efficient code can be written on top of it. Is that where this is going?

cf http://docs.libuv.org/en/v1.x/stream.html#c.uv_read_start

The onread use of a buffer seems to make unnecessary the uv alloc/read callback pair, that seems reasonable to me, it reduces roundtrip time in that only one C++ to js callback is needed. I guess the prices is the need for sync data handling. Maybe that's OK, as long as the data is immediately passed to a protocol parser, or unzipper, or something of the sort, though it wouldn't work for a simple pipe of data to fs.write(), for example (AFAICT).

re: lack of backpressure support, at the uv layer the caller has to explicitly start/stop data flow to exert backpressure. Isn't there a need for a readStart/Stop API as uv has? Or does the handle have that already? There is the Readable.pause() method, but I think the idea here is to bypass the streams API.

re: lack of backpressure support, at the uv layer the caller has to explicitly start/stop data flow to exert backpressure. Isn't there a need for a readStart/Stop API as uv has? Or does the handle have that already? There is the Readable.pause() method, but I think the idea here is to bypass the streams API.

The onread callback can explicitly return false to stop reading from the socket, so there is still backpressure support, it's just at the OS/C++ layer and not at the JS streams layer. So if you return false from the callback, you can call socket.resume() or socket.read() to resume reading from the socket.

I like the idea, we had quite a long discussion about it in denoland/deno#387

ping @nodejs/collaborators ?

@mscdex Unanswered questions in #25436 (comment)

@sam-github I answered how starting and stopping is handled in this PR in #25436 (comment). Were you looking for some other information?

My apologies @mscdex, that comment exactly answers my question, I don't know how I missed it.

Ping @nodejs/streams to review ... in particular, @mcollina, I'd love your thoughts on this.

Are you planning to add this to other areas of core? Specifically, are you planning to add it to TLS?

Probably.

What will happen if pause is called within callback?

With the PR as it currently is, node will not read from the socket.

I think this API could be a bit limiting to what it could be achieved. What I would like to see is more a situation where the same buffer moves between the producer and the consumer, like this:

I don't understand what exactly is expected with the suggested implementation. Is buffer() being called from C++ land for every socket read? Is it only being called once inside the net.Socket() constructor? If it's the former, then what would you suggest for avoiding any performance hit in doing so?

Also, due to the asynchronous nature of destination.write() and the synchronous nature of callback(), when oldBuffer is set the same Buffer will be written to destination and oldBuffer's contents could change multiple times while that write is occurring, which goes against what people expect of streams, unless you meant something else by the example?

I don't understand what exactly is expected with the suggested implementation. Is buffer() being called from C++ land for every socket read? Is it only being called once inside the net.Socket() constructor? If it's the former, then what would you suggest for avoiding any performance hit in doing so?

The key idea is to provide a mechanism to reuse multiple buffers, not just one. Whenever there is a need for a read, C++ can call buffer to get one. We can maintain a list of allocated buffers, that will flow back into the pool after being read. This mechanism is driven by the user. This should answer your second question as well.
I'm suggesting an alternative implementation that could broaden the scope, I'm not referring specifically to this specific implementation.

Whenever there is a need for a read, C++ can call buffer to get one.

That would probably cause a noticeable performance hit, unless you were willing to compromise a bit and instead have the function that the C++ side calls when the socket is read from return the next Buffer to use (by calling buffer()). That would avoid any extra round trips between JS and C++ but the semantics would be different (asking for Buffer for next read vs asking for Buffer for current read) if that matters.

That would probably cause a noticeable performance hit, unless you were willing to compromise a bit and instead have the function that the C++ side calls when the socket is read from return the next Buffer to use (by calling buffer()). That would avoid any extra round trips between JS and C++ but the semantics would be different (asking for Buffer for next read vs asking for Buffer for current read) if that matters.

+1, that's what I meant, sorry for making it complicated.

I think API should allow us to use custom memory allocators and strategies like buffer-reuse-pool. And this api shoud be compatible with dgram which is not stream-based.

@reklatsmasters I think a request like that is probably better implemented at a more global scale (e.g. via Buffer in JS/C++ land) instead of per-module.

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

@mcollina I've added support for a function that can return a Buffer now, let me know if that looks good to you.

@bnoordhuis let me know if these recent changes look good to you as well

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

Thanks @mscdex this is excellent. Will this also work for constructing net servers or is it limited to net clients?

@jorangreef Only clients at the moment.