Major-release candidate #122

ticki · Mar 10, 2017

Changes

(was that all?)

Right now, I'm going to get some sleep. I don't have much time tomorrow, so I request someone to help me with this: The tests are broken, from what I suspect to be a simple bug in CachePadded, but I'm too tired to figure it out.

sstewartgallus · Mar 11, 2017

sstewartgallus suggested changes Mar 11, 2017

View changes

Too many useless comments. Specify why not what.

sstewartgallus · Mar 11, 2017

src/cache_padded.rs

    pub fn new(t: T) -> CachePadded<T> {
+        // Assert the validity.


Useless comment.

It's not all "useless comments", it's @ticki's coding style. It's explained in the internals thread corresponding to this PR: https://internals.rust-lang.org/t/crossbeam-request-for-help/4933/16.

After the experience of working with a huge undocumented code base, I do appreciate this style very much. And even if those comments are directly deducible from the code, having a human readable comment still improves the readability.

@konstin "it's somebodies coding style" is not an argument. It directly impedes readability to labal things redundantly. Any piece of the source code, even if it is a comment, should pay its weight or go to the chopping block. Most software source code metrics such as cyclomatic complexity and number of bugs directly correlate with the numbers of line of code. More code means more room for bugs and out of date documentation.

I agree with @konstin. Concurrency code can benefit from extensive documentation, higher granularity than usual if necessary.

Most software source code metrics such as cyclomatic complexity and number of bugs directly correlate with the numbers of line of code.

I don't see how comments would contribute to cyclomatic complexity and code complexity in general.

More code means more room for bugs and out of date documentation.

So conversely, zero documentation means no room for being out-of-date? There is obviously a balance here. With core, concurrency related code, I prefer heavy commenting. Even tiny things can have significance. Also, out-of-date comments should be less of an issue as refactoring core concurrency code should be infrequent and heavily reviewed anyway.

Just my 2c.

I generally agree with @drewhk that extensive documentation helps a lot, especially for concurrency code. But for this specific comment, I agree with @sstewartgallus : Assert the validity. doesn't give us additional information than the code itself (assert_valid::<T>();). Surely it would be very helpful to write down the reason why we need to assert the validity.

Sure, there is a discussion to be had about the right balance here ;) I just wanted to voice my concern about blanket statements about comment granularity.

sstewartgallus · Mar 11, 2017

src/cache_padded.rs

        assert_valid::<T>();
+
+        // Construct the (zeroed) type.


sstewartgallus · Mar 11, 2017

src/cache_padded.rs

        };
+
+        // Copy the data into the untyped buffer.


sstewartgallus · Mar 11, 2017

src/epoch/atomic.rs

+/// `None` maps to the null pointer.
+#[inline]
+fn opt_shared_into_raw<T>(val: Option<Shared<T>>) -> *mut T {
+    // If `None`, return the null pointer.


sstewartgallus · Mar 11, 2017

src/epoch/atomic.rs

+/// `None` maps to the null pointer.
+#[inline]
+fn opt_box_into_raw<T>(val: Option<Box<T>>) -> *mut T {
+    // If `None`, return the null pointer.


sstewartgallus · Mar 11, 2017

src/sync/treiber_stack.rs

                            guard.unlinked(head);
+                            // Read the data.


Useless comment.

sstewartgallus · Mar 11, 2017

src/sync/treiber_stack.rs

                        unsafe {
+                            // Unlink the head node from the epoch.


Useless comment.

sstewartgallus · Mar 11, 2017

src/sync/treiber_stack.rs

                    let next = head.next.load(Relaxed, &guard);
-                    if self.head.cas_shared(Some(head), next, Release) {
+                    // Set the node to the tail node, unless it changed (ABA condition).


Useless comment.

sstewartgallus · Mar 11, 2017

src/sync/treiber_stack.rs

                None => return None,
            }
        }
    }

    /// Check if this queue is empty.
    pub fn is_empty(&self) -> bool {
+        // Pin the epoch.


Useless comment.

sstewartgallus · Mar 11, 2017

src/sync/treiber_stack.rs

        let guard = epoch::pin();
+        // Test if the head is a null pointer.


Useless comment.

schets · Mar 11, 2017

schets reviewed Mar 11, 2017

View changes

schets · Mar 11, 2017

src/epoch/participant.rs

+    /// call).
+    pub fn enter(&self) -> bool {
+        // Increment the counter.
+        if self.in_critical.fetch_add(1, atomic::Ordering::Relaxed) > 0 {


I expect this would make pinning more expensive. On Intel, you've essentially replaced a load->store->mfence sequence with a lock xadd -> mfence sequence, which isn't too different than a mfence->mfence sequence. Maybe the second fence is fast and can be ordered around since the previous fence already blocked everything, but it seems inefficient at first glance. The story is better but not great on ll/sc architectures. Since it's not a big change in clarity over load->add->store I think it should get changed back.

pitdicker · Mar 11, 2017

pitdicker reviewed Mar 11, 2017

View changes

pitdicker · Mar 11, 2017

src/epoch/mod.rs

+//! - When the thread subsequently reads from a lock-free data structure, the pointers it extracts
+//!   act like references with lifetime tied to the `Guard`. This allows threads to safely read
+//!   from snapshotted data, being guaranteed that the data will remain allocated until they exit
+//!   the epoch.
 //!
 //! To put the `Guard` to use, Crossbeam provides a set of three pointer types meant to work together:
 //!


Now there are only two pointer types?

arthurprs · Mar 11, 2017

arthurprs requested changes Mar 11, 2017

View changes

I did a first pass and the code looks good.

arthurprs · Mar 11, 2017

src/epoch/garbage.rs

+        //       think, especially if several threads constantly creates new garbage).
+        loop {
+            // Load the head.
+            let head = self.head.load(atomic::Ordering::Acquire);


I don't see the problem, it loads the head to set n.next then tries to replace head with n.

arthurprs · Mar 11, 2017

src/sync/arc_cell.rs

-    }
-}
-
-impl<T> ArcCell<T> {


Very useful primitive, it should be in crate. If broken we can fix it.

arthurprs · Mar 11, 2017

src/sync/ms_queue.rs

-    /// Returns `None` if the queue is observed to be empty.
-    pub fn try_pop(&self) -> Option<T> {
+    /// This returns `None` if the queue is observed to be empty.
+    pub fn dequeue(&self) -> Option<T> {


I see no reason not to stick with the standard {try_}{push/pop} here.

arthurprs · Mar 11, 2017

src/sync/treiber_stack.rs

-            data: t,
+    /// Push an element on top of the stack.
+    pub fn push(&self, elem: T) {
+        // Construct the node.


Overall I'd refrain from these comments, unless the comment is badly worded or incorrect. The code review surface is already huge to start nitpicking these.

arthurprs · Mar 11, 2017

src/sync/chase_lev.rs

-/// There may only be one worker per deque, and operations on the worker
-/// require mutable access to the worker itself.
-#[derive(Debug)]
-pub struct Worker<T> {


We need to discuss some sort of guidelines.

Amanieu · Mar 11, 2017

I have two general comments to make about the Atomic type:

Consider basing the CAS function on the newer compare_exchange API instead of the deprecated compare_and_swap one. The newer API allows you to specify a memory ordering on CAS failure and allows for weak CAS (compare_exchange_weak) which can be more efficient on certain architectures.
Could we maybe rename Atomic to something more descriptive like AtomicSharedPtr? The name Atomic somewhat conflicts with my atomic crate which provides a generic Atomic type.

Vtec234 · Mar 11, 2017

Vtec234 reviewed Mar 11, 2017

View changes

Vtec234 · Mar 11, 2017

src/epoch/participants.rs

+    /// The next node.
+    next: &'a Atomic<ParticipantNode>,
+    // Has `self.next()` **not** been called before?
+    first: bool,


From a Reddit comment by /u/dbaupp: first is never set back to false.

Vtec234 · Mar 11, 2017

Given the size and complexity of this whole thing, I've come to think that it might be better if @ticki could split this into several smaller PRs. It's barely been reviewed and we're already crawling through an 80-comment thread. A potential organisation:

Changes to EBR. Especially this need explanation, did you just refactor it and add documentation or did you make changes to algorithms? This PR should not include moving the module and merging/splitting files, since the lack of diffs makes this especially difficult to review.
Move epoch module, after above
Add compare-and-swap, rename cas
Add ability to create Shared from Guard, add Pinned
Remove Owned and AtomicOption
Remove ArcCell, Chase-Lev Deque and Scala benchmark
Other fixes and addition I missed

What do people think?

ticki · Mar 11, 2017

@Vtec234 I like that. It's definitely a mistake of mine to make such a big PR with a lot of unrelated changes.

ticki · Mar 11, 2017

btw, I've enabled such that all with write access can add comments

If checked, users with write access to crossbeam-rs/crossbeam can add new commits to your master branch. You can always change this setting later.

Please feel free to use.

gnzlbg · Mar 12, 2017

Are the test running with the thread sanitizer already on CI by default already?

jeehoonkang · Mar 14, 2017

I strongly agree with @Vtec234 's suggestion to split this PR (#122 (comment)). I would like someone to close this PR, and make a tracking issue for each change item mentioned above.

Having said about "someone", I feel this project currently lacks a proper decision making procedure, even for closing issues and merging PRs :\ For now I would like to ask if @ticki could provide leadership for this PR (which happens to be currently the most important one). If you are in short of time, please kindly tell me what you want me to do :) Of course, for a longer term, we eventually need to establish some institution.

ticki · Mar 14, 2017

@jeehoonkang

If you are in short of time, please kindly tell me what you want me to do :) Of course, for a longer term, we eventually need to establish some institution.

I really want some help with one thing in particular: The bug in CachePadded<T>, which causes most tests to fail. I suspect it's just some stupid bug, but having looked at the code too long makes it really hard to get new insides. I'd like someone to help me figure out why it is that assert_valid panics. The CachePadded is pretty hacky, so maybe it's just some change in memory layout.

Either way it's hard for me to do alone.

aturon · Mar 14, 2017

@jeehoonkang

Having said about "someone", I feel this project currently lacks a proper decision making procedure, even for closing issues and merging PRs

See #124 for the time being. We'll have to play it a bit by ear for a while, I think, and gradually formalize a procedure.

jeehoonkang · Mar 15, 2017

jeehoonkang reviewed Mar 15, 2017

View changes

jeehoonkang · Mar 15, 2017

src/mem/cache_padded.rs

+/// Do not let your code's safety or correctness depend on this being the real size. The actual
+/// value may not be equal to this.
+// TODO: For now, treat this as an arch-independent constant.
+const CACHE_LINE: usize = 64;


In x86-64, the size of CachePadded<T> changed from 256 (sizeof(usize) * CACHE_LINE = 8 * 32 = 256) to 64 (sizeof(u8) * CACHE_LINE = 1 * 64 = 64). But 64 bytes are not sufficient: e.g. Participant, which is fed to CachePadded, is 104 bytes big. That causes all the test failures.

The tests still fail for other reasons even after fixing this by setting const CACHE_LINE: usize = 256;, though..

Holy fuck! So simple.

The tests still fail for other reasons even after fixing this by setting const CACHE_LINE: usize = 256;, though..

All the tests run for me...

Oh, no. I might be wrong.

Yep. I was one the wrong HEAD.

arthurprs · Mar 22, 2017

I spend a few hours on this with no success, most commits left the build broken and then a latter commit fixes it. So git bisect was useless. I tried getting each commit compiling but after a few hours I gave up, but that's probably the only way to find it without eyeballing or some magical debug powers.

whataloadofwhat · Mar 22, 2017

Use lazy_static rather than manual implementation of it.

Why? lazy_static blocks in its implementation. Using a locking algorithm to implement lock-free data structures seems counter-intuitive, even if it is unlikely to cause an issue. The manual implementation was optimistic and lock-free, which matches the ideology of the crate a lot better I think.

Get rid of Owned<T> in favour of Box<T>

Not sure I agree with this. Owned<T> is currently implemented as a Box<T> but that's an implementation detail that I find worrying. Realistically, Owned<T> is akin to Box<ManualDrop<T>>. I'd much rather the code work with this rather than just assume that Vec<T> with len 0 and cap 1 is the same as just freeing the memory of Box<T>. It feels hacky and confusing in a codebase that is already difficult to follow (not anybody's fault, concurrency is hard).

Add owned guards through Pinned.

I feel like this is a mistake. It's not unsafe or anything, I just think people will use it incorrectly.

Add map to Shared<T>, allowing internal referencing with owning_ref.

Not sure about this either. guard::unlinked will be just plain wrong with a mapped Shared<T>. Seems like a potential footgun. &T can be mapped just fine and Shared<T> will deref into that anyway.

ticki · Mar 22, 2017

@arthurprs

I spend a few hours on this with no success, most commits left the build broken and then a latter commit fixes it. So git bisect was useless. I tried getting each commit compiling but after a few hours I gave up, but that's probably the only way to find it without eyeballing or some magical debug powers.

It's my horrible habit: Just write and then fix when you're done. Makes it hard to split up 😕.

@whataloadofwhat

Why? lazy_static blocks in its implementation. Using a locking algorithm to implement lock-free data structures seems counter-intuitive, even if it is unlikely to cause an issue. The manual implementation was optimistic and lock-free, which matches the ideology of the crate a lot better I think.

The previous implementation blocked as well. AFAICS there is no non-blocking way of doing this, and the blocking part is only during initialization and even then is rare.

Not sure I agree with this. Owned is currently implemented as a Box but that's an implementation detail that I find worrying. Realistically, Owned is akin to Box<ManualDrop>. I'd much rather the code work with this rather than just assume that Vec with len 0 and cap 1 is the same as just freeing the memory of Box. It feels hacky and confusing in a codebase that is already difficult to follow (not anybody's fault, concurrency is hard).

That is, well, very weird thing. Like, of course it calls the inner destructor, and even the free is more sensical than the vector hack. If you want a changed destructor, you can just wrap your T in a newtype.

I feel like this is a mistake. It's not unsafe or anything, I just think people will use it incorrectly.

I see literally no way of 'misusing' this?

Not sure about this either. guard::unlinked will be just plain wrong with a mapped Shared. Seems like a potential footgun. &T can be mapped just fine and Shared will deref into that anyway.

This one is interesting. I don't think it is an issue in reality, as unlinked is already unsafe.

whataloadofwhat · Mar 22, 2017

The previous implementation blocked as well. AFAICS there is no non-blocking way of doing this, and the blocking part is only during initialization and even then is rare.

Maybe blocking was the wrong word? I'm sorry. The manual implementation is lock-free. lazy_static is not.

I see literally no way of 'misusing' this?

Pinned<T> is lifetime free which makes it very tempting to just pass around and store, but doing so is going to inhibit garbage collection. Like I say, it's not unsafe, it's just ... encouraging bad patterns, I think. You shouldn't really be storing a Guard for a long period of time and Pinned<T> kind of encourages you to do so.

This one is interesting. I don't think it is an issue in reality, as unlinked is already unsafe.

This is true, but it's expanding the scope in which it can be used incorrectly. I don't think it should be done idly, but I don't see much of a benefit to it either. The old invariant is "only used on Shared<T> where no new readers are going to be created". You've now created a new invariant, "ensure the Shared<T> has not been mapped".

That is, well, very weird thing. Like, of course it calls the inner destructor, and even the free is more sensical than the vector hack. If you want a changed destructor, you can just wrap your T in a newtype.

I think I may have caused some confusion here so I'll drop this point. Sorry.

ticki · Mar 22, 2017

Pinned is lifetime free which makes it very tempting to just pass around and store, but doing so is going to inhibit garbage collection. Like I say, it's not unsafe, it's just ... encouraging bad patterns, I think. You shouldn't really be storing a Guard for a long period of time and Pinned kind of encourages you to do so.

That's a good point. It can be tempting to store, say, in a struct.

jeehoonkang · Apr 4, 2017

I am closing this, as (1) we decided to split this PR into smaller ones, and (2) there is not so much traffic on this PR these days.

As a follow-up, I will make the smaller PRs of the changes made in this PR. As noted above, the commits are quite mixed so that it will be hard to maintain the commits as-is. I will make new commits based on the diffs.

arthurprs · Apr 4, 2017

@jeehoonkang thanks for picking this up. I'll do my best to review in a timely manner.

Fix bug in `participants::Participant::Iter<'a>` noticed by dbaupp.

Unverified

The key that signed this is expired.

GPG key ID: 28F8864B5C973CB6 Learn about signing commits

0202bd5

jeehoonkang closed this Apr 4, 2017

jeehoonkang referenced this pull request Apr 4, 2017
Merged
Move and remove files #127

jeehoonkang added a commit to jeehoonkang/crossbeam that referenced this pull request Apr 6, 2017

Clean up `atomic.rs` in epoch
Many improvements are adopted from crossbeam-rs#122 by @ticki.

752cab7

This was referenced Apr 6, 2017

Closed

Clean up `atomic.rs` in `epoch` #128

Closed

Revise CachePadded #130

jeehoonkang added a commit to jeehoonkang/crossbeam that referenced this pull request Apr 7, 2017

Clean up `atomic.rs` in epoch
Many improvements are adopted from crossbeam-rs#122 by @ticki.

ffc224e

jeehoonkang referenced this pull request Jul 31, 2017
Merged
Implement epoch management #4

jeehoonkang referenced this pull request Aug 28, 2017
Closed
`lazy_static` is blocking #10

crossbeam-rs/crossbeam

Join GitHub today

Major-release candidate #122

Conversation

ticki commented Mar 10, 2017 • edited by arthurprs

canndrew and others added some commits Apr 18, 2016

sstewartgallus suggested changes Mar 11, 2017 View changes

sstewartgallus left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

schets reviewed Mar 11, 2017 View changes

This comment has been minimized.

pitdicker reviewed Mar 11, 2017 View changes

This comment has been minimized.

arthurprs requested changes Mar 11, 2017 View changes

arthurprs left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Amanieu commented Mar 11, 2017

Vtec234 reviewed Mar 11, 2017 View changes

This comment has been minimized.

This comment has been minimized.

Vtec234 commented Mar 11, 2017

This comment has been minimized.

ticki commented Mar 11, 2017

This comment has been minimized.

ticki commented Mar 11, 2017

This comment has been minimized.

gnzlbg commented Mar 12, 2017

This comment has been minimized.

jeehoonkang commented Mar 14, 2017

This comment has been minimized.

ticki commented Mar 14, 2017

This comment has been minimized.

aturon commented Mar 14, 2017

jeehoonkang reviewed Mar 15, 2017 View changes

This comment has been minimized.

jeehoonkang Mar 15, 2017 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

arthurprs commented Mar 22, 2017 • edited

This comment has been minimized.

whataloadofwhat commented Mar 22, 2017

This comment has been minimized.

ticki commented Mar 22, 2017 • edited

This comment has been minimized.

whataloadofwhat commented Mar 22, 2017

This comment has been minimized.

ticki commented Mar 22, 2017

This comment has been minimized.

jeehoonkang commented Apr 4, 2017

jeehoonkang closed this Apr 4, 2017

This comment has been minimized.

arthurprs commented Apr 4, 2017

jeehoonkang referenced this pull request Apr 4, 2017

jeehoonkang added a commit to jeehoonkang/crossbeam that referenced this pull request Apr 6, 2017

This was referenced Apr 6, 2017

jeehoonkang added a commit to jeehoonkang/crossbeam that referenced this pull request Apr 7, 2017

jeehoonkang referenced this pull request Jul 31, 2017

jeehoonkang referenced this pull request Aug 28, 2017

ticki commented Mar 10, 2017 •

edited by arthurprs

sstewartgallus suggested changes Mar 11, 2017

View changes

schets reviewed Mar 11, 2017

View changes

pitdicker reviewed Mar 11, 2017

View changes

arthurprs requested changes Mar 11, 2017

View changes

Vtec234 reviewed Mar 11, 2017

View changes

jeehoonkang reviewed Mar 15, 2017

View changes

jeehoonkang Mar 15, 2017 •

edited

arthurprs commented Mar 22, 2017 •

edited

ticki commented Mar 22, 2017 •

edited