rust-lang / socket2

Advanced configuration options for sockets.
https://docs.rs/socket2
Apache License 2.0
683 stars 227 forks source link

Shouldn't socket recv() promise in its doc allows a nicer API #501

Closed keepsimple1 closed 7 months ago

keepsimple1 commented 8 months ago

https://github.com/rust-lang/socket2/blob/c93cdcc25fd3de7ef20515b9c16ed2b5771ed268/src/socket.rs#L409-L414

A friendly question: because of the above promise, is it true that this API could change its signature to accept buf: &mut [u8] ? That will be so much helpful! (The current API is useless for many code bases where unsafe is not allowed / or strongly discouraged).

(I noticed open issues like #366 , but it seems being there more than 1 year, when will it happen?) I'm a user of this crate and I think this current signature is really unnecessary hurdle for its users.

Thomasdezeeuw commented 8 months ago

You can use the std::io::Read implementation for this. It makes a read(2) system call instead of send(2), but those behave the same way with the arguments we pass.

As for the recv method itself, it will continue to use to uninitialised bytes to not force the caller to initialise them only to overwrite them again. Using the readbuf API would be nice, but that's not stable, so not usable by this crate.

keepsimple1 commented 8 months ago

thanks for your reply. I should have, but forgot to clarify that, what I needed is .recv_from() which is in the same boat as .recv() (and doc forwarded to .recv()) .

Currently I'm using read(), but won't be able to get the source IP.

Thomasdezeeuw commented 8 months ago

For recv_from we don't have a version that uses &[u8]. We've had attempts to to add them, e.g. https://github.com/rust-lang/socket2/pull/246, but it basically came down to coping all the recv methods only to call the version with uninitialised bytes, so it wasn't worth the maintenance burden.

keepsimple1 commented 7 months ago

I guess it was probably a tough decision for you the maintainers, but I was curious how much performance gain of the version with uninitialized buffer. Among all socket2 users, how many percent of users prefer to use this version of uninitialized buffer?

It's probably impossible to change it back, but I just felt it's not a worthy trade-off, i.e. disabling / crippling common use cases for (IMO) edge cases.

Thomasdezeeuw commented 7 months ago

I guess it was probably a tough decision for you the maintainers, but I was curious how much performance gain of the version with uninitialized buffer. Among all socket2 users, how many percent of users prefer to use this version of uninitialized buffer?

I don't really have performance test ready, but here an example of using uninitialised memory for HTTP headers: https://github.com/5225225/hyper/commit/325b7e519a2abb00c3ac95486ebfad14f0119ab2 that saves ~5%.

It's probably impossible to change it back, but I just felt it's not a worthy trade-off, i.e. disabling / crippling common use cases for (IMO) edge cases.

It's not really the common case though. Most users of this library use advance features, where not initialising memory is considered the common case.

keepsimple1 commented 7 months ago

I don't really have performance test ready, but here an example of using uninitialised memory for HTTP headers: 5225225/hyper@325b7e5 that saves ~5%.

5% seems to me really not worth changing a safe Rust code to unsafe Rust code, especially if in the grand scheme of a bigger system.

Thomasdezeeuw commented 7 months ago

5% is a lot actually. That's massive improvement. Especially for a 1 line change that is documented as supported by socket2.

I think the best way forward is the read buf, which is still unstable. So closing in favour of #366.