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

AtomicBitSet Debug crashes #62

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Changes from all 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
52 changes: 45 additions & 7 deletions src/atomic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,18 @@ use {BitSetLike, DrainableBitSet};
/// without unique ownership (given that the set is big enough).
/// Removing elements does require unique ownership as an effect
/// of the hierarchy it holds. Worst case multiple writers set the
/// same bit twice (but only is told they set it).
/// same bit twice (but only one is told they set it).
///
/// It is possible to atomically remove from the set, but not at the
/// same time as atomically adding. This is because there is no way
/// to know if layer 1-3 would be left in a consistent state if they are
/// being cleared and set at the same time.
///
/// `AtromicBitSet` resolves this race by disallowing atomic
/// `AtomicBitSet` resolves this race by disallowing atomic
/// clearing of bits.
///
/// [`BitSet`]: ../struct.BitSet.html
#[derive(Debug)]
// #[derive(Debug)]
Copy link
Contributor

Choose a reason for hiding this comment

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

commented debug derive can be removed

pub struct AtomicBitSet {
layer3: AtomicUsize,
layer2: Vec<AtomicUsize>,
Expand Down Expand Up @@ -201,6 +201,13 @@ impl Default for AtomicBitSet {
}
}

impl Debug for AtomicBitSet {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
write!(f, "AtomicBitSet")?;
f.debug_list().entries(self.iter()).finish()
}
}

struct OnceAtom {
inner: AtomicPtr<[AtomicUsize; 1 << BITS]>,
marker: PhantomData<Option<Box<[AtomicUsize; 1 << BITS]>>>,
Expand Down Expand Up @@ -328,10 +335,22 @@ impl AtomicBlock {

impl Debug for AtomicBlock {
fn fmt(&self, f: &mut Formatter) -> Result<(), FormatError> {
f.debug_struct("AtomicBlock")
.field("mask", &self.mask)
.field("atom", &self.atom.get().unwrap().iter())
.finish()
let mut s = f.debug_struct("AtomicBlock");
s.field("mask", &self.mask);
if let Some(atom) = self.atom.get() {
s.field(
"atom",
&atom
Comment on lines +340 to +343
Copy link
Contributor

Choose a reason for hiding this comment

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

if this returns None I think we would still want the formatting to show that an atom field exists

e.g. in the None case perhaps it could be:

s.field("atom", &"None");

or following from my other comment to use a helper type we could do:

s.field("atom", &self.atom.get().map(DebugAtom));

.iter()
.enumerate()
.filter_map(|(idx, v)| match v.fetch_or(0, Ordering::Relaxed) {
0 => None,
x => Some((idx, x)),
})
Comment on lines +346 to +349
Copy link
Contributor

Choose a reason for hiding this comment

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

fetch_or will modify the value, I think we just want load here?

.collect::<Vec<(usize, usize)>>(),
);
}
Comment on lines +338 to +352
Copy link
Contributor

Choose a reason for hiding this comment

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

A helper type could be used here to avoid the allocation when collecting into a vec:

Suggested change
let mut s = f.debug_struct("AtomicBlock");
s.field("mask", &self.mask);
if let Some(atom) = self.atom.get() {
s.field(
"atom",
&atom
.iter()
.enumerate()
.filter_map(|(idx, v)| match v.fetch_or(0, Ordering::Relaxed) {
0 => None,
x => Some((idx, x)),
})
.collect::<Vec<(usize, usize)>>(),
);
}
struct DebugAtom<'a>(&'a [AtomicUsize; 1 << BITS]);
impl Debug for DebugAtom<'_> {
fn fmt(&self, f: &mut Formatter) -> Result<(), FormatError> {
f.debug_list()
.entries(self.0.iter().enumerate().filter_map(|(idx, v)| {
match v.load(Ordering::Relaxed) {
0 => None,
x => Some((idx, x)),
}
}))
.finish()
}
}
let mut s = f.debug_struct("AtomicBlock");
s.field("mask", &self.mask);
if let Some(atom) = self.atom.get() {
s.field("atom", &DebugAtom(atom));
}

s.finish()
}
}

Expand Down Expand Up @@ -483,4 +502,23 @@ mod atomic_set_test {
set.clear();
assert_eq!((&set).iter().count(), 0);
}

#[test]
fn debug() {
let mut bitset = AtomicBitSet::default();
println!("debug = {:?}", bitset);

bitset.add(5);
bitset.add(127194);

println!("debug = {:?}", bitset);

bitset.remove(127194);

println!("debug = {:?}", bitset);

bitset.remove(5);

println!("debug = {:?}", bitset);
}
}