Skip to content

Simple Arc implementation (without Weak refs) #253

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

Merged
merged 6 commits into from
Jan 18, 2021

Conversation

ThePuzzlemaker
Copy link
Contributor

This is a simple implementation of Arc, without weak refs. If there's anything you feel that could be improved (or if I did anything wrong), please tell me. I'm open to recommendations 🙂

Copy link
Contributor

@Havvy Havvy left a comment

Choose a reason for hiding this comment

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

Awesome that somebody is adding such a huge change; I just did a cursory look (this is not my area of knowledge), and I wasn't even going to comment, but one thing caught my eye so hard I had to.

@ThePuzzlemaker ThePuzzlemaker requested a review from Havvy January 1, 2021 19:33
Copy link
Contributor

@Havvy Havvy left a comment

Choose a reason for hiding this comment

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

Saying it's from 1.49 sounds good to me.

Havvy
Havvy previously approved these changes Jan 2, 2021
@Havvy Havvy dismissed their stale review January 2, 2021 01:39

Was just for a nit

This is a squash of the following commits:
- Fix code, remove WIP message as that was while writing this, and link to stable @ fixed 1.49 rather than latest nightly
- Improve wording on deref and ignore some code blocks
- Improve wording and formatting a bit cause I'm insane
- Fix links
- Fix links again because we all love relative links
- Remove unnecessary Drop import
- Use Box::from_raw instead of ptr::drop_in_place as that actually dealloc's the Box (i'm dumb and misinterpreted the std code :/); fix some desync between code in between sections
- Fix tests
Copy link
Contributor

@Gankra Gankra left a comment

Choose a reason for hiding this comment

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

Overall this is really great, but it definitely needs some revision.


We then need to create an atomic fence to prevent reordering of the use of the
data and deletion of the data. As described in [the standard library's
implementation of `Arc`][3]:
Copy link
Contributor

Choose a reason for hiding this comment

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

and change this to something like:


What atomic ordering should we use here? To know that, we need to consider what happens-before relationships we want to ensure (or alternatively, what dataraces we want to prevent). Drop is special because it's the one place where we mutate the Arc's payload (by dropping and freeing it). This is a potential read-write data race with all the other threads that have been happily reading the payload without any synchronization.

So we need to ensure all those non-atomic accesses have a proper happens-before relationship with us dropping and freeing the payload. To establish happens-before relationships with non-atomic accesses, we need (at least) Acquire-Release semantics.

As a reminder, Acquires ensure non-atomic accesses after them on the same thread stay after them (they happen-before everything that comes after them) and Releases ensure non-atomic accesses before them on the same thread stay before them (everything before them happens-before them).

So we have many threads that look like this:

(A) non-atomic accesses to payload
(B) atomic decrement refcount

And a "final" thread that looks like this:

(C) non-atomic accesses to payload
(D) atomic decrement refcount
(E) non-atomic free/drop contents

And we want to ensure every thread agrees that everything happens-before E.

One thing that jumps out clearly is that the non-final threads all end with an atomic access (B), and we want to keep everything else (A) before it. That's exactly what a Release does! So it seems we'd like our atomic decrement to be a Release.

if self.inner().rc.fetch_sub(1, Ordering::Release) != 1 {
  return;
}

However this on its own doesn't work -- our final thread would also use a Release, and that means (E) would be allowed to happen-before (D)! To prevent this we need Release's partner, an Acquire! We could make (D) AcquireRelease (AcqRel), but this would penalize all the other threads performing (B). So instead, we will introduce a separate Acquire that only happens if we're the final thread. And since we've already loaded all the values we need, we can use a fence.

if self.inner().rc.fetch_sub(1, Ordering::Release) != 1 {
  return;
}
atomic::fence(Ordering::Acquire);

If this helps, you can think of this like a sort of implicit RWLock: every Arc is a ReadGuard which allows unlimited read access until they go away and "Release the lock" (trapping all accesses on that thread before that point). The final thread then upgrades itself to a WriteGuard which "Acquires the lock" (creating a new critical section which strictly happens after everything else).

Copy link
Member

Choose a reason for hiding this comment

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

However this on its own doesn't work -- our final thread would also use a Release, and that means (E) would be allowed to happen-before (D)!

This sounds like for X, Y we always have that one happens-before the other or vice versa... which is not the way happens-before actually works. Happens-before is a partial order, and some events simply are unordered. So I think it'd be better to phrase this accordingly, saying something like "[...] and that means (D) would not be forced to happen-before (E)". Except that's also wrong, program-order is included in happens-before. The actual issue is between (E) and (A), isn't it? We must make (A) happens-before (E), and that's why there needs to be an "acquire" in the "final" thread.

Copy link
Member

Choose a reason for hiding this comment

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

As a reminder, Acquires ensure non-atomic accesses after them on the same thread stay after them (they happen-before everything that comes after them) and Releases ensure non-atomic accesses before them on the same thread stay before them (everything before them happens-before them).

The key thing with an acquire is that the release it reads from (and everything that comes before it) happens-before everything that comes after the acquire. Each acquire is paired with a release, and this pair establishes a happens-before link across threads. Personally, I find this way of thinking about it easier than thinking about the release and the acquire separately. (Also see what I said above: to my knowledge, happens-before includes program-order, so "X happens-before everything that comes after it in the same thread" is true for all X.)

@Gankra
Copy link
Contributor

Gankra commented Jan 8, 2021

(probably won't have a chance to review the new changes until monday)

Copy link
Contributor

@Gankra Gankra left a comment

Choose a reason for hiding this comment

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

Looks good!

ThePuzzlemaker and others added 2 commits January 17, 2021 18:24
@Gankra Gankra merged commit 1713e9f into rust-lang:master Jan 18, 2021
@Gankra
Copy link
Contributor

Gankra commented Jan 18, 2021

thanks a bunch!!

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.

6 participants