Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for niche optimization on Unix platforms #222

Merged
merged 3 commits into from
Apr 28, 2021
Merged

Allow for niche optimization on Unix platforms #222

merged 3 commits into from
Apr 28, 2021

Conversation

SabrinaJewson
Copy link

The standard library's file descriptor types have a niche of -1, allowing Option<T> to have the same size as T. By storing a TcpStream internally instead of a raw file descriptor, Socket can also take advantage of this. Currently, this incurs a slightly cost as {from, as, into}_raw_{fd, socket} won't be inlined, however if this Rust PR is merged it will be. Storing a TcpStream also allows for closing to be implemented by the standard library instead of this one, which is a nice benefit.

@Thomasdezeeuw
Copy link
Collaborator

I actually plan to use socket2 in std lib (issue #212), so this change would make it a lot harder. Do you know if there are any plans to expose the valid range of an int (i.e. what TcpStream is already using) as a public API?

@SabrinaJewson
Copy link
Author

I do not know of any plans for that, no. This niche actually makes things difficult because it would probably be considered a regression for std to remove the niche. Excluding adding more things to the language, we could hide Socket behind a std feature flag, and have libstd use SockRef instead so it can implement its own niche optimization. I don't know if that would cause a circular dependency though - potentially if -Zbuild-std is enabled?

Copy link
Collaborator

@Thomasdezeeuw Thomasdezeeuw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's best to forward with this, we'll figure the use-in-std stuff later.

I've reviewed the changes, they look good. Two points and some small things (prefixed with nit). I do have one question: does self.as_raw generate the same code as self.inner in all functions, i.e. is as_raw always inlined?

src/socket.rs Show resolved Hide resolved
src/socket.rs Show resolved Hide resolved
src/sys/unix.rs Show resolved Hide resolved
src/sys/windows.rs Show resolved Hide resolved
@Thomasdezeeuw
Copy link
Collaborator

FreeBSD failure seems unrelated, so I'm merging.

@Thomasdezeeuw Thomasdezeeuw merged commit eaa5f8f into rust-lang:master Apr 28, 2021
@Thomasdezeeuw
Copy link
Collaborator

Thanks @KaiJewson.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants