Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed size buffer for QUICStream instead of object stream #5

Closed
4 tasks done
CMCDragonkai opened this issue Apr 11, 2023 · 8 comments
Closed
4 tasks done

Fixed size buffer for QUICStream instead of object stream #5

CMCDragonkai opened this issue Apr 11, 2023 · 8 comments
Assignees
Labels
development Standard development r&d:polykey:core activity 4 End to End Networking behind Consumer NAT Devices

Comments

@CMCDragonkai
Copy link
Member

CMCDragonkai commented Apr 11, 2023

Specification

The QUICStream should be configurable with fixed size buffer. It turns out BYOB streams is not relevant for this. So instead we just need the options to configure the high water mark and related options.

Right now the stream just takes whatever chunk is acquired from the quiche library. So technically it's an object stream and not a byte stream.

@tegefaulkes need your input here to expand the spec, as you've done this already in PK with the websocket streams and RPC streams.

Benchmarks should push large amounts of data and observe that it doesn't really in very large amounts of memory usage.

Note that the quiche configuration does have configuration regarding the stream buffer limits... and even the connection buffer limits too.

The QUICStream buffer limits is purely a JS thing. So those buffers in quiche is controlling rust's memory usage. Subsequently quic stream's buffer limits is controlling JS's memory usage.

So:

QUICSTREAM <- QUICHESTREAM

If the quic stream buffer is full, it just stops reading from the quiche stream, which means the quiche stream buffer will be full, and that will produce backpressure on the remote side.

It's possible for the quichestream to be full before the quicstream is full.

Additional context

Tasks

  1. Add configuration option for setting the max buffer size for readable streams in QUICStream.
  2. Propagate this config up to where it's needed, QUICConnection, QUICClient, QUICServer, etc.
  3. Configure the readable streams to make use of the high water mark and interpret buffer size in bytes.
  4. Set a reasonable default size.
@CMCDragonkai CMCDragonkai added the development Standard development label Apr 11, 2023
@CMCDragonkai CMCDragonkai changed the title Fixed size buffer for QUICStream Fixed size buffer for QUICStream instead of object stream Apr 11, 2023
@CMCDragonkai
Copy link
Member Author

What would be the appropriate buffer size limits? Right now the default is set to 1 million bytes in initialMaxStreamDataBidiLocal and initialMaxStreamDataBidiRemote.

Benchmarks should show the effect of different buffer limits. Increased buffers should increase throughput up to a point, but reduce latency.

@tegefaulkes
Copy link
Contributor

Is there a technical reason for needing a larger buffer here? By default a web-stream will buffer up a single message. It can be configured to buffer up a byte amount as I've described before. But if the quiche stream has it's own internal buffer then isn't that all we need for buffering?

Otherwise, buffering on the webstream is set using the strategy options when creating a stream. highWaterMark: number sets the size of the internal queue. size: (chunk: any) => number maps the messages to it's actual size. If no size function is provided 1 message = 1 size in the queue, so in that case the queue is measure in messages. To measure it in bytes then the size function needs to convert chunks to it's byte length and the highWaterMark is set to the number of bytes.

When consuming a source, the controller.desiredSize will be the number of remaining space in the queue. You can over consume and fill up the queue over this amount. so the desired size is really a suggestion. When the queue exceeds the set limit then the stream's pull implementation stops being called. This is how the back-pressure is applied. In a push style system you'd have to check the controller.desiredSize, if it returns a negative value then you need to stop pushing data.

Just a note on the BYOB stream. The consumer of the readable stream provides the buffer. So you'd call const bytesRead = await readable.read(buffer, desiredAmount) and the buffer is passed to the internals to be written into. This would be the proffered option if the source also writes to a buffer in this style since it could cut down on copies. If the source provides a new buffer as output then the normal streams are preferred. Internally the buffer is just passed along the stream chain.

@CMCDragonkai
Copy link
Member Author

The quiche's buffer is nice and great. However because QUICStream will be a pull stream, it could end up reading a very small amount and then sending down the pipeline. That will result in wasted loops if the amount of data read is very small (tradeoff between latency and throughput). That's why it's important to preserve fixed size buffering across the pipeline.

At the end of the day I think experimentation is needed here to really see the different effects. My preference for byte streams comes from haskell, where IO streams were byte streams all the way through the system. And of course unix pipes too. I don't like mixing the modality of systems as I think that could cause confusions down the line.

Meaning object streams stay as object streams. Byte streams stay as byte streams.

@tegefaulkes
Copy link
Contributor

For this issue, we just need to add some config option for setting the max buffer size for the readable stream for QUICStream. We need to propagate this option up to the connection, client and server.

@tegefaulkes tegefaulkes self-assigned this May 3, 2023
@tegefaulkes
Copy link
Contributor

The writable stream can have it's own buffer. do we want to apply this to both the readable and writable webstream? If so, do we want the limits configurable separately?

@tegefaulkes
Copy link
Contributor

tegefaulkes commented May 3, 2023

I'm applying a buffer limit to both the readable and writable. They are configured separately with maxReadableStreamBytes and maxWritableStreamBytes. defaults are 100,000 bytes

tegefaulkes added a commit that referenced this issue May 3, 2023
@CMCDragonkai
Copy link
Member Author

CMCDragonkai commented May 3, 2023

Can you make sure these options work well with quiche's own options. We would need these options for both QUICClient and QUICServer.

Furthermore, is there any conflicts between options that may occur? That is quiche's options?

Should we namespace our options between quiche options and our options to avoid conflicts or will that be annoying?

@tegefaulkes
Copy link
Contributor

  1. These options are applied to both server and client.
  2. by working well do you mean they're optimal for performance? I can really tell until we do some performance benching. I think the biggest performance change we can make here is reducing the number of times we call send since it's an elevated system call. You usually do this by sending messages in batches which I think the socket supports.
  3. The quiche options are already essentially name spaced within the config object. We could namespace the other options but for now I don't think there are enough of them to warrant it.

CMCDragonkai pushed a commit that referenced this issue May 17, 2023
CMCDragonkai pushed a commit that referenced this issue May 17, 2023
@CMCDragonkai CMCDragonkai added the r&d:polykey:core activity 4 End to End Networking behind Consumer NAT Devices label Jul 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Standard development r&d:polykey:core activity 4 End to End Networking behind Consumer NAT Devices
Development

No branches or pull requests

2 participants