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

Remove ArrayVec dependency #414

Merged
merged 6 commits into from
Sep 17, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions crossbeam-epoch/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ categories = ["concurrency", "memory-management", "no-std"]

[features]
default = ["std"]
nightly = ["crossbeam-utils/nightly", "arrayvec/use_union"]
nightly = ["crossbeam-utils/nightly"]
std = ["crossbeam-utils/std", "lazy_static"]
alloc = ["crossbeam-utils/alloc"]
sanitize = [] # Makes it more likely to trigger any potential data races.
Expand All @@ -26,10 +26,6 @@ sanitize = [] # Makes it more likely to trigger any potential data races.
cfg-if = "0.1.2"
memoffset = "0.5"

[dependencies.arrayvec]
version = "0.4"
default-features = false

[dependencies.crossbeam-utils]
version = "0.6"
path = "../crossbeam-utils"
Expand Down
51 changes: 44 additions & 7 deletions crossbeam-epoch/src/internal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ use core::ptr;
use core::sync::atomic;
use core::sync::atomic::Ordering;

use arrayvec::ArrayVec;
use crossbeam_utils::CachePadded;

use atomic::{Shared, Owned};
Expand All @@ -60,10 +59,11 @@ const MAX_OBJECTS: usize = 64;
const MAX_OBJECTS: usize = 4;

/// A bag of deferred functions.
#[derive(Default, Debug)]
// can't #[derive(Debug)] because Debug is not implemented for arrays 64 items long
pub struct Bag {
/// Stashed objects.
deferreds: ArrayVec<[Deferred; MAX_OBJECTS]>,
deferreds: [Deferred; MAX_OBJECTS],
len: usize
}

/// `Bag::try_push()` requires that it is safe for another thread to execute the given functions.
Expand All @@ -77,7 +77,7 @@ impl Bag {

/// Returns `true` if the bag is empty.
pub fn is_empty(&self) -> bool {
self.deferreds.is_empty()
self.len == 0
}

/// Attempts to insert a deferred function into the bag.
Expand All @@ -89,7 +89,13 @@ impl Bag {
///
/// It should be safe for another thread to execute the given function.
pub unsafe fn try_push(&mut self, deferred: Deferred) -> Result<(), Deferred> {
self.deferreds.try_push(deferred).map_err(|e| e.element())
if self.len < MAX_OBJECTS {
self.deferreds[self.len] = deferred;
self.len += 1;
Ok(())
} else {
Err(deferred)
}
}

/// Seals the bag with the given epoch.
Expand All @@ -98,17 +104,48 @@ impl Bag {
}
}

impl Default for Bag {
fn default() -> Self {
// [no_op; MAX_OBJECTS] blocked by https://github.com/rust-lang/rust/issues/49147
#[cfg(not(feature = "sanitize"))]
return Bag { len: 0, deferreds:
[Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func),
Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func),
Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func),
Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func),
Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func),
Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func),
Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func),
Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func),
Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func),
Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func),
Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func),
Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func),
Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func),
Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func),
Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func),
Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func)]
Copy link

@jadbox jadbox Sep 12, 2019

Choose a reason for hiding this comment

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

I'm curious on two things over why are this many particular Deferred objects created here and is there a static way to populate the list than to hardcode each new obj construction (to make it DRY'er)?

Copy link
Member

@Vtec234 Vtec234 Sep 12, 2019

Choose a reason for hiding this comment

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

Because Deferred isn't copy, there is only the unsafe option to start with mem::uninitialized and then fill the array with ptr::write, but that would defeat the point of this PR. I don't believe macro_rules macros can pattern match on natural numbers (so that we could insert the same expression n times with a macro), and a compiler plugin would be overkill, so it seems we have to tolerate this until the mentioned rust-lang issue is resolved.
@Shnatsel could you mark this as TODO?

Choose a reason for hiding this comment

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

You can match on a number but you're just matching on the token form of a number, so if you match on 3 you know that you're in the 3 branch but can't automatically use the 3 in any way. You'd need to write a macro that expanded each case by hand anyway.

Copy link

Choose a reason for hiding this comment

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

You could definitely do this with a macro, maybe just not in the most natural way. For example you could hardcode 0-10 fold repetition and write 64-fold repetition as repeating by 6 * 10 + 4 🙂

};
#[cfg(feature = "sanitize")]
return Bag { len: 0, deferreds: [Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func)] };
}
}

impl Drop for Bag {
fn drop(&mut self) {
// Call all deferred functions.
for deferred in self.deferreds.drain(..) {
for i in 0..self.len {
let no_op = Deferred::new(no_op_func);
let deferred = mem::replace(&mut self.deferreds[i], no_op);
deferred.call();
}
Copy link

@bluss bluss Sep 25, 2019

Choose a reason for hiding this comment

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

This can be an iteration over &mut self.deferreds[..self.len]

}
}

fn no_op_func() {}

/// A pair of an epoch and a bag.
#[derive(Default, Debug)]
#[derive(Default)]
struct SealedBag {
epoch: Epoch,
bag: Bag,
Expand Down
1 change: 0 additions & 1 deletion crossbeam-epoch/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ cfg_if! {
)]
cfg_if! {
if #[cfg(any(feature = "alloc", feature = "std"))] {
extern crate arrayvec;
extern crate crossbeam_utils;
#[macro_use]
extern crate memoffset;
Expand Down