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
Improve the QUIC_RSTREAM implementation #19794
Conversation
This is a draft as tests need to be added to cover the new API calls. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. One comment, decision is up to you.
@hlandau still OK? |
This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago |
Needs rebase and second approval |
Add API calls to avoid copying data when reading These are ossl_quic_rstream_get_record() and ossl_quic_rstream_release_record(). Add side storage for the stream frame data. When there are too many packets referenced by the receiving stream the function ossl_quic_rstream_move_to_rbuf() can be called to move the data to a ring buffer.
9604b31
to
d57773c
Compare
Rebased. @hlandau please reconfirm. Ping @openssl/committers for second approval. The improvements should be used by the code, but that is a separate work task. |
The reason is to avoid unnecessary allocations/deallocations. |
Hmmm, feels a bit like premature optimisation. The downside will be extra copies won't it? |
Which extra copies? The ring buffer is used only when we are asked to deallocate the packets. We primarily use a linked list of received frames (with pointers to the packets holding the frames to deallocate later). The ring buffer is used only when the ossl_quic_rstream_move_to_rbuf() is called. That would be called by quic channel if there are too many packets being in process. Detecting that condition is something that still needs to be added to the quic channel implementation. |
fixup pushed addressing @mattcaswell 's comments. @hlandau @mattcaswell please approve |
CI failure looks relevant. |
Reported by Marc Schönefeld.
Should be fixed now. |
@mattcaswell @hlandau please re-approve |
This pull request is ready to merge |
Merged to master branch. Thank you for the reviews. |
Add API calls to avoid copying data when reading These are ossl_quic_rstream_get_record() and ossl_quic_rstream_release_record(). Add side storage for the stream frame data. When there are too many packets referenced by the receiving stream the function ossl_quic_rstream_move_to_rbuf() can be called to move the data to a ring buffer. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from #19794)
Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from #19794)
Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from #19794)
Reported by Marc Schönefeld. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from #19794)
Add API calls to avoid copying data when reading
These are ossl_quic_rstream_get_record() and
ossl_quic_rstream_release_record().
Add side storage for the stream frame data.
When there are too many packets referenced by the
receiving stream the function ossl_quic_rstream_move_to_rbuf() can be called to move the data to a ring buffer.
Checklist