Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add select::FdSet::fds() method #1207
Add select::FdSet::fds() method #1207
Changes from 2 commits
1b5ea70
41db1bf
8b4a433
4a101ae
bcfcf82
eb5f289
1171817
e8c7961
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
libc::FD_ISSET
should be taking a*const
pointer rather than*mut
. I just opened a PR to fix that.rust-lang/libc#1725
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an aside, if we're worried about the cost of this copy, then we might want to consider not deriving
Copy
forFdSet
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You know, that's an excellent point. On my system,
FdSet
is 128 bytes big. Users probably shouldn't copy such a time unless they really need to. However removingCopy
doesn't necessarily prevent the value from ever beingmemcpy
'd, and the consensus is that it's best to implCopy
regardless of size. See https://www.reddit.com/r/rust/comments/2xxjda/when_should_my_type_be_copy/ and https://internals.rust-lang.org/t/pre-rfc-copy-like-trait-for-large-types/11852/14 for some discussion.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that for now we should proceed without waiting for rust-lang/libc#1725 . But copying the entire structure on each loop of the iterator is woefully inefficient. I think you should retain the
&mut
reference instead.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. This sadly means that the iterator cannot implement
Clone
, but I don't see a good way around that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically a C programmer will know what the maximum file descriptor he registered was, and when inspecting the
fd_set
he won't need to iterate all the way toFD_SETSIZE
, which can be up to 64k on some platforms. But as written your iterator will always iterate that high. That's not good for performance. What if you add anmax: Option<RawFd>
argument tofds
? If supplied, the iterator won't check any higher than that. Such an argument could also be useful tohighest
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to
fds()
. I didn't add tohighest()
in the interest of not introducing unnecessary breakage. If someone wants to get that performance gain, they can useset.fds(Some(my_limit)).next_back()
, which isn't terribly hard to write and is pretty clear.