search

aiortc / aioquic

QUIC and HTTP/3 implementation in Python
BSD 3-Clause "New" or "Revised" License
1.66k stars 236 forks source link

Fairer stream write scheduling [#125]. #475

Closed rthalley closed 7 months ago

rthalley commented 7 months ago

This greatly improves multiplexing and stream fairness in the use case from #125 with no significant cost (the run I'll show is slightly slower, but I think that could be measurement noise and if not it is small). The idea is that we keep an ordering for the streams and every time we generate datagrams to send, we recompute the list, moving anyone who got stream data transmitted to the end.

All the tests pass for me. There seems to be an intermittent loss of one line of coverage related to qlog that I don't understand; we'll see if it shows up in the CI.

Here the is before case, in the terminal and qvis:

Screenshot 2024-02-25 at 17 33 33 Screenshot 2024-02-25 at 17 34 03

And now the much nicer after case:

Screenshot 2024-02-25 at 17 31 29 Screenshot 2024-02-25 at 17 32 09
codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (2b3d9b8) to head (8e84a67). Report is 1 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #475 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 25 25 Lines 4949 4961 +12 ========================================= + Hits 4949 4961 +12 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jlaine commented 7 months ago

Thanks so much for tackling this, it's long overdue!

I'm going to have to re-read this, as I'm not sure what happens in certain situations. For instance if you have a mix of streams being discarded (finished) and another stream which hits an exception.. won't we end up keeping the discarded stream in the queue?

I'm also wondering whether deque might make sense here.

rthalley commented 7 months ago

I started with deque but got rid of it as it wasn't really adding any value in the final way I did things, so I went with a plain list. Good point re the exception handling; I thought I had it covered but didn't think of that case properly. I suppose we can record discarded and do a try: ... finally: that prunes the discards always

rthalley commented 7 months ago

I fixed the exception handling. I also tested again, with bigger transfers and with qlog turned off. The new code is actually a bit faster, so I'm not worried about performance any more.