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

standard lazy types #2788

Open
wants to merge 10 commits into
base: master
from

Conversation

@matklad
Copy link
Member

matklad commented Oct 18, 2019

Add support for lazy initialized values to standard library, effectively superseding the popular lazy_static crate.

use std::sync::Lazy;

// `BACKTRACE` implements `Deref<Target = Option<String>>` 
// and is initialized on the first access
static BACKTRACE: Lazy<Option<String>> = Lazy::new(|| {
    std::env::var("RUST_BACKTRACE").ok()
});

Rendered

text/0000-standard-lazy-types.md Outdated Show resolved Hide resolved
text/0000-standard-lazy-types.md Outdated Show resolved Hide resolved
text/0000-standard-lazy-types.md Outdated Show resolved Hide resolved
text/0000-standard-lazy-types.md Outdated Show resolved Hide resolved
text/0000-standard-lazy-types.md Outdated Show resolved Hide resolved
text/0000-standard-lazy-types.md Show resolved Hide resolved
text/0000-standard-lazy-types.md Show resolved Hide resolved
Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>
@pitdicker

This comment has been minimized.

Copy link
Contributor

pitdicker commented Oct 18, 2019

Great RFC, I hope it makes it into the standard library!

One other aspect that the crate conquer_once tackles is a distinction between blocking and non-blocking methods. I believe this is one possible way to divide the api?

    /// Never blocks, returns `None` if uninitialized, or while another thread is initializing the `OnceCell` concurrently.
    pub fn get(&self) -> Option<&T>;

    /// Never blocks, returns `Err` if initialized, or while another thread is initializing the `OnceCell` concurrently.
    pub fn set(&self, value: T) -> Result<(), T>;

    /// Blocks if another thread is initializing the `OnceCell` concurrently.
    pub fn get_or_init<F>(&self, f: F) -> &T
    where
        F: FnOnce() -> T,
    ;

    /// Blocks if another thread is initializing the `OnceCell` concurrently.
    pub fn get_or_try_init<F, E>(&self, f: F) -> Result<&T, E>
	where
        F: FnOnce() -> Result<T, E>,
    ;
}

Alternatively, all four methods could be blocking, and a try_get and try_set may suffice for a non-blocking use case.

But don't let this comment derail the discussion about whether to include OnceCell in the standard library to much.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Oct 18, 2019

fn pointers are not ZSTs, so we waste one pointer per static lazy value. Lazy locals will generally rely on type-inference and will use more specific closure type.

It looks like rust-lang/rust#63065 will allow using a zero-size closure type in a static:

static FOO: Lazy<Foo, impl FnOnce() -> Foo> = Lazy::new(|| foo());

But this will require spelling the item type twice. Maybe that repetition could be avoided… with a macro… that could be named lazy_static! :)

@KrishnaSannasi

This comment has been minimized.

Copy link

KrishnaSannasi commented Oct 18, 2019

@SimonSapin or we can add bounds to Lazy like so

struct Lazy<F: FnOnce<()>> { .. }

and use it like this,

static FOO: Lazy<impl FnOnce() -> Foo> = Lazy(foo);
@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Oct 18, 2019

@KrishnaSannasi How does that help with the requirement that static items name their type without inference?

static FOO: &[_] = &[2, 4];

(Playground)

   Compiling playground v0.0.1 (/playground)
error[E0121]: the type placeholder `_` is not allowed within types on item signatures
 --> src/lib.rs:1:15
  |
1 | static FOO: &[_] = &[2, 4];
  |               ^ not allowed in type signatures
@KrishnaSannasi

This comment has been minimized.

Copy link

KrishnaSannasi commented Oct 19, 2019

@SimonSapin I'm not sure what you are asking, I didn't use _ anywhere in my example. It would be nice if we could have type inference in static/const that only depended on their declaration, but we don't have that (yet).

You proposed using impl Trait to get rid of the cost of fn() -> ..., and noted a paper cut where you would have to name a type multiple times, and I proposed a way to get rid of that paper cut by changing Lazy by removing the redundant type parameter.

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Oct 19, 2019

It doesn't really seem all that worth it to me to spend time worrying about how to inline a function that is called at most one time in the entire execution of a program.

@pitdicker

This comment has been minimized.

Copy link
Contributor

pitdicker commented Oct 20, 2019

A small difference that may be worth noting: std::sync::Once will only run its closure once, while OnceCell::get_or{_try}_init will one run its closure once successfully.

@matklad

This comment has been minimized.

Copy link
Member Author

matklad commented Oct 21, 2019

@pitdicker pointed out that we actually can have some part of the sync API exposed via core. This unfortunately increases design/decision space a bit :) I've amended the rfc.

text/0000-standard-lazy-types.md Outdated Show resolved Hide resolved
@tarcieri

This comment has been minimized.

Copy link

tarcieri commented Oct 21, 2019

@jhpratt the author of this RFC @matklad is the author of once_cell. It also mentions:

The proposed API is directly copied from once_cell crate.

Altogether, this RFC proposes to add four types:

* `std::cell::OnceCell`, `std::cell::Lazy`
* `std::sync::OnceCell`, `std::sync::Lazy`

This comment has been minimized.

Copy link
@nixpulvis

nixpulvis Oct 21, 2019

I see that we have sync::Once and cell::Cell, but this may be confusing. Just a thought.

This comment has been minimized.

Copy link
@KrishnaSannasi

KrishnaSannasi Oct 21, 2019

This is kinda like the combination of both, so the naming may be good (modulo Send/Sync concerns). But it would be better if the !Sync version had a different name from the Sync version.

This comment has been minimized.

Copy link
@clarfon

clarfon Oct 22, 2019

Contributor

Perhaps acknowledging OnceCell as a WORM (write once, read many) might make WormCell be a better name? Assuming that the implementation isn't too sluggish.

This comment has been minimized.

Copy link
@clarfon

clarfon Oct 22, 2019

Contributor

I also would really like to see there be a different name for these, rather than relying on the fact that they lie in two separate modules. Maybe just prefixing the sync implementations with Sync is enough, i.e. SyncLazy and Lazy.

This comment has been minimized.

Copy link
@matklad

matklad Oct 23, 2019

Author Member

I would be against WORM terminology, as it seems rather obscure: I don't think I see it often enough to not consult wikipedia. A more appropriate term would probably be SingleAssignmentCell, but that's a mouthful.

Ultimately, I doubt that there's a perfect solution for cell/sync naming, so the libs team would have to just decide on some particular naming scheme. I am personally ok with Sync prefix. The disadvantage of SyncX scheme is that it makes more common case longer to spell, and is a tautology (sync::SyncOnceCell).

I personally like the proposed cell::OnceCell / sync::OnceCell variant the most:

  • if you want extra clarity, you can qualify the type with cell:: or sync:: on the call site, instead of importing it
  • due to how awesome Rust is, it's impossible to use cell::OnceCell instead of sync::OnceCell, and the mistake in other direction is not really a mistake: just a very slight perf loss
  • in practice, I think it would be rare for a module to use cell and sync versions simultaneously.

This comment has been minimized.

Copy link
@nixpulvis

nixpulvis Oct 23, 2019

I think I like the way it's organized in the once_cell crate with explicit sync::* and unsync::* modules.

The part that feels the most strange to me about this currently proposal is that a OnceCell lives in both cell (makes sense, since it's a type of cell), and sync (makes sense, given that Once is also defined here). Something's conflated here, though this could well be outside the scope of this RFC. Either way, this is the first "cell" added to the sync module.

For another point of reference, we currently have Rc defined in std::rc and a similar Arc defined in std::sync. I might argue this would make more sense as std::rc::{Rc, sync::Arc}, which would solve my issue with OnceCell as well, though I don't expect this to be possible with backwards compatibility requirements.

This comment has been minimized.

Copy link
@matklad

matklad Oct 23, 2019

Author Member

Yeah, another possibility is to introduce something like lazy and lazy::sync modules. As it stands, std::sync today feels a bit like, well, kitchen sync, it has a bit of everything.

I can imagine an alternative world, where std::sync is only for synchronization primitives, atomic and mpsc are top-level modules and Arc lives in rc. In this world, having an std::lazy module would definitely be better.

With current stdlib structure, std::sync::OnceCell blends better with the background.

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin Oct 25, 2019

Contributor

Also, everything in std::cell is !Sync so it sort of maps to crates.io’s once_cell::unsync.

This comment has been minimized.

Copy link
@sfackler

sfackler Oct 25, 2019

Member

The libs team has discussed moving stuff out of std::sync into more appropriate modules (e.g. std::mutex::Mutex). I think ideally we wouldn't throw more stuff in there.

@pitdicker pitdicker referenced this pull request Oct 23, 2019
@darkstalker

This comment has been minimized.

Copy link

darkstalker commented Oct 26, 2019

I was expecting this to implement haskell-style lazy values. Can I use this outside global scope? Maybe use another more descriptive name? like LazyStatic

@matklad

This comment has been minimized.

Copy link
Member Author

matklad commented Oct 26, 2019

Yes, you can use this in non-static scope, see the very end of the guide section.

@bstrie

This comment has been minimized.

Copy link
Contributor

bstrie commented Oct 29, 2019

+1. Nay, such is my exuberance that I dare say, +2. Ever since finding out about once_cell I've considered it a slam-dunk for inclusion in the stdlib: it satisfies a ubiquitous use case, supplies a fundamental primitive, encapsulates tricky unsafety, has a small, self-contained, and obvious API, and supplants a macro solution with a more idiomatic approach.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Nov 4, 2019

I'd love to see this added to the standard library.

I share the concern that I'd prefer to not have two types with the same name distinguished only by module. I've run into far too many cases of annoying conflicts between io::Write and fmt::Write, and I'd like to not repeat that.

That issue aside, 👍.

@shepmaster

This comment has been minimized.

Copy link
Member

shepmaster commented Nov 4, 2019

I'd prefer to not have two types with the same name distinguished only by module

As a counter-bikeshed, I'm fine using modules to namespace types. When I need both, I import only the module or rename it:

fn foo(_: impl io::Write) -> impl fmt::Write {}
use io::Write as IoWrite;
use fmt::Write as FmtWrite;

fn foo(_: impl IoWrite) -> impl FmtWrite {}

Only when you need both (which my experience has shown to be rare) do you need the disambiguation.

@matklad

This comment has been minimized.

Copy link
Member Author

matklad commented Nov 4, 2019

I think Writes are especially problematic, because they are traits (with identically named write_fmt name and duck-typed format! to boot), and it's easier to confuse traits than types. As an anecdata-point, the result-alias idiom does not confuse me, while Write traits do.

My hypothesis is that cell::OnceCell and sync::OnceCell would not be as bad as io::Write / fmt::Write. I don't know of a strictly better naming scheme :)

As a cell can be empty or fully initialized, the proposed API does not use poisoning.
If an initialization function panics, the cell remains uninitialized.
An alternative would be to add poisoning, which will make all subsequent `get` calls to panic.

This comment has been minimized.

Copy link
@pitdicker

pitdicker Nov 5, 2019

Contributor

I would like to compare the reason Mutex and Once use poisoning with OnceCell.

  • Mutex is often used when multiple operations need to be done on some data to keep it in a consistent state. In that case it makes sense to poison the Mutex on panic, so other threads can know the data may now be inconsistent. It is not necessary for memory safety, but an extra safeguard.
  • std::sync::Once can also poison itself on panic. As it guarantees to run only once, it makes sense to 'report' it when that one execution failed.

So in both cases there is a good reason to report something went wrong. Is there such a reason for OnceCell?

  • We don't have to warn about possibly leaving things in an inconsistent state, as the OnceCell will simply remain empty on panic.
  • The basic guarantee that makes OnceCell safe is that there will only ever be one write to the wrapped value. That doesn't necessary guarantee that some closure will only be executed once: if it panics in one thread, another may give it another try.

So I think the question of whether poisoning on OnceCell and Lazy is desirable is tied to the question: do we want to warn if the initialization closure is run more than once? And how much may that warning cost?

As a guess I would say users of OnceCell mostly care about whether the value is initialized, and not if the initialization function is run exactly once.

So I wouldn't want to see a warning on every use unless you do some 'soothening'. Be it having OnceCell.get() returning None, or Lazy panicking on deref. That seems much to invasive.

But we could add the 'warning' with limited convenience overhead to OnceCell.get_or_init(), OnceCell.try_get_or_init(), and Lazy::new(). Just provide the closure that they take with a bool that indicates whether a previous attempt to initialize failed.

Example:

impl OnceCell {
    /* ... */

    pub fn get_or_init<F>(&self, f: F) -> &T
    where
        F: FnOnce(bool) -> T;
}
let cell = OnceCell::new();
// When you don't care about panicking, notice the extra `_`:
cell.get_or_init(|_| 21 + 21);
// If you want to handle it:
cell.get_or_init(|panicked| { if panicked { panic!(); 21 + 21 }});

This would be a breaking change for the current API of once_cell. But it seems like a nice and easy way for users to opt into stronger guarantees.

I think it would be good if the simple OnceCell.set() would just override a previous panic and initialize 'no matter what'.

B.t.w. the current behavior of lazy_static is different from once_cell. lazy_static will poison itself on panic, and has no way to recover.

This comment has been minimized.

Copy link
@matklad

matklad Nov 13, 2019

Author Member

I feel pretty strongly that, by default, we shouldn't expose knobs to tweak panicking behavior. Users of the API should not think about panics at all, unless they are in very specific circumstances.

So, I think we should either poison, or not poison, without any changes to the API (well, if we implement poisoning we might add some future-extension like force_init a-la Once, but that's definitely out of scope for the initial version).

I ... don't really know what would be the difference between poisoned and non-poisoned versions in practical usage. Abstract argument about the lack of invariants which could be broken favors non-poisoning. Consistency with all other std synchronization primitives favors poisoning.

@pitdicker pitdicker referenced this pull request Nov 5, 2019

```rust
fn set(&self, value: T) -> (&T, Option<T>);
```

This comment has been minimized.

Copy link
@pitdicker

pitdicker Nov 5, 2019

Contributor

Wouldn't returning either a reference or the original value in the Result make more sense?

fn set(&self, value: T) -> Result<&T, T>;

I like this signature more, because it states the guarantee, and can allow users to avoid a get right after set.

It does not really cost us anything either. set already has to check an atomic to see if the cell is initialized, and creating the reference can be optimized away if it is not used.

This comment has been minimized.

Copy link
@matklad

matklad Nov 6, 2019

Author Member

If the set fails, you still can get a reference back. That is, getting back back (old) &T and (new) T is a possible valid outcome of a blocking set operation

This comment has been minimized.

Copy link
@pitdicker

pitdicker Nov 7, 2019

Contributor

O of course.
Some example code if you want to set, and then get a value.

The variant in the main text of the RFC:

pub fn set(&self, value: T) -> Result<(), T>;

let _unused = my_cell.set(42);
let val = my_cell.get();

Alternative variant in the RFC:

fn set(&self, value: T) -> (&T, Option<T>);

let (val, _unused) = my_cell.set(42);

My alternative:

fn set(&self, value: T) -> Result<&T, T>;

let val = match my_cell.set(42) {
    Ok(val) => val,
    Err(_unused) => my_cell.get(),
}

Okay, I prefer the basic variant in the RFC 👍

It is possible to provide a non-blocking alternative to `set`:

```rust
fn try_set(&self, value: T) -> Result<&T, (Option<&T>, T)>

This comment has been minimized.

Copy link
@pitdicker

pitdicker Nov 5, 2019

Contributor

(about the optional extension try_set)
Wow, this is an ugly signature! As it does not establish the invariant that the cell is initialized, it seems better to not return a reference at all.

fn try_set(&self, value: T) -> Result<(), T>

That is, we can store some initial state in the cell and consume it during initialization.
In practice, such flexibility seems to be rarely required.
Even if we add a type, similar to `OnceFlipCell`, having a dedicated `OnceCell` (which *could* be implemented on top of `OnceFlipCell`) type simplifies a common use-case.

This comment has been minimized.

Copy link
@pitdicker

pitdicker Nov 5, 2019

Contributor

To be honest this alternative feels like a useless snowflake, maybe nice for a specialized crate. I think it is better to concentrate on the main insight: OnceCell is safe because it gets only written once.

This alternative OnceFlipCell that combines a OnceCell with an enum distracts from that, and imho the RFC is better off without it.

This comment has been minimized.

It is possible because, while `OnceCell` needs block for full API, its internal state can be implemented as a single `AtomicUsize`, so the `core` part does not need to know about blocking.
It is unclear if this API would be significantly useful.
In particular, the guarantees of non-blocking `set` are pretty weak, and are not enough to implement the `Lazy` wrapper.

This comment has been minimized.

Copy link
@pitdicker

pitdicker Nov 5, 2019

Contributor

First this post on reddit seems relevant:

I am not an expert in the things that people use #[no_std] for, but my hypothesis is that, to make a robust #[no_std] library, one needs some more invasive changes than just swapping all mutexes for spin locks. I think that it is totally possible that a no_std target has some support for real blocking or preemption, and using spinning in this case seems wrong.

A potential disaster movie scenario is this: there's no_std-only crate A which uses once_cell with default_features = false. An application B, which uses std and runs on linux, uses A. No other crate depends on once_cell, so its std feature is not activated, and it uses spinlock. A thread of B gets preempted while in get_or_init called from A. Another thread of B calls into A again, and is stuck busy-waiting. Granted, a lot of stars need to align to make this really bad, but the possibility still exists.

On the other hand, if you are writing a no_std library, there's a high chance that you actually don't need blocking at all: in no_std your opportunities are limited, and whatever you do probably fits into lock free initialization. rand/#50 is a good example of this.

If you are writing a no_std crate, and you are absolutely sure that you need blocking, I think it's best to parametrize your library by the Mutex trait, so that the users of your crate might plug it into whichever blocking facilities the host environment provides.

I wonder if spinning is always such a bad idea. OnceCell::get_or_init takes a closure that may take arbitrary long to execute, with the guarantee that is not running concurrently on another thread. But what if the initialization function is cheap and running it more than once doesn't matter? Then a thread could just start to run the closure first, and only spin for a few moments while the value is memcpy'd into place.

This would be pretty much the same as how a library would have to use try_set if it wants to support no_std, but knows as little about the target as OnceCell:

let cell = OnceCell::new();
let my_cheap_initialization = 21 + 21;
cell.try_set(my_cheap_initialization);
while cell.get().in_none() {}

This wouldn't combine well with OnceCell, because one thread could use our hypothetical initialization function, while another thread could do something else. But it is an option for Lazy, were the method of initialization is fixed at creation time:

impl<T, F> Lazy<T, F> {
    /// Creates a new lazy value with the given initializing function.
    pub const fn new(init: F) -> Lazy<T, F>;

    /// Creates a new lazy value with the given initializing function.
    /// The function may be executed by more than one thread at once, only the
    /// result of the first one to finish will be used.
    /// can be used in no_std.
    pub const fn new_concurrent(init: F) -> Lazy<T, F>;
}

This is just an idea, and a compromise. But as some no_std solution seems highly desired (see lazy_static), maybe something to consider.

This comment has been minimized.

Copy link
@pitdicker

pitdicker Nov 5, 2019

Contributor

Or should we just say: if you want to use sync::OnceCell and sync::Lazy use crate x.

Then make that crate depend on something like the thread_parker implementation, something that knows how to park/unpark threads without the standard library on different platforms and (maybe in the future) different devices.

This comment has been minimized.

Copy link
@matklad

matklad Nov 6, 2019

Author Member

But what if the initialization function is cheap and running it more than once doesn't matter?

Yeah, this is an alternative design. If you are potentially ready to run initialization function several times and through away the result, you can even make this lock free, by boxing a value and atomically swapping the pointer.

This is a valuable, but different, primitive. For std, I think it makes sense to lean towards more simple semantics, and guarantee that, while one thread runs initialization, all other block.

/// remains uninitialized.
///
/// It is an error to reentrantly initialize the cell from `f`. Doing
/// so results in a panic or a deadlock.

This comment has been minimized.

Copy link
@pitdicker

pitdicker Nov 5, 2019

Contributor

It may be obvious, but not to me: it is impossible to detect reentrant initialization always and guarantee a panic.

Detecting reentrancy requires a way to know the difference between another thread that wants to initialize concurrently, and a reentrant initialization function.

Now it is not all that hard to detect if the current thread is trying to initialize the OnceCell recursively. Just compare the thread id (or something else unique, like a the address of thread local static) with the id stored when initialization begun.

But if the initialization function spawns other threads, I see no good way to differentiate between concurrent and reentrant initialization.

Still, it seems nice to be able to give the guarantee that reentrant initialization will result in a panic as long as the initialization function doesn't involve other threads.

DrSensor added a commit to darkpools/darkpools_publicapi_ws_ticker that referenced this pull request Nov 7, 2019
see:
- Geal/nom#1058 (comment) (especially on async-std issue)
- rust-lang-nursery/lazy-static.rs#111
- rust-lang/rfcs#2788 (ongoing RFC)
DrSensor added a commit to darkpools/darkpools_publicapi_ws_ticker that referenced this pull request Nov 8, 2019
* refactor(config): replace match with unwrap_or

* refactor: replace lazy_static with once_cell

see:
- Geal/nom#1058 (comment) (especially on async-std issue)
- rust-lang-nursery/lazy-static.rs#111
- rust-lang/rfcs#2788 (ongoing RFC)

* refactor: remove inline attribute

There is several I/O operations in inlined function

* ci: add workflow for uploading build release

* ci: build and push nightly docker image

* ci: cache build release dependencies

* ci(docker): change image name

Can't publish same image name on different repository within same organization

note: I've sent request for deletion

* ci(docker): only publish on master branch

* fix: lint error
@matklad

This comment has been minimized.

Copy link
Member Author

matklad commented Nov 9, 2019

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Nov 10, 2019
…nieu

Refactor sync::Once

`std::sync::Once` contains some tricky code to park and unpark waiting threads. [once_cell](https://github.com/matklad/once_cell) has very similar code copied from here. I tried to add more comments and refactor the code to make it more readable (at least in my opinion). My PR to `once_cell` was rejected, because it is an advantage to remain close to the implementation in std, and because I made a mess of the atomic orderings. So now a PR here, with similar changes to `std::sync::Once`!

The initial goal was to see if there is some way to detect reentrant initialization instead of deadlocking. No luck there yet, but you first have to understand and document the complexities of the existing code 😄.

*Maybe not this entire PR will be acceptable, but I hope at least some of the commits can be useful.*

Individual commits:

#### Rename state to state_and_queue
Just a more accurate description, although a bit wordy. It helped me on a first read through the code, where before `state` was use to encode a pointer in to nodes of a linked list.

#### Simplify loop conditions in RUNNING and add comments
In the relevant loop there are two things to be careful about:
- make sure not to enqueue the current thread only while still RUNNING, otherwise we will never be woken up (the status may have changed while trying to enqueue this thread).
- pick up if another thread just replaced the head of the linked list.

Because the first check was part of the condition of the while loop, the rest of the parking code also had to live in that loop. It took me a while to get the subtlety here, and it should now be clearer.

Also call out that we really have to wait until signaled, otherwise we leave a dangling reference.

#### Don't mutate waiter nodes
Previously while waking up other threads the managing thread would `take()` out the `Thread` struct and use that to unpark the other thread. It is just as easy to clone it, just 24 bytes. This way `Waiter.thread` does not need an `Option`, `Waiter.next` does not need to be a mutable pointer, and there is less data that needs to be synchronised by later atomic operations.

#### Turn Finish into WaiterQueue
In my opinion these changes make it just a bit more clear what is going on with the thread parking stuff.

#### Move thread parking to a seperate function
Maybe controversial, but with this last commit all the thread parking stuff has a reasonably clean seperation from the state changes in `Once`. This is arguably the trickier part of `Once`, compared to the loop in `call_inner`. It may make it easier to reuse parts of this code (see rust-lang/rfcs#2788 (comment)). Not sure if that ever becomes a reality though.

#### Reduce the amount of comments in call_inner
With the changes from the previous commits, the code pretty much speaks for itself, and the amount of comments is hurting readability a bit.

#### Use more precise atomic orderings
Now the hard one. This is the one change that is not anything but a pure refactor or change of comments.

I have a dislike for using `SeqCst` everywhere, because it hides what the atomics are supposed to do. the rationale was:
> This cold path uses SeqCst consistently because the performance difference really does not matter there, and SeqCst minimizes the chances of something going wrong.

But in my opinion, having the proper orderings and some explanation helps to understand what is going on. My rationale for the used orderings (also included as comment):

When running `Once` we deal with multiple atomics: `Once.state_and_queue` and an unknown number of `Waiter.signaled`.
* `state_and_queue` is used (1) as a state flag, (2) for synchronizing the data that is the result of the `Once`, and (3) for synchronizing `Waiter` nodes.
    - At the end of the `call_inner` function we have to make sure the result of the `Once` is acquired. So every load which can be the only one to load COMPLETED must have at least Acquire ordering, which means all three of them.
    - `WaiterQueue::Drop` is the only place that may store COMPLETED, and must do so with Release ordering to make result available.
    - `wait` inserts `Waiter` nodes as a pointer in `state_and_queue`, and needs to make the nodes available with Release ordering. The load in its `compare_and_swap` can be Relaxed because it only has to compare the atomic, not to read other data.
    - `WaiterQueue::Drop` must see the `Waiter` nodes, so it must load `state_and_queue` with Acquire ordering.
    - There is just one store where `state_and_queue` is used only as a state flag, without having to synchronize data: switching the state from INCOMPLETE to RUNNING in `call_inner`. This store can be Relaxed, but the read has to be Acquire because of the requirements mentioned above.
* `Waiter.signaled` is both used as a flag, and to protect a field with interior mutability in `Waiter`. `Waiter.thread` is changed in `WaiterQueue::Drop` which then sets `signaled` with Release ordering. After `wait` loads `signaled` with Acquire and sees it is true, it needs to see the changes to drop the `Waiter` struct correctly.
* There is one place where the two atomics `Once.state_and_queue` and `Waiter.signaled` come together, and might be reordered by the compiler or processor. Because both use Aquire ordering such a reordering is not allowed, so no need for SeqCst.

cc @matklad
seamlik added a commit to seamlik/riko that referenced this pull request Nov 15, 2019
`lazy_init` has a bug that prevents applying attributes on the static variables.

`once_cell` is being included in the standard library. (rust-lang/rfcs#2788)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.