Skip to content

Commit 4bf3dc9

Browse files
flubmatheus23
andauthored
fix: improve packet scheduling for not-validated paths (#444)
* it's the weekend, this is useful in my head so let's check in totally broken code. * more * wip * wip * wip * try more stuff * moar * more wip * can't send anything when there's nothing to send This stopped us from going to the next space to coalesce the Initial with the Handshake packet on the server-side. And probably lots of other stuff * wip * and check for loss probes that need to be sent * some doc updates * remove SendableFrames::validation, it was redundant Turns out this was exactly the same case as SendableFrames::space_id_only. * Add a test that checks we send on an available path * simplify the packet scheduling logic a little * Simplify, immediate close during handshake is broken though * wip * Guess we do need a new field for this Slightly sad to have to make an explicit exception for this, at least currently. * clippy * may_send_data has been made smarter, don't need this * wip * tweak logging * proptests should maybe log errors? * drive to idle for a 1000 iterations, 100 is a bit small * turns out we don't need this and i find it a bit confusing * remove stuff split off into #494 * Remove changes split off into 495 * typo * style * ww * remove this old thing * tweak * needs_loss_probe is a variable above, it's not that bad * collapse these two cases. it's a hard call though * well doh, the doc comment says why * wordsmithing, because 90% of packet scheduling is about that * put this back, split off into another pr * fix doc comment * Typos from code review Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com> * Do not allocate and move scheduling info to separate function Two bits of PR review: - Move the scheduling to a separate function. - Avoid an allocation, but this has some tradeoffs. - I now have two loops that look for the PathId by doing `next_path_id = self.path.keys().find(|i| **i > path_id).copied();`. It might be possible to fold the MTU discovery in the main poll loop, MTU packets would get a slightly higher priority but probably not really harmful overall. - I now need to do the computation for `have_validated_status_available_space many more times. I'm not sure how much the compiler manages to remove all of that. Is it smart enough to figure out that `have_validate_status_available_space` won't change between the calls and does it move it out? Does it make the iteration as fast as the previous version? On the other hand, we now have some situations where we don't have to compute the scheduling information, and no longer need to compute it for all paths if we don't send on the last path. What do you think, which version is better (though I also adopted @matheus23's feedback about splitting it off to a function, but that doesn't affect this really. It does make the diff a little bit more though)? As an aside, in working out of how scheduling should work it was really helpful to have to extremely explicit as a bunch of data that's computed up-front. But it's fair that now we know this is how it should work that we can implement it in the most optimal way. * Small attempt at making this clearer * fix docs * Rename field based on more feedback * fix docs --------- Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com>
1 parent 2156ac1 commit 4bf3dc9

3 files changed

Lines changed: 288 additions & 126 deletions

File tree

0 commit comments

Comments
 (0)