-
Notifications
You must be signed in to change notification settings - Fork 1
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
Use a BYOB
readable stream to optimise the QUICStream
readable stream
#65
Comments
Relevant discussion here: 5fce16d#r1349859926 Basically copying the chunks is good for UX, but it's not good for performance, since it's not part of the streams spec then I think it is implementation-specified. So by supporting BYOB, we allow the user to be able to recover the more efficient sharing of buffers. The same thing should be done in js-ws @amydevs. |
After experimenting with the BYOB ReadableStream, I'm starting to think it might not be a good fit for the following reasons.
I can go into more detail. but from what I've seen this doesn't fulfil our needs. That said, we need to fix the problem where we have a 1MB buffer for each readable stream. I think we just need to make that buffer much smaller on the order of a few KB. |
Is this something that can be solved with an alternative - something that is running on the rust side? Also just to sanity check this, can you throw your requirements into ChatGPT (mention what you have tried), ask it for alternative solutions. |
Chat GPT didn't have any more insight than I had. https://chat.openai.com/share/5323e80c-f2af-4a8c-9c65-8394c3c965f7 Moving forward we're just going to significantly reduce the buffer size used by the streams. It should be a simple change. We'll need to do some bench-marking to see the difference. I think performance will be the same unless we have very large messages. But for our RPC usage that really shouldn't be the case. I'm focusing on other stuff now so I'll get back to this. |
I am kind of confused here. During some benchmarks we saw that the longest time was doing the CPU operation of reading or writing into the stream on the rust side. Due to passing control between JS side and Rust side this slowed down other operations due to single threadedness but also FFI cost. One idea is to introduce multithreading to the rust side. If that occurs then we still have to copy data between JS side and Rust side right? Reducing the number copying that occurs would a good idea. BYOB was supposed to help reduce of number of copies. However we have to beware of the possibility of asynchronous clobbering. So I'm not entirely understanding the constraints right now. |
There are 3 main constraints
Ideally the BYOB stream was meant to solve all 3. But from experimenting with it, it falls short in 2/3 ways.
It light of these 3 things I don't think using the BYOB would be worth the extra complexity. On top of that it only functions as a BYOB if you're using the BYOB mode reader. So it's very limiting. For comparison, reducing the main buffer for the QUICStream only addresses constraint 1. But it's the main problem right now. |
Closed as |
Whoops, closed it the wrong way. |
Specification
Currently the readable stream in
QUICStream
uses a single buffer thatstream_recv
writes to and gets sliced, copied and enqueued into the readable stream. Previously this was just sliced and enqueued. There was a fix in #63 to copy the sliced buffer to prevent all enqueued buffers referencing the same memory but this has a performance cost associated with it.We want to look into changing the readable stream to a
BYOB
readable stream. This will allow us to cut down on copies by allowing us to pass in buffers to read into. This should remove the need to copy or create a new buffer for each read by using the provided buffer.We'll need to check if
BOYB
readable streams can be used the same way as normal readable streams.Additional context
Tasks
QUICStream
readable stream with aBYOB
readable stream.The text was updated successfully, but these errors were encountered: