diff --git a/.gitignore b/.gitignore index 69369904..ecb95b05 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,4 @@ /target **/*.rs.bk Cargo.lock +**/.vscode/* diff --git a/src/iter/traverser.rs b/src/iter/traverser.rs index 2d15c297..80a58bb9 100644 --- a/src/iter/traverser.rs +++ b/src/iter/traverser.rs @@ -174,9 +174,16 @@ impl<'g, K, V> Iterator for NodeIter<'g, K, V> { // are also traversed via the `next` pointers of their // contained node e = Some( - &TreeNode::get_tree_node( - tree_bin.first.load(Ordering::SeqCst, self.guard), - ) + // safety: `bin` was read under our guard, at which + // point the tree was valid. Since our guard pins + // the current epoch, the TreeNodes remain valid for + // at least as long as we hold onto the guard. + // Structurally, TreeNodes always point to TreeNodes, so this is sound. + &unsafe { + TreeNode::get_tree_node( + tree_bin.first.load(Ordering::SeqCst, self.guard), + ) + } .node, ); } diff --git a/src/lib.rs b/src/lib.rs index 02b64e58..f5a6b9f7 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -55,13 +55,19 @@ //! operation. When possible, it is a good idea to provide a size estimate by using the //! [`with_capacity`](HashMap::with_capacity) constructor. Note that using many keys with //! exactly the same [`Hash`](std::hash::Hash) value is a sure way to slow down performance of any -//! hash table. +//! hash table. To ameliorate impact, keys are required to be [`Ord`](std::cmp::Ord), which may +//! be used by the map to help break ties. //! /* //! TODO: dynamic load factor +//! */ +//! # Set projections //! -//! TODO: set projection +//! A set projection of a concurrent hash table may be created through [`HashSet`], which provides +//! the same instantiation options as [`HashMap`], such as [`new`](HashSet::new) and [`with_capacity`](HashSet::with_capacity). +//! A hash table may be viewed as a collection of its keys using [`keys`](HashMap::keys). //! +/* //! TODO: frequency map through computeIfAbsent //! //! TODO: bulk operations like forEach, search, and reduce @@ -76,16 +82,18 @@ //! file](http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/main/java/util/concurrent/ConcurrentHashMap.java?view=markup). //! //! The primary design goal of this hash table is to maintain concurrent readability (typically -//! method `get()`, but also iterators and related methods) while minimizing update contention. +//! method [`get`](HashMap::get), but also iterators and related methods) while minimizing update contention. //! Secondary goals are to keep space consumption about the same or better than java.util.HashMap, //! and to support high initial insertion rates on an empty table by many threads. //! //! This map usually acts as a binned (bucketed) hash table. Each key-value mapping is held in a -//! `BinEntry`. Most nodes are of type `BinEntry::Node` with hash, key, value, and a `next` field. -//! However, some nodes are of type `BinEntry::Moved`; these "forwarding nodes" are placed at the -//! heads of bins during resizing. The Java version also has other special node types, but these -//! have not yet been implemented in this port. These special nodes are all either uncommon or -//! transient. +//! `BinEntry`. Most nodes are of type `BinEntry::Node` with hash, key, value, and a `next` field. +//! However, some other types of nodes exist: `BinEntry::TreeNode`s are arranged in balanced trees +//! instead of linear lists. Bins of type `BinEntry::Tree` hold the roots of sets of `BinEntry::TreeNode`s. +//! Some nodes are of type `BinEntry::Moved`; these "forwarding nodes" are placed at the +//! heads of bins during resizing. The Java version also has other special node types, but these +//! have not yet been implemented in this port. These special nodes are all either uncommon or +//! transient. //! /* //! TODO: TreeNodes, ReservationNodes @@ -95,8 +103,8 @@ //! `BinEntry`). Table accesses require atomic reads, writes, and CASes. //! //! Insertion (via `put`) of the first node in an empty bin is performed by just CASing it to the -//! bin. This is by far the most common case for put operations under most key/hash distributions. -//! Other update operations (insert, delete, and replace) require locks. We do not want to waste +//! bin. This is by far the most common case for put operations under most key/hash distributions. +//! Other update operations (insert, delete, and replace) require locks. We do not want to waste //! the space required to associate a distinct lock object with each bin, so we instead embed a //! lock inside each node, and use the lock in the the first node of a bin list as the lock for the //! bin. @@ -109,7 +117,7 @@ //! The main disadvantage of per-bin locks is that other update operations on other nodes in a bin //! list protected by the same lock can stall, for example when user `Eq` implementations or //! mapping functions take a long time. However, statistically, under random hash codes, this is -//! not a common problem. Ideally, the frequency of nodes in bins follows a Poisson distribution +//! not a common problem. Ideally, the frequency of nodes in bins follows a Poisson distribution //! (http://en.wikipedia.org/wiki/Poisson_distribution) with a parameter of about 0.5 on average, //! given the resizing threshold of 0.75, although with a large variance because of resizing //! granularity. Ignoring variance, the expected occurrences of list size `k` are `exp(-0.5) * @@ -132,30 +140,34 @@ //! #elements)` under random hashes. //! //! Actual hash code distributions encountered in practice sometimes deviate significantly from -//! uniform randomness. This includes the case when `N > (1<<30)`, so some keys MUST collide. +//! uniform randomness. This includes the case when `N > (1<<30)`, so some keys MUST collide. //! Similarly for dumb or hostile usages in which multiple keys are designed to have identical hash -//! codes or ones that differs only in masked-out high bits. Here, the Java implementation uses an -//! optimization where a bin is turned into a binary tree, but this has not yet been ported over to -//! the Rust version. -/* TODO */ +//! codes or ones that differs only in masked-out high bits. So a secondary strategy is used that +//! applies when the number of nodes in a bin exceeds a threshold. These `BinEntry::Tree` bins use +//! a balanced tree to hold nodes (a specialized form of red-black trees), bounding search time to +//! `O(log N)`. Each search step in such a bin is at least twice as slow as in a regular list, but +//! given that N cannot exceed `(1<<64)` (before running out of adresses) this bounds search steps, +//! lock hold times, etc, to reasonable constants (roughly 100 nodes inspected per operation worst +//! case) as keys are required to be comparable ([`Ord`](std::cmp::Ord)). `BinEntry::Tree` nodes +//! (`BinEntry::TreeNode`s) also maintain the same `next` traversal pointers as regular nodes, so +//! can be traversed in iterators in a similar way. //! //! The table is resized when occupancy exceeds a percentage threshold (nominally, 0.75, but see -//! below). Any thread noticing an overfull bin may assist in resizing after the initiating thread +//! below). Any thread noticing an overfull bin may assist in resizing after the initiating thread //! allocates and sets up the replacement array. However, rather than stalling, these other threads -//! may proceed with insertions etc. Resizing proceeds by transferring bins, one by one, from the -//! table to the next table. However, threads claim small blocks of indices to transfer (via the -//! field `transfer_index`) before doing so, reducing contention. A generation stamp in the field -//! `size_ctl` ensures that resizings do not overlap. Because we are using power-of-two expansion, -//! the elements from each bin must either stay at same index, or move with a power of two offset. -//! We eliminate unnecessary node creation by catching cases where old nodes can be reused because -//! their next fields won't change. On average, only about one-sixth of them need cloning when a -//! table doubles. The nodes they replace will be garbage collectible as soon as they are no longer -//! referenced by any reader thread that may be in the midst of concurrently traversing table. -//! Upon transfer, the old table bin contains only a special forwarding node (`BinEntry::Moved`) -//! that contains the next table as its key. On encountering a forwarding node, access and update -//! operations restart, using the new table. -//! -/* TODO: note on TreeBins */ +//! may proceed with insertions etc. The use of `BinEntry::Tree` bins shields us from the worst case +//! effects of overfilling while resizes are in progress. Resizing proceeds by transferring bins, +//! one by one, from the table to the next table. However, threads claim small blocks of indices to +//! transfer (via the field `transfer_index`) before doing so, reducing contention. A generation +//! stamp in the field `size_ctl` ensures that resizings do not overlap. Because we are using +//! power-of-two expansion, the elements from each bin must either stay at same index, or move with +//! a power of two offset. We eliminate unnecessary node creation by catching cases where old nodes +//! can be reused because their next fields won't change. On average, only about one-sixth of them +//! need cloning when a table doubles. The nodes they replace will be garbage collectible as soon +//! as they are no longer referenced by any reader thread that may be in the midst of concurrently +//! traversing table. Upon transfer, the old table bin contains only a special forwarding node +//! (`BinEntry::Moved`) that contains the next table as its key. On encountering a forwarding node, +//! access and update operations restart, using the new table. //! //! Each bin transfer requires its bin lock, which can stall waiting for locks while resizing. //! However, because other threads can join in and help resize rather than contend for locks, @@ -190,10 +202,26 @@ //! occurring at threshold is around 13%, meaning that only about 1 in 8 puts check threshold (and //! after resizing, many fewer do so). //! */ -/* TODO: //! -//! TreeBins comparisons and locking -*/ +/* NOTE that we don't actually use most of the Java Code's complicated comparisons and tiebreakers + * since we require total ordering among the keys via `Ord` as opposed to a runtime check against + * Java's `Comparable` interface. */ +//! `BinEntry::Tree` bins use a special form of comparison for search and related operations (which +//! is the main reason they do not use existing collections such as tree maps). The contained tree +//! is primarily ordered by hash value, then by [`cmp`](std::cmp::Ord::cmp) order on keys. The +//! red-black balancing code is updated from pre-jdk colelctions (http://gee.cs.oswego.edu/dl/classes/collections/RBCell.java) +//! based in turn on Cormen, Leiserson, and Rivest "Introduction to Algorithms" (CLR). +//! +//! `BinEntry::Tree` bins also require an additional locking mechanism. While list traversal is +//! always possible by readers even during updates, tree traversal is not, mainly because of +//! tree-rotations that may change the root node and/or its linkages. Tree bins include a simple +//! reaad-write lock mechanism parasitic on the main bin-synchronization strategy: Structural +//! adjustments associated with an insertion or removal are already bin-locked (and so cannot +//! conflict with other writers) but must wait for ongoing readers to finish. Since there can be +//! only one such waiter, we use a simple scheme using a single `waiter` field to block writers. +//! However, readers need never block. If the root lock is held, they proceed along the slow +//! traversal path (via next-pointers) until the lock becomes available or the list is exhausted, +//! whichever comes first. These cases are not fast, but maximize aggregate expected throughput. //! //! ## Garbage collection //! @@ -213,6 +241,7 @@ intra_doc_link_resolution_failure )] #![warn(rust_2018_idioms)] +#![allow(clippy::cognitive_complexity)] mod map; mod map_ref; diff --git a/src/map.rs b/src/map.rs index add3d7c5..3ea57bdc 100644 --- a/src/map.rs +++ b/src/map.rs @@ -73,7 +73,7 @@ macro_rules! load_factor { /// A concurrent hash table. /// -/// Flurry uses an [`Guards`] to control the lifetime of the resources that get stored and +/// Flurry uses [`Guards`] to control the lifetime of the resources that get stored and /// extracted from the map. [`Guards`] are acquired through the [`epoch::pin`], [`HashMap::pin`] /// and [`HashMap::guard`] functions. For more information, see the [`notes in the crate-level /// documentation`]. @@ -764,15 +764,18 @@ where // safety: bin is a valid pointer. // - // there are two cases when a bin pointer is invalidated: + // there are three cases when a bin pointer is invalidated: // // 1. if the table was resized, bin is a move entry, and the resize has completed. in // that case, the table (and all its heads) will be dropped in the next epoch // following that. // 2. if the table is being resized, bin may be swapped with a move entry. the old bin // will then be dropped in the following epoch after that happens. + // 3. when elements are inserted into or removed from the map, bin may be changed into + // or from a TreeBin from or into a regular, linear bin. the old bin will also be + // dropped in the following epoch if that happens. // - // in both cases, we held the guard when we got the reference to the bin. if any such + // in all cases, we held the guard when we got the reference to the bin. if any such // swap happened, it must have happened _after_ we read. since we did the read while // pinning the epoch, the drop must happen in the _next_ epoch (i.e., the one that we // are holding up by holding on to our guard). @@ -919,7 +922,12 @@ where let mut high_count = 0; let mut e = tree_bin.first.load(Ordering::Relaxed, guard); while !e.is_null() { - let tree_node = TreeNode::get_tree_node(e); + // safety: the TreeBin was read under our guard, at + // which point the tree structure was valid. Since our + // guard pins the current epoch, the TreeNodes remain + // valid for at least as long as we hold onto the guard. + // Structurally, TreeNodes always point to TreeNodes, so this is sound. + let tree_node = unsafe { TreeNode::get_tree_node(e) }; let hash = tree_node.node.hash; let new_node = TreeNode::new( tree_node.node.key.clone(), @@ -937,7 +945,9 @@ where // this is the first element inserted into the low bin low = new_node; } else { - TreeNode::get_tree_node(low_tail) + // safety: `low_tail` was just created by us and not shared. + // Structurally, TreeNodes always point to TreeNodes, so this is sound. + unsafe { TreeNode::get_tree_node(low_tail) } .node .next .store(new_node, Ordering::Relaxed); @@ -952,7 +962,9 @@ where // this is the first element inserted into the high bin high = new_node; } else { - TreeNode::get_tree_node(high_tail) + // safety: `high_tail` was just created by us and not shared. + // Structurally, TreeNodes always point to TreeNodes, so this is sound. + unsafe { TreeNode::get_tree_node(high_tail) } .node .next .store(new_node, Ordering::Relaxed); @@ -969,22 +981,11 @@ where // bin is too small. since the tree nodes are // already behind shared references, we have to // clean them up manually. - let l = Self::untreeify(low, guard); + let low_linear = Self::untreeify(low, guard); // safety: we have just created `low` and its `next` // nodes and have never shared them - let mut p = unsafe { low.into_owned() }; - loop { - let tree_node = if let BinEntry::TreeNode(tree_node) = *p.into_box() { - tree_node - } else { - unreachable!("Trees can only ever contain TreeNodes"); - }; - if tree_node.node.next.load(Ordering::SeqCst, guard).is_null() { - break; - } - p = unsafe { tree_node.node.next.into_owned() }; - } - l + unsafe { Self::drop_tree_nodes(low, guard) }; + low_linear } else { // the new bin will also be a tree bin. if both the high // bin and the low bin are non-empty, we have to @@ -995,33 +996,30 @@ where Owned::new(BinEntry::Tree(TreeBin::new(low, guard))).into_shared(guard) } else { reused_bin = true; + // since we also don't use the created low nodes here, + // we need to clean them up. + // safety: we have just created `low` and its `next` + // nodes and have never shared them + unsafe { Self::drop_tree_nodes(low, guard) }; bin } }; let high_bin = if high_count <= UNTREEIFY_THRESHOLD { - let h = Self::untreeify(high, guard); + let high_linear = Self::untreeify(high, guard); // safety: we have just created `high` and its `next` // nodes and have never shared them - let mut p = unsafe { high.into_owned() }; - loop { - let tree_node = if let BinEntry::TreeNode(tree_node) = *p.into_box() { - tree_node - } else { - unreachable!("Trees can only ever contain TreeNodes"); - }; - if tree_node.node.next.load(Ordering::SeqCst, guard).is_null() { - break; - } - p = unsafe { tree_node.node.next.into_owned() }; - } - h + unsafe { Self::drop_tree_nodes(high, guard) }; + high_linear + } else if low_count != 0 { + Owned::new(BinEntry::Tree(TreeBin::new(high, guard))).into_shared(guard) } else { - if low_count != 0 { - Owned::new(BinEntry::Tree(TreeBin::new(high, guard))).into_shared(guard) - } else { - reused_bin = true; - bin - } + reused_bin = true; + // since we also don't use the created low nodes here, + // we need to clean them up. + // safety: we have just created `high` and its `next` + // nodes and have never shared them + unsafe { Self::drop_tree_nodes(high, guard) }; + bin }; next_table.store_bin(i, low_bin); @@ -1055,6 +1053,22 @@ where } } + /// Drops the given list of tree nodes, but not their values. + /// + /// # Safety + /// The pointers to the tree nodes must be valid and the caller must be the single owner + /// of the tree nodes. + unsafe fn drop_tree_nodes<'g>(from: Shared<'g, BinEntry>, guard: &'g Guard) { + let mut p = from; + while !p.is_null() { + if let BinEntry::TreeNode(ref tree_node) = *p.into_owned().into_box() { + p = tree_node.node.next.load(Ordering::SeqCst, guard); + } else { + unreachable!("Trees can only ever contain TreeNodes"); + }; + } + } + fn help_transfer<'g>( &'g self, table: Shared<'g, Table>, @@ -1242,15 +1256,18 @@ where // safety: bin is a valid pointer. // - // there are two cases when a bin pointer is invalidated: + // there are three cases when a bin pointer is invalidated: // // 1. if the table was resized, bin is a move entry, and the resize has completed. in // that case, the table (and all its heads) will be dropped in the next epoch // following that. // 2. if the table is being resized, bin may be swapped with a move entry. the old bin // will then be dropped in the following epoch after that happens. + // 3. when elements are inserted into or removed from the map, bin may be changed into + // or from a TreeBin from or into a regular, linear bin. the old bin will also be + // dropped in the following epoch if that happens. // - // in both cases, we held the guard when we got the reference to the bin. if any such + // in all cases, we held the guard when we got the reference to the bin. if any such // swap happened, it must have happened _after_ we read. since we did the read while // pinning the epoch, the drop must happen in the _next_ epoch (i.e., the one that we // are holding up by holding on to our guard). @@ -1262,10 +1279,11 @@ where // next epoch after it is removed. since it wasn't removed, and the epoch was pinned, that // cannot be until after we drop our guard. let node = unsafe { node.deref() }; - Some( - node.as_node() - .expect("`BinEntry::find` should always return a Node"), - ) + Some(match node { + BinEntry::Node(ref n) => n, + BinEntry::TreeNode(ref tn) => &tn.node, + _ => panic!("`Table::find` should always return a Node"), + }) } /// Returns `true` if the map contains a value for the specified key. @@ -1487,14 +1505,18 @@ where while !p.is_null() { delta -= 1; p = { - // safety: we loaded p under guard, and guard is still pinned, so p has not been dropped. - let tree_node = TreeNode::get_tree_node(p); - let next = tree_node.node.next.load(Ordering::SeqCst, guard); + // safety: the TreeBin was read under our guard, at + // which point the tree was valid. Since our guard + // pins the current epoch, the TreeNodes remain + // valid for at least as long as we hold onto the + // guard. + // Structurally, TreeNodes always point to TreeNodes, so this is sound. + let tree_node = unsafe { TreeNode::get_tree_node(p) }; // NOTE: we do not drop the TreeNodes or their // values here, since they will be dropped together // with the containing TreeBin (`tree_bin`) in its // `drop` - next + tree_node.node.next.load(Ordering::SeqCst, guard) }; } drop(bin_lock); @@ -1602,7 +1624,7 @@ where let node = Owned::new(BinEntry::Node(Node { key: key.clone(), value: Atomic::from(value), - hash: hash, + hash, next: Atomic::null(), lock: parking_lot::Mutex::new(()), })); @@ -1619,22 +1641,25 @@ where } } - // slow path -- bin is non-empty + let mut old_val = None; + // safety: bin is a valid pointer. // - // there are two cases when a bin pointer is invalidated: + // there are three cases when a bin pointer is invalidated: // // 1. if the table was resized, bin is a move entry, and the resize has completed. in // that case, the table (and all its heads) will be dropped in the next epoch // following that. // 2. if the table is being resized, bin may be swapped with a move entry. the old bin // will then be dropped in the following epoch after that happens. + // 3. when elements are inserted into or removed from the map, bin may be changed into + // or from a TreeBin from or into a regular, linear bin. the old bin will also be + // dropped in the following epoch if that happens. // - // in both cases, we held the guard when we got the reference to the bin. if any such + // in all cases, we held the guard when we got the reference to the bin. if any such // swap happened, it must have happened _after_ we read. since we did the read while // pinning the epoch, the drop must happen in the _next_ epoch (i.e., the one that we // are holding up by holding on to our guard). - let mut old_val = None; match *unsafe { bin.deref() } { BinEntry::Moved => { table = self.help_transfer(table, guard); @@ -1728,7 +1753,7 @@ where let node = Owned::new(BinEntry::Node(Node { key, value: Atomic::from(value), - hash: hash, + hash, next: Atomic::null(), lock: parking_lot::Mutex::new(()), })); @@ -1762,7 +1787,12 @@ where bin_count = 2; let p = tree_bin.find_or_put_tree_val(hash, key, value, guard); if !p.is_null() { - let tree_node = TreeNode::get_tree_node(p); + // safety: the TreeBin was read under our guard, at + // which point the tree structure was valid. Since our + // guard pins the current epoch, the TreeNodes remain + // valid for at least as long as we hold onto the guard. + // Structurally, TreeNodes always point to TreeNodes, so this is sound. + let tree_node = unsafe { TreeNode::get_tree_node(p) }; old_val = { let current_value = tree_node.node.value.load(Ordering::SeqCst, guard); // safety: since the value is present now, and we've held a guard from @@ -1815,7 +1845,7 @@ where // However, our code doesn't (it uses continue) and `bin_count` // _cannot_ be 0 at this point. if bin_count >= TREEIFY_THRESHOLD { - self.treeify_bin(table, bini, guard); + self.treeify_bin(t, bini, guard); } guard.flush(); if old_val.is_some() { @@ -1825,7 +1855,7 @@ where } // increment count, since we only get here if we did not return an old (updated) value self.add_count(1, Some(bin_count), guard); - return None; + None } fn put_all>(&self, iter: I, guard: &Guard) { @@ -1902,15 +1932,18 @@ where // slow path -- bin is non-empty // safety: bin is a valid pointer. // - // there are two cases when a bin pointer is invalidated: + // there are three cases when a bin pointer is invalidated: // // 1. if the table was resized, bin is a move entry, and the resize has completed. in // that case, the table (and all its heads) will be dropped in the next epoch // following that. // 2. if the table is being resized, bin may be swapped with a move entry. the old bin // will then be dropped in the following epoch after that happens. + // 3. when elements are inserted into or removed from the map, bin may be changed into + // or from a TreeBin from or into a regular, linear bin. the old bin will also be + // dropped in the following epoch if that happens. // - // in both cases, we held the guard when we got the reference to the bin. if any such + // in all cases, we held the guard when we got the reference to the bin. if any such // swap happened, it must have happened _after_ we read. since we did the read while // pinning the epoch, the drop must happen in the _next_ epoch (i.e., the one that we // are holding up by holding on to our guard). @@ -2063,7 +2096,14 @@ where None } else { // a node for the given key exists, so we try to update it - let n = &TreeNode::get_tree_node(p).node; + // safety: the TreeBin was read under our guard, + // at which point the tree structure was valid. + // Since our guard pins the current epoch, the + // TreeNodes and `p` in particular remain valid + // for at least as long as we hold onto the + // guard. + // Structurally, TreeNodes always point to TreeNodes, so this is sound. + let n = &unsafe { TreeNode::get_tree_node(p) }.node; let current_value = n.value.load(Ordering::SeqCst, guard); // safety: since the value is present now, and we've held a guard from @@ -2105,7 +2145,9 @@ where } else { removed_node = true; // remove the BinEntry::TreeNode containing the removed key value pair from the bucket - let need_to_untreeify = tree_bin.remove_tree_node(p, guard); + // safety: `p` is marked for garbage collection below if the bin gets untreeified + let need_to_untreeify = + unsafe { tree_bin.remove_tree_node(p, guard) }; if need_to_untreeify { let linear_bin = Self::untreeify( tree_bin.first.load(Ordering::SeqCst, guard), @@ -2119,7 +2161,13 @@ where // <= our epoch and have to be dropped before the epoch can advance // past the destruction of the old bin. After the store, threads will // always see the linear bin, so the cannot obtain new references either. - unsafe { guard.defer_destroy(bin) }; + // + // The same holds for `p`, which does not get dropped together with `bin` + // here since `remove_tree_node` indicated that the bin needs to be untreeified. + unsafe { + guard.defer_destroy(bin); + guard.defer_destroy(p); + } } None } @@ -2144,7 +2192,7 @@ where self.add_count(-1, Some(bin_count), guard); } guard.flush(); - return new_val; + new_val } /// Removes a key from the map, returning a reference to the value at the @@ -2265,15 +2313,18 @@ where let mut old_val: Option<(&'g K, Shared<'g, V>)> = None; // safety: bin is a valid pointer. // - // there are two cases when a bin pointer is invalidated: + // there are three cases when a bin pointer is invalidated: // // 1. if the table was resized, bin is a move entry, and the resize has completed. in // that case, the table (and all its heads) will be dropped in the next epoch // following that. // 2. if the table is being resized, bin may be swapped with a move entry. the old bin // will then be dropped in the following epoch after that happens. + // 3. when elements are inserted into or removed from the map, bin may be changed into + // or from a TreeBin from or into a regular, linear bin. the old bin will also be + // dropped in the following epoch if that happens. // - // in both cases, we held the guard when we got the reference to the bin. if any such + // in all cases, we held the guard when we got the reference to the bin. if any such // swap happened, it must have happened _after_ we read. since we did the read while // pinning the epoch, the drop must happen in the _next_ epoch (i.e., the one that we // are holding up by holding on to our guard). @@ -2360,7 +2411,14 @@ where if !root.is_null() { let p = TreeNode::find_tree_node(root, hash, key, guard); if !p.is_null() { - let n = &TreeNode::get_tree_node(p).node; + // safety: the TreeBin was read under our guard, + // at which point the tree structure was valid. + // Since our guard pins the current epoch, the + // TreeNodes and `p` in particular remain valid + // for at least as long as we hold onto the + // guard. + // Structurally, TreeNodes always point to TreeNodes, so this is sound. + let n = &unsafe { TreeNode::get_tree_node(p) }.node; let pv = n.value.load(Ordering::SeqCst, guard); // just remove the node if the value is the one we expected at method call @@ -2372,7 +2430,9 @@ where // found the node but we have a new value to replace the old one n.value.store(Owned::new(nv), Ordering::SeqCst); } else { - let need_to_untreeify = tree_bin.remove_tree_node(p, guard); + // safety: `p` is marked for garbage collection below if the bin gets untreeified + let need_to_untreeify = + unsafe { tree_bin.remove_tree_node(p, guard) }; if need_to_untreeify { let linear_bin = Self::untreeify( tree_bin.first.load(Ordering::SeqCst, guard), @@ -2381,7 +2441,10 @@ where t.store_bin(bini, linear_bin); // the old bin is now garbage // safety: same as in put - unsafe { guard.defer_destroy(bin) }; + unsafe { + guard.defer_destroy(bin); + guard.defer_destroy(p); + } } } } @@ -2508,27 +2571,37 @@ where { /// Replaces all linked nodes in the bin at the given index unless the table /// is too small, in which case a resize is initiated instead. - fn treeify_bin<'g>(&'g self, tab: Shared<'g, Table>, index: usize, guard: &'g Guard) { - if tab.is_null() { - return; - } - - let tab_deref = tab.deref(); - let n = tab_deref.len(); + fn treeify_bin<'g>(&'g self, tab: &Table, index: usize, guard: &'g Guard) { + let n = tab.len(); if n < MIN_TREEIFY_CAPACITY { self.try_presize(n << 1, guard); } else { - let b = tab_deref.bin(index, guard); - if !b.is_null() { - if let BinEntry::Node(ref node) = b.deref() { + let bin = tab.bin(index, guard); + if !bin.is_null() { + // safety: we loaded `bin` while the epoch was pinned by our + // guard. if the bin was replaced since then, the old bin still + // won't be dropped until after we release our guard. + if let BinEntry::Node(ref node) = unsafe { bin.deref() } { let lock = node.lock.lock(); - // check if `b` is still the head - if tab_deref.bin(index, guard) == b { - let mut e = b; + // check if `bin` is still the head + if tab.bin(index, guard) == bin { + let mut e = bin; let mut head = Shared::null(); let mut last_tree_node = Shared::null(); while !e.is_null() { - let e_deref = e.deref().as_node().unwrap(); + // safety: either `e` is `bin`, in which case it is + // valid due to the above, or `e` was obtained from + // a next pointer. Any next pointer obtained from + // bin is valid at the time we look up bin in the + // table, at which point the epoch is pinned by our + // guard. Since we found the next pointer in a valid + // map and it is not null (as checked above), the + // node it points to was present (i.e. not removed) + // from the map in the current epoch. Thus, because + // the epoch cannot advance until we release our + // guard, `e` is also valid if it was obtained from + // a next pointer. + let e_deref = unsafe { e.deref() }.as_node().unwrap(); let new_tree_node = TreeNode::new( e_deref.key.clone(), e_deref.value.clone(), // this uses a load with Ordering::Relaxed, but write access is synchronized through the bin lock @@ -2556,12 +2629,9 @@ where last_tree_node = new_tree_node; e = e_deref.next.load(Ordering::SeqCst, guard); } - tab_deref.store_bin( - index, - Owned::new(BinEntry::Tree(TreeBin::new(head, guard))), - ); + tab.store_bin(index, Owned::new(BinEntry::Tree(TreeBin::new(head, guard)))); // make sure the old bin entries get dropped - e = b; + e = bin; while !e.is_null() { // safety: we just replaced the bin containing this // BinEntry, making it unreachable for other threads @@ -2604,7 +2674,15 @@ where let mut last_node: Shared<'_, BinEntry> = Shared::null(); let mut q = node; while !q.is_null() { - let q_deref = q.deref().as_tree_node().unwrap(); + // safety: we only untreeify sequences of TreeNodes which either + // - were just created (e.g. in transfer) and are thus valid, or + // + // - are read from a TreeBin loaded from the map. In this case, + // the bin gets loaded under our guard and at that point all + // of its nodes (its `first` and all `next` nodes) are valid. + // As `q` is not `null` (checked above), this means that `q` + // remains a valid pointer at least until our guard is dropped. + let q_deref = unsafe { q.deref() }.as_tree_node().unwrap(); let new_node = Owned::new(BinEntry::Node(Node { hash: q_deref.node.hash, key: q_deref.node.key.clone(), diff --git a/src/node.rs b/src/node.rs index 314f5b53..3889fd89 100644 --- a/src/node.rs +++ b/src/node.rs @@ -86,7 +86,7 @@ pub(crate) struct TreeNode { } impl TreeNode { - pub fn new( + pub(crate) fn new( key: K, value: Atomic, hash: u64, @@ -95,13 +95,13 @@ impl TreeNode { ) -> Self { TreeNode { node: Node { - key: key, - value: value, - hash: hash, - next: next, + key, + value, + hash, + next, lock: parking_lot::Mutex::new(()), }, - parent: parent, + parent, left: Atomic::null(), right: Atomic::null(), prev: Atomic::null(), @@ -131,7 +131,12 @@ impl TreeNode { // assignments from checks. let mut p = from; while !p.is_null() { - let p_deref = Self::get_tree_node(p); + // safety: the containing TreeBin of all TreeNodes was read under our + // guard, at which point the tree structure was valid. Since our guard + // pins the current epoch, the TreeNodes remain valid for at least as + // long as we hold onto the guard. + // Structurally, TreeNodes always point to TreeNodes, so this is sound. + let p_deref = unsafe { Self::get_tree_node(p) }; let p_left = p_deref.left.load(Ordering::SeqCst, guard); let p_right = p_deref.right.load(Ordering::SeqCst, guard); let p_hash = p_deref.node.hash; @@ -181,7 +186,7 @@ impl TreeNode { // cases cannot occur here. } - return Shared::null(); + Shared::null() } } @@ -207,13 +212,17 @@ impl TreeBin where K: Ord, { - pub fn new<'g>(bin: Shared<'g, BinEntry>, guard: &'g Guard) -> Self { + pub(crate) fn new<'g>(bin: Shared<'g, BinEntry>, guard: &'g Guard) -> Self { let mut root: Shared<'_, BinEntry> = Shared::null(); let mut x = bin; let mut next: Shared<'_, BinEntry>; + // safety: when creating a new TreeBin, the nodes used are created just + // for this bin and not shared with another thread, so they cannot get + // invalidated. + // Structurally, TreeNodes always point to TreeNodes, so this is sound. while !x.is_null() { - let x_deref = TreeNode::get_tree_node(x); + let x_deref = unsafe { TreeNode::get_tree_node(x) }; next = x_deref.node.next.load(Ordering::SeqCst, guard); x_deref.left.store(Shared::null(), Ordering::Relaxed); x_deref.right.store(Shared::null(), Ordering::Relaxed); @@ -234,37 +243,38 @@ where // find out where to insert x let mut p = root; loop { - let dir: i8; - let pd = TreeNode::get_tree_node(p); - let p_key = &pd.node.key; - let p_hash = pd.node.hash; - if p_hash > hash { - dir = -1; - } else if p_hash < hash { - dir = 1; - } else { - dir = match p_key.cmp(&key) { + let p_deref = unsafe { TreeNode::get_tree_node(p) }; + let p_key = &p_deref.node.key; + let p_hash = p_deref.node.hash; + let dir = match p_hash.cmp(&hash) { + std::cmp::Ordering::Greater => -1, + std::cmp::Ordering::Less => 1, + std::cmp::Ordering::Equal => match p_key.cmp(&key) { std::cmp::Ordering::Greater => -1, std::cmp::Ordering::Less => 1, std::cmp::Ordering::Equal => unreachable!("one key references two nodes"), - } - } + }, + }; // Select successor of p in the direction dir. We will continue // to descend the tree through this successor. let xp = p; p = if dir <= 0 { - pd.left.load(Ordering::SeqCst, guard) + p_deref.left.load(Ordering::SeqCst, guard) } else { - pd.right.load(Ordering::SeqCst, guard) + p_deref.right.load(Ordering::SeqCst, guard) }; if p.is_null() { x_deref.parent.store(xp, Ordering::SeqCst); if dir <= 0 { - TreeNode::get_tree_node(xp).left.store(x, Ordering::SeqCst); + unsafe { TreeNode::get_tree_node(xp) } + .left + .store(x, Ordering::SeqCst); } else { - TreeNode::get_tree_node(xp).right.store(x, Ordering::SeqCst); + unsafe { TreeNode::get_tree_node(xp) } + .right + .store(x, Ordering::SeqCst); } root = TreeNode::balance_insertion(root, x, guard); break; @@ -274,7 +284,9 @@ where x = next; } - assert!(TreeNode::check_invariants(root, guard)); + if cfg!(debug) { + assert!(TreeNode::check_invariants(root, guard)); + } TreeBin { root: Atomic::from(root), first: Atomic::from(bin), @@ -352,7 +364,24 @@ impl TreeBin { K: Borrow, Q: ?Sized + Ord, { - let bin_deref = bin.deref().as_tree_bin().unwrap(); + // safety: bin is a valid pointer. + // + // there are three cases when a bin pointer is invalidated: + // + // 1. if the table was resized, bin is a move entry, and the resize has completed. in + // that case, the table (and all its heads) will be dropped in the next epoch + // following that. + // 2. if the table is being resized, bin may be swapped with a move entry. the old bin + // will then be dropped in the following epoch after that happens. + // 3. when elements are inserted into or removed from the map, bin may be changed into + // or from a TreeBin from or into a regular, linear bin. the old bin will also be + // dropped in the following epoch if that happens. + // + // in all cases, we held the guard when we got the reference to the bin. if any such + // swap happened, it must have happened _after_ we read. since we did the read while + // pinning the epoch, the drop must happen in the _next_ epoch (i.e., the one that we + // are holding up by holding on to our guard). + let bin_deref = unsafe { bin.deref() }.as_tree_bin().unwrap(); let mut element = bin_deref.first.load(Ordering::SeqCst, guard); while !element.is_null() { let s = bin_deref.lock_state.load(Ordering::SeqCst); @@ -361,7 +390,13 @@ impl TreeBin { // (write). As long as that's the case, we follow the `next` // pointers of the `TreeNode` linearly, as we cannot trust the // tree's structure. - let element_deref = TreeNode::get_tree_node(element); + // + // safety: we were read under our guard, at which point the tree + // structure was valid. Since our guard pins the current epoch, + // the TreeNodes remain valid for at least as long as we hold + // onto the guard. + // Structurally, TreeNodes always point to TreeNodes, so this is sound. + let element_deref = unsafe { TreeNode::get_tree_node(element) }; let element_key = &element_deref.node.key; if element_deref.node.hash == hash && element_key.borrow() == key { return element; @@ -384,28 +419,45 @@ impl TreeBin { // check if another thread is waiting and, if so, unpark it let waiter = &bin_deref.waiter.load(Ordering::SeqCst, guard); if !waiter.is_null() { - waiter.deref().unpark(); + // safety: we do not destroy waiting threads + unsafe { waiter.deref() }.unpark(); } } return p; } } - return Shared::null(); + Shared::null() } - /// Removes the given node, which must be present before this call. This is + /// Unlinks the given node, which must be present before this call. This is /// messier than typical red-black deletion code because we cannot swap the /// contents of an interior node with a leaf successor that is pinned by /// `next` pointers that are accessible independently of the bin lock. So /// instead we swap the tree links. /// /// Returns `true` if the bin is now too small and should be untreeified. - pub(crate) fn remove_tree_node<'g>( + /// + /// # Safety + /// The given node is only marked for garbage collection if the bin remains + /// large enough to be a `TreeBin`. If this method returns `true`, indicating + /// that the bin should be untreeified, the given node is only unlinked from + /// linear traversal, but not from the tree. This makes the node unreachable + /// through linear reads and excludes it from being dropped when the bin is + /// dropped. However, reading threads may still obtain a reference to until + /// the bin is swapped out for a linear bin. The caller of this method _must_ + /// ensure that the given node is properly marked for garbage collection + /// _after_ this bin has bin swapped out. + pub(crate) unsafe fn remove_tree_node<'g>( &'g self, p: Shared<'g, BinEntry>, guard: &'g Guard, ) -> bool { + // safety: we were read under our guard, at which point the tree + // structure was valid. Since our guard pins the current epoch, the + // TreeNodes remain valid for at least as long as we hold onto the + // guard. Additionally, this method assumes `p` to be non-null. + // Structurally, TreeNodes always point to TreeNodes, so this is sound. let p_deref = TreeNode::get_tree_node(p); let next = p_deref.node.next.load(Ordering::SeqCst, guard); let prev = p_deref.prev.load(Ordering::SeqCst, guard); @@ -424,6 +476,7 @@ impl TreeBin { .prev .store(prev, Ordering::SeqCst); } + if self.first.load(Ordering::SeqCst, guard).is_null() { // since the bin was not empty previously (it contained p), // `self.first` is `null` only if we just stored `null` via `next`. @@ -617,15 +670,25 @@ impl TreeBin { } // mark the old node and its value for garbage collection - guard.defer_destroy(p_deref.node.value.load(Ordering::Relaxed, guard)); - guard.defer_destroy(p); + // safety: we just completely unlinked `p` from both linear and tree + // traversal, making it and its value unreachable for any future thread. + // Any existing references to one of them were obtained under a guard + // that pins an epoch <= our epoch, and thus have to be released before + // `p` is actually dropped. + #[allow(unused_unsafe)] + unsafe { + guard.defer_destroy(p_deref.node.value.load(Ordering::Relaxed, guard)); + guard.defer_destroy(p); + } self.unlock_root(); - assert!(TreeNode::check_invariants( - self.root.load(Ordering::SeqCst, guard), - guard - )); - return false; + if cfg!(debug) { + assert!(TreeNode::check_invariants( + self.root.load(Ordering::SeqCst, guard), + guard + )); + } + false } } @@ -659,34 +722,38 @@ where self.first.store(tree_node, Ordering::Release); return Shared::null(); } + // safety: we were read under our guard, at which point the tree + // structure was valid. Since our guard pins the current epoch, the + // TreeNodes remain valid for at least as long as we hold onto the + // guard. + // Structurally, TreeNodes always point to TreeNodes, so this is sound. loop { - let p_deref = TreeNode::get_tree_node(p); + let p_deref = unsafe { TreeNode::get_tree_node(p) }; let p_hash = p_deref.node.hash; - let dir: i8; - if p_hash > hash { - dir = -1; - } else if p_hash < hash { - dir = 1; - } else { - let p_key = &p_deref.node.key; - if *p_key == key { - // a node with the given key already exists, so we return it - return p; - } - dir = match p_key.cmp(&key) { - std::cmp::Ordering::Greater => -1, - std::cmp::Ordering::Less => 1, - std::cmp::Ordering::Equal => { - unreachable!("Ord and Eq have to match and Eq is checked above") + let dir = match p_hash.cmp(&hash) { + std::cmp::Ordering::Greater => -1, + std::cmp::Ordering::Less => 1, + std::cmp::Ordering::Equal => { + let p_key = &p_deref.node.key; + if *p_key == key { + // a node with the given key already exists, so we return it + return p; } - }; - // NOTE: the Java code has some addional cases here in case the - // keys _are not_ equal (p_key != key and !key.equals(p_key)), - // but _compare_ equal (k.compareTo(p_key) == 0). In this case, - // both children are searched and if a matching node exists it - // is returned. Since `Eq` and `Ord` must match, these cases - // cannot occur here. - } + match p_key.cmp(&key) { + std::cmp::Ordering::Greater => -1, + std::cmp::Ordering::Less => 1, + std::cmp::Ordering::Equal => { + unreachable!("Ord and Eq have to match and Eq is checked above") + } + } + // NOTE: the Java code has some addional cases here in case the + // keys _are not_ equal (p_key != key and !key.equals(p_key)), + // but _compare_ equal (k.compareTo(p_key) == 0). In this case, + // both children are searched and if a matching node exists it + // is returned. Since `Eq` and `Ord` must match, these cases + // cannot occur here. + } + }; // proceed in direction `dir` let xp = p; @@ -711,18 +778,27 @@ where .into_shared(guard); self.first.store(x, Ordering::SeqCst); if !first.is_null() { - TreeNode::get_tree_node(first) + unsafe { TreeNode::get_tree_node(first) } .prev .store(x, Ordering::SeqCst); } if dir <= 0 { - TreeNode::get_tree_node(xp).left.store(x, Ordering::SeqCst); + unsafe { TreeNode::get_tree_node(xp) } + .left + .store(x, Ordering::SeqCst); } else { - TreeNode::get_tree_node(xp).right.store(x, Ordering::SeqCst); + unsafe { TreeNode::get_tree_node(xp) } + .right + .store(x, Ordering::SeqCst); } - if !TreeNode::get_tree_node(xp).red.load(Ordering::SeqCst) { - TreeNode::get_tree_node(x).red.store(true, Ordering::SeqCst); + if !unsafe { TreeNode::get_tree_node(xp) } + .red + .load(Ordering::SeqCst) + { + unsafe { TreeNode::get_tree_node(x) } + .red + .store(true, Ordering::SeqCst); } else { self.lock_root(); self.root.store( @@ -739,11 +815,13 @@ where } } - assert!(TreeNode::check_invariants( - self.root.load(Ordering::SeqCst, guard), - guard - )); - return Shared::null(); + if cfg!(debug) { + assert!(TreeNode::check_invariants( + self.root.load(Ordering::SeqCst, guard), + guard + )); + } + Shared::null() } } @@ -757,7 +835,10 @@ impl Drop for TreeBin { // assume ownership of atomically shared references. note that it is // sufficient to follow the `next` pointers of the `first` element in // the bin, since the tree pointers point to the same nodes. - let _waiter = unsafe { self.waiter.load(Ordering::SeqCst, guard).into_owned() }; + let waiter = self.waiter.load(Ordering::SeqCst, guard); + if !waiter.is_null() { + let _ = unsafe { waiter.into_owned() }; + } let p = self.first.load(Ordering::SeqCst, guard); if p.is_null() { // this TreeBin is empty @@ -785,8 +866,16 @@ impl Drop for TreeBin { // Red-black tree methods, all adapted from CLR impl TreeNode { + /// Gets the `BinEntry::TreeNode(tree_node)` behind the given pointer and + /// returns its `tree_node`. + /// + /// # Safety + /// All safety concers of [`deref`](Shared::deref) apply. In particular, the + /// supplied pointer must be non-null and must point to valid memory. + /// Additionally, it must point to an instance of BinEntry that is actually a + /// TreeNode. #[inline] - pub(crate) fn get_tree_node<'g>(bin: Shared<'g, BinEntry>) -> &'g TreeNode { + pub(crate) unsafe fn get_tree_node<'g>(bin: Shared<'g, BinEntry>) -> &'g TreeNode { bin.deref().as_tree_node().unwrap() } @@ -798,14 +887,19 @@ impl TreeNode { if p.is_null() { return root; } - let p_deref = Self::get_tree_node(p); + // safety: the containing TreeBin of all TreeNodes was read under our + // guard, at which point the tree structure was valid. Since our guard + // pins the current epoch, the TreeNodes remain valid for at least as + // long as we hold onto the guard. + // Structurally, TreeNodes always point to TreeNodes, so this is sound. + let p_deref = unsafe { Self::get_tree_node(p) }; let right = p_deref.right.load(Ordering::SeqCst, guard); if !right.is_null() { - let right_deref = Self::get_tree_node(right); + let right_deref = unsafe { Self::get_tree_node(right) }; let right_left = right_deref.left.load(Ordering::SeqCst, guard); p_deref.right.store(right_left, Ordering::SeqCst); if !right_left.is_null() { - Self::get_tree_node(right_left) + unsafe { Self::get_tree_node(right_left) } .parent .store(p, Ordering::SeqCst); } @@ -816,7 +910,7 @@ impl TreeNode { root = right; right_deref.red.store(false, Ordering::Relaxed); } else { - let p_parent_deref = Self::get_tree_node(p_parent); + let p_parent_deref = unsafe { Self::get_tree_node(p_parent) }; if p_parent_deref.left.load(Ordering::SeqCst, guard) == p { p_parent_deref.left.store(right, Ordering::SeqCst); } else { @@ -839,14 +933,19 @@ impl TreeNode { if p.is_null() { return root; } - let p_deref = Self::get_tree_node(p); + // safety: the containing TreeBin of all TreeNodes was read under our + // guard, at which point the tree structure was valid. Since our guard + // pins the current epoch, the TreeNodes remain valid for at least as + // long as we hold onto the guard. + // Structurally, TreeNodes always point to TreeNodes, so this is sound. + let p_deref = unsafe { Self::get_tree_node(p) }; let left = p_deref.left.load(Ordering::SeqCst, guard); if !left.is_null() { - let left_deref = Self::get_tree_node(left); + let left_deref = unsafe { Self::get_tree_node(left) }; let left_right = left_deref.right.load(Ordering::SeqCst, guard); p_deref.left.store(left_right, Ordering::SeqCst); if !left_right.is_null() { - Self::get_tree_node(left_right) + unsafe { Self::get_tree_node(left_right) } .parent .store(p, Ordering::SeqCst); } @@ -857,7 +956,7 @@ impl TreeNode { root = left; left_deref.red.store(false, Ordering::Relaxed); } else { - let p_parent_deref = Self::get_tree_node(p_parent); + let p_parent_deref = unsafe { Self::get_tree_node(p_parent) }; if p_parent_deref.right.load(Ordering::SeqCst, guard) == p { p_parent_deref.right.store(left, Ordering::SeqCst); } else { @@ -877,70 +976,84 @@ impl TreeNode { mut x: Shared<'g, BinEntry>, guard: &'g Guard, ) -> Shared<'g, BinEntry> { - Self::get_tree_node(x).red.store(true, Ordering::Relaxed); + // safety: the containing TreeBin of all TreeNodes was read under our + // guard, at which point the tree structure was valid. Since our guard + // pins the current epoch, the TreeNodes remain valid for at least as + // long as we hold onto the guard. + // Structurally, TreeNodes always point to TreeNodes, so this is sound. + unsafe { Self::get_tree_node(x) } + .red + .store(true, Ordering::Relaxed); let mut x_parent: Shared<'_, BinEntry>; let mut x_parent_parent: Shared<'_, BinEntry>; let mut x_parent_parent_left: Shared<'_, BinEntry>; let mut x_parent_parent_right: Shared<'_, BinEntry>; loop { - x_parent = Self::get_tree_node(x).parent.load(Ordering::SeqCst, guard); + x_parent = unsafe { Self::get_tree_node(x) } + .parent + .load(Ordering::SeqCst, guard); if x_parent.is_null() { - Self::get_tree_node(x).red.store(false, Ordering::Relaxed); + unsafe { Self::get_tree_node(x) } + .red + .store(false, Ordering::Relaxed); return x; } - x_parent_parent = Self::get_tree_node(x_parent) + x_parent_parent = unsafe { Self::get_tree_node(x_parent) } .parent .load(Ordering::SeqCst, guard); - if !Self::get_tree_node(x_parent).red.load(Ordering::Relaxed) + if !unsafe { Self::get_tree_node(x_parent) } + .red + .load(Ordering::Relaxed) || x_parent_parent.is_null() { return root; } - x_parent_parent_left = Self::get_tree_node(x_parent_parent) + x_parent_parent_left = unsafe { Self::get_tree_node(x_parent_parent) } .left .load(Ordering::SeqCst, guard); if x_parent == x_parent_parent_left { - x_parent_parent_right = Self::get_tree_node(x_parent_parent) + x_parent_parent_right = unsafe { Self::get_tree_node(x_parent_parent) } .right .load(Ordering::SeqCst, guard); if !x_parent_parent_right.is_null() - && Self::get_tree_node(x_parent_parent_right) + && unsafe { Self::get_tree_node(x_parent_parent_right) } .red .load(Ordering::Relaxed) { - Self::get_tree_node(x_parent_parent_right) + unsafe { Self::get_tree_node(x_parent_parent_right) } .red .store(false, Ordering::Relaxed); - Self::get_tree_node(x_parent) + unsafe { Self::get_tree_node(x_parent) } .red .store(false, Ordering::Relaxed); - Self::get_tree_node(x_parent_parent) + unsafe { Self::get_tree_node(x_parent_parent) } .red .store(true, Ordering::Relaxed); x = x_parent_parent; } else { - if x == Self::get_tree_node(x_parent) + if x == unsafe { Self::get_tree_node(x_parent) } .right .load(Ordering::SeqCst, guard) { x = x_parent; root = Self::rotate_left(root, x, guard); - x_parent = Self::get_tree_node(x).parent.load(Ordering::SeqCst, guard); + x_parent = + unsafe { Self::get_tree_node(x).parent.load(Ordering::SeqCst, guard) }; x_parent_parent = if x_parent.is_null() { Shared::null() } else { - Self::get_tree_node(x_parent) + unsafe { Self::get_tree_node(x_parent) } .parent .load(Ordering::SeqCst, guard) }; } if !x_parent.is_null() { - Self::get_tree_node(x_parent) + unsafe { Self::get_tree_node(x_parent) } .red .store(false, Ordering::Relaxed); if !x_parent_parent.is_null() { - Self::get_tree_node(x_parent_parent) + unsafe { Self::get_tree_node(x_parent_parent) } .red .store(true, Ordering::Relaxed); root = Self::rotate_right(root, x_parent_parent, guard); @@ -948,42 +1061,44 @@ impl TreeNode { } } } else if !x_parent_parent_left.is_null() - && Self::get_tree_node(x_parent_parent_left) + && unsafe { Self::get_tree_node(x_parent_parent_left) } .red .load(Ordering::Relaxed) { - Self::get_tree_node(x_parent_parent_left) + unsafe { Self::get_tree_node(x_parent_parent_left) } .red .store(false, Ordering::Relaxed); - Self::get_tree_node(x_parent) + unsafe { Self::get_tree_node(x_parent) } .red .store(false, Ordering::Relaxed); - Self::get_tree_node(x_parent_parent) + unsafe { Self::get_tree_node(x_parent_parent) } .red .store(true, Ordering::Relaxed); x = x_parent_parent; } else { - if x == Self::get_tree_node(x_parent) + if x == unsafe { Self::get_tree_node(x_parent) } .left .load(Ordering::SeqCst, guard) { x = x_parent; root = Self::rotate_right(root, x, guard); - x_parent = Self::get_tree_node(x).parent.load(Ordering::SeqCst, guard); + x_parent = unsafe { Self::get_tree_node(x) } + .parent + .load(Ordering::SeqCst, guard); x_parent_parent = if x_parent.is_null() { Shared::null() } else { - Self::get_tree_node(x_parent) + unsafe { Self::get_tree_node(x_parent) } .parent .load(Ordering::SeqCst, guard) }; } if !x_parent.is_null() { - Self::get_tree_node(x_parent) + unsafe { Self::get_tree_node(x_parent) } .red .store(false, Ordering::Relaxed); if !x_parent_parent.is_null() { - Self::get_tree_node(x_parent_parent) + unsafe { Self::get_tree_node(x_parent_parent) } .red .store(true, Ordering::Relaxed); root = Self::rotate_left(root, x_parent_parent, guard); @@ -1001,39 +1116,50 @@ impl TreeNode { let mut x_parent: Shared<'_, BinEntry>; let mut x_parent_left: Shared<'_, BinEntry>; let mut x_parent_right: Shared<'_, BinEntry>; + // safety: the containing TreeBin of all TreeNodes was read under our + // guard, at which point the tree structure was valid. Since our guard + // pins the current epoch, the TreeNodes remain valid for at least as + // long as we hold onto the guard. + // Structurally, TreeNodes always point to TreeNodes, so this is sound. loop { if x.is_null() || x == root { return root; } - x_parent = Self::get_tree_node(x).parent.load(Ordering::SeqCst, guard); + x_parent = unsafe { Self::get_tree_node(x) } + .parent + .load(Ordering::SeqCst, guard); if x_parent.is_null() { - Self::get_tree_node(x).red.store(false, Ordering::Relaxed); + unsafe { Self::get_tree_node(x) } + .red + .store(false, Ordering::Relaxed); return x; } - x_parent_left = Self::get_tree_node(x_parent) + x_parent_left = unsafe { Self::get_tree_node(x_parent) } .left .load(Ordering::SeqCst, guard); if x_parent_left == x { - x_parent_right = Self::get_tree_node(x_parent) + x_parent_right = unsafe { Self::get_tree_node(x_parent) } .right .load(Ordering::SeqCst, guard); if !x_parent_right.is_null() - && Self::get_tree_node(x_parent_right) + && unsafe { Self::get_tree_node(x_parent_right) } .red .load(Ordering::Relaxed) { - Self::get_tree_node(x_parent_right) + unsafe { Self::get_tree_node(x_parent_right) } .red .store(false, Ordering::Relaxed); - Self::get_tree_node(x_parent) + unsafe { Self::get_tree_node(x_parent) } .red .store(true, Ordering::Relaxed); root = Self::rotate_left(root, x_parent, guard); - x_parent = Self::get_tree_node(x).parent.load(Ordering::SeqCst, guard); + x_parent = unsafe { Self::get_tree_node(x) } + .parent + .load(Ordering::SeqCst, guard); x_parent_right = if x_parent.is_null() { Shared::null() } else { - Self::get_tree_node(x_parent) + unsafe { Self::get_tree_node(x_parent) } .right .load(Ordering::SeqCst, guard) }; @@ -1041,64 +1167,74 @@ impl TreeNode { if x_parent_right.is_null() { x = x_parent; } else { - let s_left = Self::get_tree_node(x_parent_right) + let s_left = unsafe { Self::get_tree_node(x_parent_right) } .left .load(Ordering::SeqCst, guard); - let mut s_right = Self::get_tree_node(x_parent_right) + let mut s_right = unsafe { Self::get_tree_node(x_parent_right) } .right .load(Ordering::SeqCst, guard); if (s_right.is_null() - || !Self::get_tree_node(s_right).red.load(Ordering::Relaxed)) + || !unsafe { Self::get_tree_node(s_right) } + .red + .load(Ordering::Relaxed)) && (s_left.is_null() - || !Self::get_tree_node(s_left).red.load(Ordering::Relaxed)) + || !unsafe { Self::get_tree_node(s_left) } + .red + .load(Ordering::Relaxed)) { - Self::get_tree_node(x_parent_right) + unsafe { Self::get_tree_node(x_parent_right) } .red .store(true, Ordering::Relaxed); x = x_parent; } else { if s_right.is_null() - || !Self::get_tree_node(s_right).red.load(Ordering::Relaxed) + || !unsafe { Self::get_tree_node(s_right) } + .red + .load(Ordering::Relaxed) { if !s_left.is_null() { - Self::get_tree_node(s_left) + unsafe { Self::get_tree_node(s_left) } .red .store(false, Ordering::Relaxed); } - Self::get_tree_node(x_parent_right) + unsafe { Self::get_tree_node(x_parent_right) } .red .store(true, Ordering::Relaxed); root = Self::rotate_right(root, x_parent_right, guard); - x_parent = Self::get_tree_node(x).parent.load(Ordering::SeqCst, guard); + x_parent = unsafe { Self::get_tree_node(x) } + .parent + .load(Ordering::SeqCst, guard); x_parent_right = if x_parent.is_null() { Shared::null() } else { - Self::get_tree_node(x_parent) + unsafe { Self::get_tree_node(x_parent) } .right .load(Ordering::SeqCst, guard) }; } if !x_parent_right.is_null() { - Self::get_tree_node(x_parent_right).red.store( + unsafe { Self::get_tree_node(x_parent_right) }.red.store( if x_parent.is_null() { false } else { - Self::get_tree_node(x_parent).red.load(Ordering::Relaxed) + unsafe { Self::get_tree_node(x_parent) } + .red + .load(Ordering::Relaxed) }, Ordering::Relaxed, ); - s_right = Self::get_tree_node(x_parent_right) + s_right = unsafe { Self::get_tree_node(x_parent_right) } .right .load(Ordering::SeqCst, guard); if !s_right.is_null() { - Self::get_tree_node(s_right) + unsafe { Self::get_tree_node(s_right) } .red .store(false, Ordering::Relaxed); } } if !x_parent.is_null() { - Self::get_tree_node(x_parent) + unsafe { Self::get_tree_node(x_parent) } .red .store(false, Ordering::Relaxed); root = Self::rotate_left(root, x_parent, guard); @@ -1109,22 +1245,24 @@ impl TreeNode { } else { // symmetric if !x_parent_left.is_null() - && Self::get_tree_node(x_parent_left) + && unsafe { Self::get_tree_node(x_parent_left) } .red .load(Ordering::Relaxed) { - Self::get_tree_node(x_parent_left) + unsafe { Self::get_tree_node(x_parent_left) } .red .store(false, Ordering::Relaxed); - Self::get_tree_node(x_parent) + unsafe { Self::get_tree_node(x_parent) } .red .store(true, Ordering::Relaxed); root = Self::rotate_right(root, x_parent, guard); - x_parent = Self::get_tree_node(x).parent.load(Ordering::SeqCst, guard); + x_parent = unsafe { Self::get_tree_node(x) } + .parent + .load(Ordering::SeqCst, guard); x_parent_left = if x_parent.is_null() { Shared::null() } else { - Self::get_tree_node(x_parent) + unsafe { Self::get_tree_node(x_parent) } .left .load(Ordering::SeqCst, guard) }; @@ -1132,64 +1270,74 @@ impl TreeNode { if x_parent_left.is_null() { x = x_parent; } else { - let mut s_left = Self::get_tree_node(x_parent_left) + let mut s_left = unsafe { Self::get_tree_node(x_parent_left) } .left .load(Ordering::SeqCst, guard); - let s_right = Self::get_tree_node(x_parent_left) + let s_right = unsafe { Self::get_tree_node(x_parent_left) } .right .load(Ordering::SeqCst, guard); if (s_right.is_null() - || !Self::get_tree_node(s_right).red.load(Ordering::Relaxed)) + || !unsafe { Self::get_tree_node(s_right) } + .red + .load(Ordering::Relaxed)) && (s_left.is_null() - || !Self::get_tree_node(s_left).red.load(Ordering::Relaxed)) + || !unsafe { Self::get_tree_node(s_left) } + .red + .load(Ordering::Relaxed)) { - Self::get_tree_node(x_parent_left) + unsafe { Self::get_tree_node(x_parent_left) } .red .store(true, Ordering::Relaxed); x = x_parent; } else { if s_left.is_null() - || !Self::get_tree_node(s_left).red.load(Ordering::Relaxed) + || !unsafe { Self::get_tree_node(s_left) } + .red + .load(Ordering::Relaxed) { if !s_right.is_null() { - Self::get_tree_node(s_right) + unsafe { Self::get_tree_node(s_right) } .red .store(false, Ordering::Relaxed); } - Self::get_tree_node(x_parent_left) + unsafe { Self::get_tree_node(x_parent_left) } .red .store(true, Ordering::Relaxed); root = Self::rotate_left(root, x_parent_left, guard); - x_parent = Self::get_tree_node(x).parent.load(Ordering::SeqCst, guard); + x_parent = unsafe { Self::get_tree_node(x) } + .parent + .load(Ordering::SeqCst, guard); x_parent_left = if x_parent.is_null() { Shared::null() } else { - Self::get_tree_node(x_parent) + unsafe { Self::get_tree_node(x_parent) } .left .load(Ordering::SeqCst, guard) }; } if !x_parent_left.is_null() { - Self::get_tree_node(x_parent_left).red.store( + unsafe { Self::get_tree_node(x_parent_left) }.red.store( if x_parent.is_null() { false } else { - Self::get_tree_node(x_parent).red.load(Ordering::Relaxed) + unsafe { Self::get_tree_node(x_parent) } + .red + .load(Ordering::Relaxed) }, Ordering::Relaxed, ); - s_left = Self::get_tree_node(x_parent_left) + s_left = unsafe { Self::get_tree_node(x_parent_left) } .left .load(Ordering::SeqCst, guard); if !s_left.is_null() { - Self::get_tree_node(s_left) + unsafe { Self::get_tree_node(s_left) } .red .store(false, Ordering::Relaxed); } } if !x_parent.is_null() { - Self::get_tree_node(x_parent) + unsafe { Self::get_tree_node(x_parent) } .red .store(false, Ordering::Relaxed); root = Self::rotate_right(root, x_parent, guard); @@ -1202,7 +1350,12 @@ impl TreeNode { } /// Checks invariants recursively for the tree of Nodes rootet at t. fn check_invariants<'g>(t: Shared<'g, BinEntry>, guard: &'g Guard) -> bool { - let t_deref = Self::get_tree_node(t); + // safety: the containing TreeBin of all TreeNodes was read under our + // guard, at which point the tree structure was valid. Since our guard + // pins the current epoch, the TreeNodes remain valid for at least as + // long as we hold onto the guard. + // Structurally, TreeNodes always point to TreeNodes, so this is sound. + let t_deref = unsafe { Self::get_tree_node(t) }; let t_parent = t_deref.parent.load(Ordering::SeqCst, guard); let t_left = t_deref.left.load(Ordering::SeqCst, guard); let t_right = t_deref.right.load(Ordering::SeqCst, guard); @@ -1210,19 +1363,19 @@ impl TreeNode { let t_next = t_deref.node.next.load(Ordering::SeqCst, guard); if !t_back.is_null() { - let t_back_deref = Self::get_tree_node(t_back); + let t_back_deref = unsafe { Self::get_tree_node(t_back) }; if t_back_deref.node.next.load(Ordering::SeqCst, guard) != t { return false; } } if !t_next.is_null() { - let t_next_deref = Self::get_tree_node(t_next); + let t_next_deref = unsafe { Self::get_tree_node(t_next) }; if t_next_deref.prev.load(Ordering::SeqCst, guard) != t { return false; } } if !t_parent.is_null() { - let t_parent_deref = Self::get_tree_node(t_parent); + let t_parent_deref = unsafe { Self::get_tree_node(t_parent) }; if t_parent_deref.left.load(Ordering::SeqCst, guard) != t && t_parent_deref.right.load(Ordering::SeqCst, guard) != t { @@ -1230,7 +1383,7 @@ impl TreeNode { } } if !t_left.is_null() { - let t_left_deref = Self::get_tree_node(t_left); + let t_left_deref = unsafe { Self::get_tree_node(t_left) }; if t_left_deref.parent.load(Ordering::SeqCst, guard) != t || t_left_deref.node.hash > t_deref.node.hash { @@ -1238,7 +1391,7 @@ impl TreeNode { } } if !t_right.is_null() { - let t_right_deref = Self::get_tree_node(t_right); + let t_right_deref = unsafe { Self::get_tree_node(t_right) }; if t_right_deref.parent.load(Ordering::SeqCst, guard) != t || t_right_deref.node.hash < t_deref.node.hash { @@ -1246,20 +1399,15 @@ impl TreeNode { } } if t_deref.red.load(Ordering::Relaxed) && !t_left.is_null() && !t_right.is_null() { - let t_left_deref = Self::get_tree_node(t_left); - let t_right_deref = Self::get_tree_node(t_right); + let t_left_deref = unsafe { Self::get_tree_node(t_left) }; + let t_right_deref = unsafe { Self::get_tree_node(t_right) }; if t_left_deref.red.load(Ordering::Relaxed) && t_right_deref.red.load(Ordering::Relaxed) { return false; } } - if !t_left.is_null() && !Self::check_invariants(t_left, guard) { - false - } else if !t_right.is_null() && !Self::check_invariants(t_right, guard) { - false - } else { - true - } + !(!t_left.is_null() && !Self::check_invariants(t_left, guard) + || !t_right.is_null() && !Self::check_invariants(t_right, guard)) } } diff --git a/tests/jdk/map_check.rs b/tests/jdk/map_check.rs index 2bc5ea1f..e376e3d4 100644 --- a/tests/jdk/map_check.rs +++ b/tests/jdk/map_check.rs @@ -9,7 +9,7 @@ const ABSENT_MASK: usize = ABSENT_SIZE - 1; fn t1(map: &HashMap, keys: &[K], expect: usize) where - K: Sync + Send + Clone + Hash + Eq, + K: Sync + Send + Clone + Hash + Ord, V: Sync + Send, { let mut sum = 0; @@ -27,7 +27,7 @@ where fn t2(map: &HashMap, keys: &[K], expect: usize) where - K: 'static + Sync + Send + Copy + Hash + Eq + std::fmt::Display, + K: 'static + Sync + Send + Copy + Hash + Ord + std::fmt::Display, { let mut sum = 0; let guard = epoch::pin(); @@ -41,7 +41,7 @@ where fn t3(map: &HashMap, keys: &[K], expect: usize) where - K: 'static + Sync + Send + Copy + Hash + Eq, + K: 'static + Sync + Send + Copy + Hash + Ord, { let mut sum = 0; let guard = epoch::pin(); @@ -55,7 +55,7 @@ where fn t4(map: &HashMap, keys: &[K], expect: usize) where - K: Sync + Send + Copy + Hash + Eq, + K: Sync + Send + Copy + Hash + Ord, { let mut sum = 0; let guard = epoch::pin(); @@ -69,7 +69,7 @@ where fn t5(map: &HashMap, keys: &[K], expect: usize) where - K: 'static + Sync + Send + Copy + Hash + Eq, + K: 'static + Sync + Send + Copy + Hash + Ord, { let mut sum = 0; let guard = epoch::pin(); @@ -85,7 +85,7 @@ where fn t6(map: &HashMap, keys1: &[K], keys2: &[K], expect: usize) where - K: Sync + Send + Clone + Hash + Eq, + K: Sync + Send + Clone + Hash + Ord, V: Sync + Send, { let mut sum = 0; @@ -103,7 +103,7 @@ where fn t7(map: &HashMap, k1: &[K], k2: &[K]) where - K: Sync + Send + Copy + Hash + Eq, + K: Sync + Send + Copy + Hash + Ord, { let mut sum = 0; let guard = epoch::pin();