Clean up Mutex/Instant::now() use for send message error logging

[recatek]

No description provided.

[Ralith]

This adds abstraction to simple code and introduces some complex cfg gates. Can you elaborate on the motivation?

[recatek]

Sure. If you don't have/use logging, this saves a little space in UdpSocketState, reduces some code duplication, and skips an Instant::now(). Overall it also makes it easier to audit for usage of Instant::now() in quinn-proto/quinn-udp (trying to stay I/O-agnostic).

[gretchenfrage]

Sure. If you don't have/use logging, this saves a little space in UdpSocketState, reduces some code duplication, and skips an Instant::now(). Overall it also makes it easier to audit for usage of Instant::now() in quinn-proto/quinn-udp (trying to stay I/O-agnostic).

Storing a Mutex<Instant> per UDP socket and calling Instant::now() both seem trivial in cost. Reducing code duplication at the cost of net-increasing lines of code and adding cfg-gated type aliases seems like a net loss in terms of code simplicity.

I'm in favor of locking down usage of Instant::now(), but perhaps it would be better to start with more clearly articulating what your target use case is specifically and work from there to see how we can accommodate it?

[djc]

If you want to go here (I hope this is motivated by actual benchmarks rather than "premature" optimization efforts), I don't think this is the right way to go. Instead, I'd extract the error code into an ErrorSink type (not a trait, and definitely not a type alias) which could get swapped out with different implementations depending on platform and/or features.

[recatek]

Using an ErrorSink is a great idiom, thanks, I switched to that. Overall it's less about perf and more about consistency. Particularly quinn-proto but also, as far as I can tell, quinn-udp both appear to be built to avoid calling Instant::now() in business code to leave that part of syscall/IO work to the user. I did a review of those two crates and saw one use of it in business code (removed in #2157), many usages of it in tests (which is fine), and this use in logging. I figured that reducing the places where it's acceptably called would make it easier to audit these usages. That it spares some bytes and avoids a syscall or two at runtime is a nice but extremely minor value add.

[djc]

Overall it's less about perf and more about consistency. Particularly quinn-proto but also, as far as I can tell, quinn-udp both appear to be built to avoid calling Instant::now() in business code to leave that part of syscall/IO work to the user. I did a review of those two crates and saw one use of it in business code (removed in #2157), many usages of it in tests (which is fine), and this use in logging. I figured that reducing the places where it's acceptably called would make it easier to audit these usages. That it spares some bytes and avoids a syscall or two at runtime is a nice but extremely minor value add.

Well, I think you're overindexing on a particular aspect of a design goal - our design goal for quinn-proto was that it would be deterministic (that is, all function call outputs can be fully determined from function call inputs). That means there should not be Instant::now() calls with legible values. The instance you found did not actually have a legible value. I don't think the use for logging in quinn-udp is particularly legible, and we definitely don't have the deterministicness goals for quinn-udp (since that's all about interacting with the network subsystem).

As is, I don't think we should merge this changes. If you can clean up the commit history (the 2nd or 3rd time I asked...) to have one clean commit that refactors the current implementation to have a better abstraction boundary, and then a second commit that makes the implementation optional, we can consider whether that trade-off makes sense here.

[recatek]

If you can clean up the commit history (the 2nd or 3rd time I asked...)

Sorry, I'm used to squashing before merging a PR, not beforehand. I'm not sure what is being asked for here but I can look it up and try my best. The current state of the files changed in the PR should be accurate to what I'd like to merge. Do you want me to close this and open a new clean PR?

current implementation to have a better abstraction boundary, and then a second commit that makes the implementation optional

Is the abstraction boundary you're asking for here the ErrorSink or something else?

FWIW I'm not terribly committed to this so I'm happy to just close this if it isn't a decent change.

[recatek]

I think that cleaned it up as requested?

[recatek]

I have unbelievably messed up this branch trying to squash it (I have no idea what I'm doing here) and nobody wants this change so I'm just going to close it.