Skip to content
This repository has been archived by the owner on Nov 5, 2018. It is now read-only.

Embed Block messages #101

Merged
4 commits merged into from
Oct 4, 2018
Merged

Embed Block messages #101

4 commits merged into from
Oct 4, 2018

Conversation

kleimkuhler
Copy link
Contributor

@kleimkuhler kleimkuhler commented Oct 3, 2018

Closes #81

This builds off #100, and reverts back to the use of UnsafeCell within Slot<T>.

There are a few things I would like to discuss in this PR:

  • I changed the naming of slots to msgs. I don't have a strong preference on naming, but found myself thinking of that field as messages anyways so I did it for clarity.
  • I am unsure if msgs needs to be type UnsafeCell<Slot<T>>; 0]. The type [Slot<T>; 0] still passes, and allows a few get()s to be removed. I see the importance of msg in Slot<T> being UnsafeCell, but it's not as clear in this situation.
  • There is a new msg_count field in Block<T>. Do we want to consider embedding msg_count in start_index and just do some bitwise operations to retrieve the respective values? It would decrease the memory size of a Block<T> but also limits the size that start_index can be (which will still be large).

Any other feedback is welcome!

Currently not all tests finish. I still need to figure out why. It may
be in regards to the `fn defer_destroy` at list.rs:340
@ghost
Copy link

ghost commented Oct 3, 2018

This looks pretty good! :)

Regarding block allocation and deallocation, it seems we always turn a *mut Block<T> into a Shared<Block<T>> right after allocation, and we only destroy blocks in Shared<Block<T>> by turning them into *mut Block<T>. I therefore suggest to change the signatures like this:

impl<T> Block<T> {
    fn new<'a>(start_index: usize, msg_count: usize) -> Shared<'a, Block<T>>;
    unsafe fn destroy(ptr: Shared<Block<T>>);
}

The constructor doesn't really need to be unsafe because it doesn't do anything crazy. This will save us a lot of conversion between Shared and raw pointers and reduce the number of unsafe blocks.

Regarding the Travis failure, let's bump the minimum required Rust version to 1.28. (bump it in README.md and in .travis.yml).

Some bikeshedding:

  • slots vs msgs: I think of msg as a single concrete message and of slot as a place where a message can be stored but isn't necessarily present at all times (i.e. a slot can be empty or filled). So I'd probably go with slots.

  • msg_count reminds me of Vec::len(), while it's actually more like Vec::capacity(). I'd probably name this field cap because it's the capacity of a block, i.e. how many messages the block can hold. Note that we already have a constant named BLOCK_CAP, which would now be good to rename to MAX_BLOCK_CAP. :)

Regarding UnsafeCell, msgs doesn't need to be of type [UnsafeCell<Slot<T>>; 0]. Note that Slot<T> itself consists of ready (of type AtomicBool, which allows shared mutability) and of msg (of type UnsafeCell<ManuallyDrop<T>>, which is already an UnsafeCell). So we're covered.

We could shave off a few bytes from Block<T> by embedding msg_count into a few bits of start_index or into the tag of next, but I wouldn't worry about it now. I believe the overall impact of the additional field is very small anyway.

@kleimkuhler
Copy link
Contributor Author

@stjepang Thank you for the explanations! Good catch on the method signatures; I'll change those.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Just a few nits. Other than that, we can merge once Travis is happy.

let offset = head_index.wrapping_sub(head.start_index);

let slot = head.slots.get_unchecked(offset);
drop((*slot.msg.get()).take());
ManuallyDrop::drop(&mut (*slot).msg.get().read());
Copy link

Choose a reason for hiding this comment

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

We probably don't need to do .read() here. This should work as well:

ManuallyDrop::drop(&mut *(*slot).msg.get());

src/flavors/list.rs Outdated Show resolved Hide resolved
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Perfect, thank you! :)

@ghost ghost merged commit 15da092 into crossbeam-rs:master Oct 4, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants