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

Tracking Issue for restructuring sys_common #84187

Closed
CDirkx opened this issue Apr 14, 2021 · 9 comments
Closed

Tracking Issue for restructuring sys_common #84187

CDirkx opened this issue Apr 14, 2021 · 9 comments
Labels
A-technical-debt Area: Internal cleanup work C-cleanup Category: PRs that clean code up or issues documenting cleanup. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC E-help-wanted Call for participation: Help is requested to fix this issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@CDirkx
Copy link
Contributor

CDirkx commented Apr 14, 2021

The relationship between std::sys_common, std::sys and the rest of std is complex, with dependencies going in all directions: std depending on sys_common, sys_common depending on sys, and sys depending on sys_common and std. Ideally sys_common would be split into two and the dependencies between them all would form a dag.

There is a lot of interdependence between std, sys and sys_common, this is because sys_common contains several types of code:

  • abstractions over the different platform implementations in std::sys (for example std::sys_common::mutex)
  • code shared between platforms (for example std::sys_common::alloc)
  • code that is not platform-dependent (for example std::sys_common::poison)

In order to reduce the interdependence, sys_common will be restructured:

  • A new module sys::common is introduced; code that is shared by all platforms will be moved from sys_common to this new module.
  • Code that is shared between some but not all platforms will be moved to sys and shared using #[path] instead.
  • Code that is not platform-dependent will be moved out of sys_common to the appropriate place in std.

Ideally the end-result of this is sys_common again only containing platform-independent abstractions on top of sys.

@m-ou-se m-ou-se added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Apr 14, 2021
@CDirkx CDirkx changed the title Tracking Issue for splitting sys_common Tracking Issue for restructuring sys_common Apr 23, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Apr 23, 2021
Move `sys_common::poison` to `sync::poison`

`sys_common` should not contain publicly exported types, only platform-independent abstractions on top of `sys`, which `sys_common::poison` is not. There is thus no reason for the module to not live under `sync`.

Part of rust-lang#84187.
JohnTitor added a commit to JohnTitor/rust that referenced this issue Apr 23, 2021
Move `sys_common::poison` to `sync::poison`

`sys_common` should not contain publicly exported types, only platform-independent abstractions on top of `sys`, which `sys_common::poison` is not. There is thus no reason for the module to not live under `sync`.

Part of rust-lang#84187.
JohnTitor added a commit to JohnTitor/rust that referenced this issue Apr 23, 2021
Move `sys_common::poison` to `sync::poison`

`sys_common` should not contain publicly exported types, only platform-independent abstractions on top of `sys`, which `sys_common::poison` is not. There is thus no reason for the module to not live under `sync`.

Part of rust-lang#84187.
@JohnTitor JohnTitor added the C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC label Apr 28, 2021
@the8472 the8472 added the A-technical-debt Area: Internal cleanup work label Feb 8, 2022
@joboet
Copy link
Member

joboet commented Nov 13, 2022

This guideline came up whilst discussing #104329. However, I find the wording here to be confusing:

  • Code that is not platform-dependent will be moved out of sys_common to the appropriate place in std.

sounds to me like the exact opposite of

the end-result of this is sys_common again only containing platform-independent abstractions on top of sys.

Furthermore, this part of the guideline is clear

  • Code that is shared between some but not all platforms will be moved to sys and shared using #[path] instead.

but it is very limiting. Some code (like the generic fallback for Once) has no place inside sys since it is shared by not all, but most of the platforms. The current default seems to be the UNIX module, which is problematic since not everything is UNIX (e.g. Windows also uses that code). There definitely exists a need for a place for this kind of shared code to go, and sys_common is currently that place.

An alternate approach could be to move sys into sys_common and select (with cfg_if!) the required code for each feature there (selecting either the unique implementation in the former sys module or shared code living in sys_common). This would get rid of the unidiomatic #[path] imports, and also establish a strictly DAG-based structure (which seems to be a central idea of this guideline).

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 28, 2022
Move `ReentrantMutex` to `std::sync`

If I understand rust-lang#84187 correctly, `sys_common` should not contain platform-independent code, even if it is private.
@workingjubilee workingjubilee added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Mar 6, 2023
@workingjubilee
Copy link
Member

The #[path] route kiiinda sucks. Better than nothing, but still.

I think what's most important is that we eliminate the currently borderline circular dependencies in e.g. the networking code. It's an absolute rat's nest, making it easy to get lost and mistake where things are or where they should go in there. And #[path] enables such mazy tendencies rather than fighting against them.

I agree that sys_common should be more neutral. I think what I want to see is something like:

mod a exposes implementation primitives in systems
mod b composes parts from mod a into a single system
mod c reifies systems into a uniform abstraction
then mod c is pulled into std.

This probably gets more complex in reality, with multiple "kinds" of mod a and mod b.
cc @devsnek

@joboet
Copy link
Member

joboet commented Mar 7, 2023

I think what I want to see is something like:

mod a exposes implementation primitives in systems
mod b composes parts from mod a into a single system
mod c reifies systems into a uniform abstraction
then mod c is pulled into std.

I like that idea.

By the way: Would it make sense to discuss this synchronously at a libs-team meeting? I'm quite curious as to what the team thinks here.

@joboet
Copy link
Member

joboet commented Apr 21, 2023

I'm nominating this for T-libs to discuss and brainstorm solutions. A short summary:

There are currently three places in std where code shared between some, but not all platforms is put:

  • sys_common contains the shared thread parker implementations, the shared Once implementations and some shared networking stuff
  • sys::common now contains the thread_local macro implementations
  • sys::unix has served as a "lowest common denominator" module for things like futex-locks or raw byte OsStr encodings

This is inconsistent and makes it difficult to find relevant code. Also, a better structure would hopefully allow getting rid of the unidiomatic #[path]-based module imports commonly seen on the smaller platforms, which have caused quite some breakage (see #109727, #99800 and possibly others).

@rustbot label +I-libs-nominated

@rustbot rustbot added the I-libs-nominated Nominated for discussion during a libs team meeting. label Apr 21, 2023
@CDirkx
Copy link
Contributor Author

CDirkx commented Apr 21, 2023

I think what I want to see is something like:
mod a exposes implementation primitives in systems
mod b composes parts from mod a into a single system
mod c reifies systems into a uniform abstraction
then mod c is pulled into std.

This is indeed like I originally envisioned when working on this (but I haven't been involved with the Rust project for some time).
rust-structure
With sys_common (ideally with a different name, sys_wrapper or sys_interface or something) acting as a wrapper around system specific implementations, and sys::common only containing simple primitives, and all dependencies forming a DAG. I also remember wanting this for making it possible to split sys out of std into seperate crates for each platform, but I don't know if that is still a goal of the project.

@m-ou-se
Copy link
Member

m-ou-se commented Jun 21, 2023

We discussed this a few weeks ago in the libs meeting. We concluded that the currrent sys::common and sys_common setup is in such a bad state that basically any consistent new organisation would be a great improvement.

We didn't have strong opinions on what the new organisation should be. But since the organisation of these modules don't result in any stable guarantees, so we don't necessarily need to come up with the perfect new module layout right away.

So, ideally, what I think we need is someone to go through everything that currently exists in sys and sys_common and come up with a way to organise all those things into modules in a way that is less confusing than what we currently have. The tricky thing is that there are many cases of items that exist for e.g. only two platforms, or nearly all platforms, or in some other way don't fit into a layout with only platform-specific and 'common' modules.

Another thing that makes this tricky, is that it's currently not easy to run the compiler checks (./x.py check) for all of the different targets supported by std. (See #109099.)

Once there is a plan for a layout that seems like it could cover all items in sys/sys_common, the actual changes can then perhaps happen incrementally.

Marking this issue as E-help-wanted.

@m-ou-se m-ou-se added E-help-wanted Call for participation: Help is requested to fix this issue. and removed I-libs-nominated Nominated for discussion during a libs team meeting. labels Jun 21, 2023
@joboet
Copy link
Member

joboet commented Oct 22, 2023

Building on @workingjubilee's proposal and incorporating @ChrisDenton's idea in #116816, I'd like to propose the following structure:

  • sys::api contains platform-specific building blocks and safe abstractions.
  • sys::.. contains modules for individual features like filesystem support or thread-locals
    • These modules contain platform-specific or shared implementations of the individual features

std (potentially excluding std::os) now only pulls the unified abstractions from sys and is forbidden from including sys::api. sys::api and sys::* however can use features from std, which is necessary to use things like Mutex synchronization.

This structure has the benefit of allowing easier sharing of code between platforms, and is closer to patterns in crates providing cross-platform abstractions (see for instance getrandom). Unfortunately, some platform-specific code is now split across both sys::api::platform_name and sys::feature_name, but this is also the case with the current structure.

Examples

(to be expanded as required)

Filesystem support

The UNIX filesystem support is implemented atop the FileDesc abstraction, which provides a safe interface to the read/write syscalls. This abstraction is moved to sys::api::unix::fd. The unified abstraction atop this interface (the current sys::unix::fs module) is moved to sys::fs::unix and reexported (when the platform is UNIX) in sys::fs.

Thread parking

Thread parking support is mostly provided via shared implementations like the one utilizing futex, but some platforms (Darwin, Windows) have their own implementations. The implementations of Parker are all moved to sys::thread_parking::.., while sys::api::unix::thread_parking exposes the futex wrappers (Linux, ...) or a Semaphore structure (macOS) or the park and unpark functions (NetBSD).

Thread locals

The three thread-local implementations currently living in sys::common::thread_local are moved to sys::thread_local. Their building blocks (destructor registration (assuming #116850 is merged), lazy keys) are also moved to this module. In sys::api only remain the platform abstractions like guard enabling or key creation.

@workingjubilee
Copy link
Member

Said this on Zulip, but I'm basically in favor with two details:

  • I would like to not see an all-consuming std::sys::unix, nor a std::sys::api::unix, but rather something somewhat better divvied-up along the real divisions between "unix-y" platforms. I think we'd get farther with something like
    • std::sys::api::apple
    • std::sys::api::bsd
    • std::sys::api::linux
  • api seems like a strange name, but it's whatever, ultimately. a common name for this pattern is "PAL", or "platform abstraction layer", so it could be std::sys::pal, I guess?

@joboet
Copy link
Member

joboet commented Mar 12, 2024

Closing in favour of #117276.

@joboet joboet closed this as completed Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-technical-debt Area: Internal cleanup work C-cleanup Category: PRs that clean code up or issues documenting cleanup. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC E-help-wanted Call for participation: Help is requested to fix this issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants