mirror of
https://github.com/n0-computer/noq.git
synced 2026-09-21 10:43:25 +00:00
Floris Bruynooghe
f3a73c7bdd
* 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 * refactor(proto): ordering of REACH_OUT and OBSERVED_ADDR building The ordering of these frames was a bit too eager. REACH_OUT is important timing-wise. But it is not more important than HANDSHAKE_DONE, PING, IMMEDIATE_ACK, ACK, ACK_FREQUENCY and should anyway not be sent on a path that also needs PATH_CHALLENGE. PATH_RESPONSE could be the one exception but it is also small. If one of those frames do end up in the same packet as REACH_OUT there will still be place for the REACH_OUT frame. The CRYPTO frame is left after it, because after the handshake that is only carrying auxiliarry non-time-sensitive information, and REACH_OUT is also only possible in the data space kind. OBSERVED_ADDRESS is definitely not that high priority, it might need to move back even further. --------- Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com>