~mht/cmr

37c55e552b99a28fca7ab248c827cbb65eced5ee — Martin Hafskjold Thoresen 3 years ago 03d1605
Add some comments and debug stuff

We're still looping somehow. With either insert or delete in the 80/10/10
benchmark we're fine, so there's probably some interleaving of those two
operations that contains the bug. In addition, we stopped looping after the
`defer_unchecked` line was removed from `delete`, which suggests that some
address is reused, and which causes the bug. But this would mean that we are
reading freed memory, which makes it weird that the looping is so consistent.

Maybe try to work backwards: there is a loop. How would a loop be formed in
`delete` or `insert`. Which invariants do we have in the list? Etc.
M crossbeam-skiporder/src/hashmap.rs => crossbeam-skiporder/src/hashmap.rs +2 -0
@@ 170,6 170,8 @@ where
            }

            if entry.insert_between(new_node, g).is_err() {
                // We could restart from the current node if we just lost the cas race, but we start from the
                // sentinel node instead. TODO: change and look at perf impact.
                continue 'restart;
            }


M crossbeam-skiporder/src/lib.rs => crossbeam-skiporder/src/lib.rs +5 -0
@@ 1,5 1,7 @@
#![allow(dead_code)]

use std::alloc::System;

extern crate crossbeam_epoch;
extern crate crossbeam_utils;



@@ 7,3 9,6 @@ pub mod list;
pub mod hashmap;

pub use hashmap::*;

#[global_allocator]
static ALLOC: System = System;

M crossbeam-skiporder/src/list.rs => crossbeam-skiporder/src/list.rs +33 -5
@@ 161,24 161,36 @@ impl<'a, T: Debug> Entry<'a, T> {
        }
    }

    /// If this function returns `Err(IllegalState)` then it will not do anything useful.
    pub fn step(&mut self, p: &'a epoch::Guard) -> Result<T> {
        // If the current pointer is null, we have reached the end, so return `Empty`.
        if self.current.is_null() {
            return Err(Empty);
        }
        let curr = unsafe { self.current.deref() };
        let next = curr.next.load(SeqCst, p);
        if next == self.current {
        let curr: &Node<T> = unsafe { self.current.deref() };
        let next: Shared<Node<T>> = curr.next.load(SeqCst, p);
        if next == self.current { // sanity check which should never fire! (node pointing to itself)
            panic!("Node points to self!");
        }

        // If the pointer has a tag, the `current` node is being deleted, and potentially not in the list anymore.
        if next.tag() == 1 {
            // `current` is also the first node we have seen. This may happen if the `Node` the `Entry` is created
            // from is being deleted before we call `step`. In this case we cannot try to delete it, since we don'
            // have a pointer to the previous node.
            if self.previous == self.current {
                return Err(IllegalState);
            }
            // Help out by swining the `next` pointer on the previous node to the next node.
            match self.delete(p) {
                Ok(_) | Err(NotConsecutive) => {
                // Success. Good for us.
                Ok(_) => {}
                // It turns out that `prev.next != current`. Look closer at `prev.next`:
                Err(NotConsecutive) => {
                    let curr = unsafe { self.previous.deref().next.load(SeqCst, p) };
                    // "Reset" `self.current` to what we now think is the current.
                    self.current = curr;
                    // Oops, `self.previous` is also being deleted! Abort!
                    if curr.tag() == 1 {
                        return Err(IllegalState);
                    }


@@ 272,10 284,17 @@ impl<'a, T: Debug> Entry<'a, T> {
    where
        F: Fn(&T) -> bool,
    {
        use ::std::collections::HashMap;
        // let mut seen = HashMap::new();
        let mut iter = 0;
        loop {
            if self.current.is_null() {
                return Err(Empty);
            }
            // let previous = seen.insert(self.current.as_raw() as usize, iter);
            // if let Some(prev_iter) = previous {
            //     panic!("This value has already been seen! (so we're looping) {}/{}", prev_iter, iter);
            // }
            let curr = unsafe { self.current.deref() };
            if f(curr.data()) {
                return Ok(Done);


@@ 285,6 304,7 @@ impl<'a, T: Debug> Entry<'a, T> {
                e @ Err(IllegalState) => return e,
                _ => {}
            }
            iter += 1;
        }
    }



@@ 295,10 315,17 @@ impl<'a, T: Debug> Entry<'a, T> {
    where
        F: FnMut(&T) -> Option<bool>,
    {
        use ::std::collections::HashMap;
        // let mut seen = HashMap::new();
        let mut iter = 0;
        loop {
            if self.current.is_null() {
                return Err(Empty);
            }
            // let previous = seen.insert(self.current.as_raw() as usize, iter);
            // if let Some(prev_iter) = previous {
            //     panic!("This value has already been seen! (so we're looping) {}/{}", prev_iter, iter);
            // }
            let curr = unsafe { self.current.deref() };
            match f(curr.data()) {
                Some(true) => return Ok(Done),


@@ 307,9 334,10 @@ impl<'a, T: Debug> Entry<'a, T> {
            }

            match self.step(g) {
                e @ Err(IllegalState) => return e,
                e @ Err(_) => return e,
                _ => {}
            }
            iter += 1;
        }
    }