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

Partially implement thread scheduling attributes API proposal #101222

Closed
wants to merge 2 commits into from
Closed

Partially implement thread scheduling attributes API proposal #101222

wants to merge 2 commits into from

Conversation

ian-h-chamberlain
Copy link
Contributor

@ian-h-chamberlain ian-h-chamberlain commented Aug 31, 2022

This PR is a sketch of the standard library API proposed in rust-lang/libs-team#71 (comment)

It most likely doesn't compile on all platforms, and it won't work correctly on Linux without added libc support for the corresponding pthread APIs, but I think this works as a proof-of-concept to create a portable-enough API that abstracts over the OS-specific details.

Many of the changes are taken from #98514 but the public API is somewhat different. See the libs-team discussion linked above for more details.

CC @AzureMarker @Meziu

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 31, 2022
@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 31, 2022
@@ -97,6 +102,9 @@ impl Thread {
}
}

pub type Priority = ();
Copy link
Member

Choose a reason for hiding this comment

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

These should be structs instead of type definitions in case this is implemented for the target in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this is a private type, I think it should be possible to change its type freely without any consequences to user code (alias or otherwise). However if it was re-exported as pub (per your other comment) then I agree it would be better as struct Priority(())

Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah, this is sys and not actually public

@@ -346,6 +361,18 @@ impl Builder {
self
}

#[unstable(feature = "thread_scheduling", issue = "none")]
pub fn priority(mut self, priority: Priority) -> Builder {
Copy link
Member

Choose a reason for hiding this comment

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

Why can't this take in imp::Priority?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could only do so if imp::Priority was also re-exported (otherwise it's a private type in a public interface).

I couldn't find many (if any) instances of re-exporting things from sys, although in this case it would really be a re-export of stuff from os which is already public, so maybe it would be fine? Even so, I think it would mean that Priority + Affinity would have do be documented in every variation of sys::* rather than just once in the public interface.

Also, for those unsupported platforms that just use () my thinking was it would make more sense to use an "unconstructible" public type. I suppose re-exporting a struct Priority(()) would achieve the same.

@ian-h-chamberlain
Copy link
Contributor Author

ian-h-chamberlain commented Nov 28, 2022

@joshtriplett have you (or any other libs team) had any chance to look at this and see if it meets your expectations from the discussion in rust-lang/libs-team#71 ? If not, are there other changes or tests/use-cases I could add to prove out this API further to hopefully move along the ACP? Thanks!

Also tried to ping @rust3ds/active to look over the docs I added / provide any inputs, but I guess Github doesn't let you cross-org ping like that.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
configure: rust.debug-assertions := True
configure: rust.overflow-checks := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
Attempting with retry: make prepare
---
    |
19  | pub type Priority = ();
    | ----------------------- previous definition of the type `Priority` here
...
109 | pub type Priority = ();
    | ^^^^^^^^^^^^^^^^^^^^^^^ `Priority` redefined here
    |
    = note: `Priority` must be defined only once in the type namespace of this module

error[E0428]: the name `Affinity` is defined multiple times
    |
20  | pub type Affinity = ();
20  | pub type Affinity = ();
    | ----------------------- previous definition of the type `Affinity` here
110 | pub type Affinity = ();
110 | pub type Affinity = ();
    | ^^^^^^^^^^^^^^^^^^^^^^^ `Affinity` redefined here
    |
    = note: `Affinity` must be defined only once in the type namespace of this module
For more information about this error, try `rustc --explain E0428`.
error: could not compile `std` due to 2 previous errors
Build completed unsuccessfully in 0:00:25

@bors
Copy link
Contributor

bors commented Jan 23, 2023

☔ The latest upstream changes (presumably #106981) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants