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

Remove Select structure; implement inline select! macro #17601

Closed
wants to merge 1 commit into from

Conversation

apoelstra
Copy link
Contributor

The old Select structure was very inefficient and required the user
to use unsafe code (and manually maintain a "this variable will never
move" invariant). Searching through several popular repositories, the
only instance I found of it being used was in servo --- to avoid the
setup cost of putting select! in a loop, which would create a new
Select on every iteration.

This commit deletes the Select structure entirely and moves all of
its code into the select! macro. The benefit of this is that there
is no more need for allocations, no more need for unsafe code, and
no setup costs (so servo can now use select! directly).

This also changes the interface to select! to fix #12902.

Fixes #12902.

[breaking-change]

The old `Select` structure was very inefficient and required the user
to use unsafe code (and manually maintain a "this variable will never
move" invariant). Searching through several popular repositories, the
only instance I found of it being used was in servo --- to avoid the
setup cost of putting `select!` in a loop, which would create a new
`Select` on every iteration.

This commit deletes the `Select` structure entirely and moves all of
its code into the `select!` macro. The benefit of this is that there
is no more need for allocations, no more need for unsafe code, and
no setup costs (so servo can now use `select!` directly).

This also changes the interface to select! to fix rust-lang#12902.

Fixes rust-lang#12902.

[breaking-change]
@rust-highfive
Copy link
Collaborator

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

#[allow(unused_imports)]
mod test {
use std::prelude::*;

Copy link

Choose a reason for hiding this comment

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

Many of these tests are still valid and most likely appropriate to keep. Updating them may be as simple as changing the select! invocations to use the new syntax.

@ghost
Copy link

ghost commented Sep 28, 2014

Great work, thanks!

It's a bit of a shame we have to let the limitations of macros-by-example drive the syntactical changes but since the macro would probably still stay experimental, it's not a huge problem.

For the record, have you considered using a procedural macro as a wrapper for the macro-by-example to keep the old syntax and have it work properly with the receiver being an arbitrary expression?

@apoelstra
Copy link
Contributor Author

Thanks @jakub-. Should I move the deleted tests to libstd/macros.rs?

Regarding a procedural macro, it did cross my mind, but I've never written one (that I recall), and I don't think this would be an appropriate place for a first attempt :).

@jdm
Copy link
Contributor

jdm commented Sep 28, 2014

Servo's usage of Select is actually a bit more complicated than you make out - http://mxr.mozilla.org/servo/source/components/script/script_task.rs#425 . In particular, we have conditional inclusion based on other variables now.

@lucidd
Copy link
Contributor

lucidd commented Sep 28, 2014

I use the Select struct in one of my projects to select over a Vec<Receiver> i think this is not possible with the select macro at the moment right? Here are the 2 functions i need this for.

@huonw
Copy link
Member

huonw commented Sep 28, 2014

cc @alexcrichton

@apoelstra
Copy link
Contributor Author

@lucidd That's correct, select! cannot do this. You can copy the code of select! out (it is not much longer than the code needed to use Select) if you like.

People on IRC are telling me that the select/io stuff is slated to be rewritten. So maybe this is too big a breaking change if we are just going to break again in the next month or two?

@jdm Oops! I need to update my copy of servo then, that if statement was not in the tree when I checked.

@alexcrichton
Copy link
Member

I will review this tomorrow.

@alexcrichton alexcrichton assigned aturon and alexcrichton and unassigned aturon Sep 28, 2014
($($name:pat from $rx:expr => $code:expr),+) => {
select!{ $($name from $rx using recv => $code),+ }
};
($($name:pat from $rx:expr using $meth:ident => $code:expr),+) => ({
Copy link
Member

Choose a reason for hiding this comment

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

I originally was somewhat wary of using syntax like this because it's pretty alien with respect to the rest of rust code. That being said it was unfortunate that $expr.$ident() didn't quite work out as I expected.

@alexcrichton
Copy link
Member

The benefit of this is that there is no more need for allocations

I was under the impression that using Select did not require any allocations, could you point me to the point where something was allocated?

no more need for unsafe code

This is a bit of an interesting pro because none of the usage of the macro previously had any unsafe code, only when you used the Select struct itself. It seems that this keeps the status quo (no unsafe code in macros), except it removes Select.

and no setup costs

Out of curiosity, have you measured this? The only setup cost was designed to be maintenance of a simple linked list, which I wouldn't expect to have any lasting impact.


I definitely agree that Select is not the best structure in the world, I'm sad that it ended up having an unsafe interface, and I've always wanted to improve it! That being said, it has some concrete advantages over a purely macro-based system:

  • The setup cost of the macro is still nonzero. Attempting to select on a channel involves a few atomics to register yourself as available for selection. The Select structure allowed to you bring this cost up front and then select in a loop, whereas only using select! means you have to re-register yourself as interested each iteration of the loop.
  • The Select structure allowed for programmatic control over what's being selected over. As mentioned by @jdm, it looks like servo is already doing this and I think this can be important rather than requiring you to always statically know the set of channels that need to be selected over.

It seems like #12902 is largely just about the macro rather than the Select struct itself. Would it be possible to tweak the macro syntax (as done here) and not remove the Select structure?

@apoelstra
Copy link
Contributor Author

After discussion with @alexcrichton on IRC, I have decided to close this. It is too much of a burden on Select users, and the unsafety can likely be dealt with in a more surgical manner.

@apoelstra apoelstra closed this Sep 29, 2014
@apoelstra apoelstra deleted the new-select branch September 29, 2014 15:14
lnicola pushed a commit to lnicola/rust that referenced this pull request Jul 28, 2024
Fix incorrect encoding of literals in the proc-macro-api on version 4

Quick follow up on rust-lang/rust-analyzer#17559 breaking things
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.

Cannot use select! with Receiver contained in a struct
7 participants