Skip to content

Commit

Permalink
Move borrowed keys out of node operation
Browse files Browse the repository at this point in the history
**Description**
 - Move the "borrowed key" pattern of `Q where K: Borrow<Q> + AsBytes,
   Q: AsBytes + ?Sized` out of the node operations
 - Remove some more unneeded instances of `: AsBytes`

**Motivation**
The "borrowed key" pattern is largely only important at the TreeMap
API level. Beyond that, only the key bytes and concrete key type are
needed.

**Testing Done**
`./scripts/full-test.sh nightly`
  • Loading branch information
declanvk committed Jul 8, 2024
1 parent 64f97e7 commit ca5fcae
Show file tree
Hide file tree
Showing 10 changed files with 55 additions and 123 deletions.
12 changes: 6 additions & 6 deletions src/collections/map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ impl<K, V, const PREFIX_LEN: usize> TreeMap<K, V, PREFIX_LEN> {
// means there can only exist other immutable references aside from this one,
// and no mutable references. That means that no mutating operations can occur
// on the root node or any child of the root node.
let search_result = unsafe { search_unchecked(root, key)? };
let search_result = unsafe { search_unchecked(root, key.as_bytes())? };

// SAFETY: The lifetime chosen the value reference is bounded by the lifetime of
// the immutable reference to the `TreeMap`. The memory of the value will not be
Expand Down Expand Up @@ -196,7 +196,7 @@ impl<K, V, const PREFIX_LEN: usize> TreeMap<K, V, PREFIX_LEN> {
// means there cannot exist any other reference (mutable or immutable) to the
// same `TreeMap`. Which means that no other mutating operations could be
// happening during the `search_unchecked` call.
let search_result = unsafe { search_unchecked(root, key)? };
let search_result = unsafe { search_unchecked(root, key.as_bytes())? };

// SAFETY: The lifetime chosen the value reference is bounded by the lifetime of
// the mutable reference to the `TreeMap`. The value pointed to by the returned
Expand Down Expand Up @@ -639,7 +639,7 @@ impl<K, V, const PREFIX_LEN: usize> TreeMap<K, V, PREFIX_LEN> {
// that there are no other references (mutable or immutable) to this same
// object. Meaning that our access to the root node is unique and there are no
// other accesses to any node in the tree.
let insert_point = unsafe { search_for_insert_point(root, &key)? };
let insert_point = unsafe { search_for_insert_point(root, key.as_bytes())? };
let insert_result = self.apply_insert_point(insert_point, key, value);
Ok(insert_result.existing_leaf.map(|leaf| leaf.into_entry().1))
} else {
Expand Down Expand Up @@ -673,7 +673,7 @@ impl<K, V, const PREFIX_LEN: usize> TreeMap<K, V, PREFIX_LEN> {
// that there are no other references (mutable or immutable) to this same
// object. Meaning that our access to the root node is unique and there are no
// other accesses to any node in the tree.
let delete_point = unsafe { search_for_delete_point(root, key)? };
let delete_point = unsafe { search_for_delete_point(root, key.as_bytes())? };
let delete_result = self.apply_delete_point(delete_point);
Some(delete_result.deleted_leaf.into_entry())
} else {
Expand Down Expand Up @@ -1273,7 +1273,7 @@ impl<K, V, const PREFIX_LEN: usize> TreeMap<K, V, PREFIX_LEN> {
// that there are no other references (mutable or immutable) to this same
// object. Meaning that our access to the root node is unique and there are no
// other accesses to any node in the tree.
let insert_point = unsafe { search_for_insert_point(root, &key)? };
let insert_point = unsafe { search_for_insert_point(root, key.as_bytes())? };
match insert_point.insert_type {
Exact { leaf_node_ptr } => Entry::Occupied(OccupiedEntry {
map: self,
Expand Down Expand Up @@ -1314,7 +1314,7 @@ impl<K, V, const PREFIX_LEN: usize> TreeMap<K, V, PREFIX_LEN> {
// that there are no other references (mutable or immutable) to this same
// object. Meaning that our access to the root node is unique and there are no
// other accesses to any node in the tree.
let insert_point = unsafe { search_for_insert_point(root, key)? };
let insert_point = unsafe { search_for_insert_point(root, key.as_bytes())? };
match insert_point.insert_type {
Exact { leaf_node_ptr } => EntryRef::Occupied(OccupiedEntryRef {
map: self,
Expand Down
21 changes: 9 additions & 12 deletions src/nodes/operations/delete.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
use std::borrow::Borrow;

use crate::{
nodes::operations::lookup, AsBytes, ConcreteNodePtr, InnerNode, LeafNode, NodePtr,
OpaqueNodePtr,
Expand Down Expand Up @@ -294,13 +292,12 @@ impl<K, V, const PREFIX_LEN: usize> DeletePoint<K, V, PREFIX_LEN> {
/// - This function cannot be called concurrently with any mutating operation
/// on `root` or any child node of `root`. This function will arbitrarily
/// read to any child in the given tree.
pub unsafe fn search_for_delete_point<Q, K, V, const PREFIX_LEN: usize>(
pub unsafe fn search_for_delete_point<K, V, const PREFIX_LEN: usize>(
root: OpaqueNodePtr<K, V, PREFIX_LEN>,
key: &Q,
key_bytes: &[u8],
) -> Option<DeletePoint<K, V, PREFIX_LEN>>
where
K: Borrow<Q> + AsBytes,
Q: AsBytes + ?Sized,
K: AsBytes,
{
let mut current_grandparent = None;
let mut current_parent = None;
Expand All @@ -312,29 +309,29 @@ where
ConcreteNodePtr::Node4(inner_ptr) => unsafe {
// SAFETY: The safety requirement is covered by the safety requirement on the
// containing function
lookup::check_prefix_lookup_child(inner_ptr, key, &mut current_depth)
lookup::check_prefix_lookup_child(inner_ptr, key_bytes, &mut current_depth)
},
ConcreteNodePtr::Node16(inner_ptr) => unsafe {
// SAFETY: The safety requirement is covered by the safety requirement on the
// containing function
lookup::check_prefix_lookup_child(inner_ptr, key, &mut current_depth)
lookup::check_prefix_lookup_child(inner_ptr, key_bytes, &mut current_depth)
},
ConcreteNodePtr::Node48(inner_ptr) => unsafe {
// SAFETY: The safety requirement is covered by the safety requirement on the
// containing function
lookup::check_prefix_lookup_child(inner_ptr, key, &mut current_depth)
lookup::check_prefix_lookup_child(inner_ptr, key_bytes, &mut current_depth)
},
ConcreteNodePtr::Node256(inner_ptr) => unsafe {
// SAFETY: The safety requirement is covered by the safety requirement on the
// containing function
lookup::check_prefix_lookup_child(inner_ptr, key, &mut current_depth)
lookup::check_prefix_lookup_child(inner_ptr, key_bytes, &mut current_depth)
},
ConcreteNodePtr::LeafNode(leaf_node_ptr) => {
let leaf_node = leaf_node_ptr.read();

// Specifically we are matching the leaf node stored key against the full search
// key to confirm that it is the right value.
if leaf_node.matches_full_key(key) {
if leaf_node.matches_full_key(key_bytes) {
return Some(DeletePoint {
grandparent_ptr_and_parent_key_byte: current_grandparent,
parent_ptr_and_child_key_byte: current_parent,
Expand All @@ -352,7 +349,7 @@ where
);

// This should not panic because the current_depth will be greater than zero
let last_key_byte = key.as_bytes()[current_depth - 1];
let last_key_byte = key_bytes[current_depth - 1];

current_grandparent = current_parent;
current_parent = Some((current_node, last_key_byte));
Expand Down
12 changes: 5 additions & 7 deletions src/nodes/operations/insert.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use crate::{
AsBytes, ConcreteNodePtr, InnerNode, InnerNode4, LeafNode, MatchPrefixResult, Mismatch,
NodePtr, OpaqueNodePtr,
};
use std::{borrow::Borrow, error::Error, fmt, marker::PhantomData, ops::ControlFlow};
use std::{error::Error, fmt, marker::PhantomData, ops::ControlFlow};

/// The results of a successful tree insert
#[derive(Debug)]
Expand Down Expand Up @@ -439,13 +439,12 @@ pub enum InsertSearchResultType<K, V, const PREFIX_LEN: usize> {
/// # Errors
/// - If the given `key` is a prefix of an existing key, this function will
/// return an error.
pub unsafe fn search_for_insert_point<K, V, Q, const PREFIX_LEN: usize>(
pub unsafe fn search_for_insert_point<K, V, const PREFIX_LEN: usize>(
root: OpaqueNodePtr<K, V, PREFIX_LEN>,
key: &Q,
key_bytes: &[u8],
) -> Result<InsertPoint<K, V, PREFIX_LEN>, InsertPrefixError>
where
K: AsBytes + Borrow<Q>,
Q: AsBytes + ?Sized,
K: AsBytes,
{
fn test_prefix_identify_insert<K, V, N, const PREFIX_LEN: usize>(
inner_ptr: NodePtr<PREFIX_LEN, N>,
Expand Down Expand Up @@ -500,7 +499,6 @@ where
let mut current_parent = None;
let mut current_node = root;
let mut current_depth = 0;
let key_bytes = key.as_bytes();

loop {
let lookup_result = match current_node.to_node_ptr() {
Expand All @@ -519,7 +517,7 @@ where
ConcreteNodePtr::LeafNode(leaf_node_ptr) => {
let leaf_node = leaf_node_ptr.read();

if leaf_node.matches_full_key(key) {
if leaf_node.matches_full_key(key_bytes) {
return Ok(InsertPoint {
key_bytes_used: current_depth,
grandparent_ptr_and_parent_key_byte: current_grandparent,
Expand Down
6 changes: 3 additions & 3 deletions src/nodes/operations/insert/tests.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
use crate::{
deallocate_tree, search_unchecked,
tests_common::{generate_keys_skewed, insert_unchecked, setup_tree_from_entries},
AsBytes, InnerNode, InnerNode4, InnerNodeCompressed, InsertPrefixError, LeafNode, NodePtr,
NodeType, OpaqueNodePtr,
InnerNode, InnerNode4, InnerNodeCompressed, InsertPrefixError, LeafNode, NodePtr, NodeType,
OpaqueNodePtr,
};

#[test]
Expand Down Expand Up @@ -271,7 +271,7 @@ fn insert_existing_key_overwrite() {

let current_root: OpaqueNodePtr<Box<[u8]>, char, 16> = setup_tree_from_entries(entries_it);

unsafe fn get_value<K: AsBytes, V: Copy, const PREFIX_LEN: usize>(
unsafe fn get_value<K, V: Copy, const PREFIX_LEN: usize>(
n: NodePtr<PREFIX_LEN, LeafNode<K, V>>,
) -> V {
unsafe { *n.as_ref().value_ref() }
Expand Down
32 changes: 14 additions & 18 deletions src/nodes/operations/lookup.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
use std::borrow::Borrow;

use crate::{
AsBytes, ConcreteNodePtr, InnerNode, LeafNode, MatchPrefixResult, NodePtr, OpaqueNodePtr,
};
Expand All @@ -10,13 +8,12 @@ use crate::{
/// - This function cannot be called concurrently with any mutating operation
/// on `root` or any child node of `root`. This function will arbitrarily
/// read to any child in the given tree.
pub unsafe fn search_unchecked<Q, K, V, const PREFIX_LEN: usize>(
pub unsafe fn search_unchecked<K, V, const PREFIX_LEN: usize>(
root: OpaqueNodePtr<K, V, PREFIX_LEN>,
key: &Q,
key_bytes: &[u8],
) -> Option<NodePtr<PREFIX_LEN, LeafNode<K, V>>>
where
K: Borrow<Q> + AsBytes,
Q: AsBytes + ?Sized,
K: AsBytes,
{
let mut current_node = root;
let mut current_depth = 0;
Expand All @@ -26,29 +23,29 @@ where
ConcreteNodePtr::Node4(inner_ptr) => unsafe {
// SAFETY: The safety requirement is covered by the safety requirement on the
// containing function
check_prefix_lookup_child(inner_ptr, key, &mut current_depth)
check_prefix_lookup_child(inner_ptr, key_bytes, &mut current_depth)
},
ConcreteNodePtr::Node16(inner_ptr) => unsafe {
// SAFETY: The safety requirement is covered by the safety requirement on the
// containing function
check_prefix_lookup_child(inner_ptr, key, &mut current_depth)
check_prefix_lookup_child(inner_ptr, key_bytes, &mut current_depth)
},
ConcreteNodePtr::Node48(inner_ptr) => unsafe {
// SAFETY: The safety requirement is covered by the safety requirement on the
// containing function
check_prefix_lookup_child(inner_ptr, key, &mut current_depth)
check_prefix_lookup_child(inner_ptr, key_bytes, &mut current_depth)
},
ConcreteNodePtr::Node256(inner_ptr) => unsafe {
// SAFETY: The safety requirement is covered by the safety requirement on the
// containing function
check_prefix_lookup_child(inner_ptr, key, &mut current_depth)
check_prefix_lookup_child(inner_ptr, key_bytes, &mut current_depth)
},
ConcreteNodePtr::LeafNode(leaf_node_ptr) => {
let leaf_node = leaf_node_ptr.read();

// Specifically we are matching the leaf node stored key against the full search
// key to confirm that it is the right value.
if leaf_node.matches_full_key(key) {
if leaf_node.matches_full_key(key_bytes) {
return Some(leaf_node_ptr);
} else {
return None;
Expand All @@ -67,30 +64,29 @@ where
/// # Safety
/// - No other access or mutation to the `inner_ptr` Node can happen while this
/// function runs.
pub(crate) unsafe fn check_prefix_lookup_child<Q, K, V, N, const PREFIX_LEN: usize>(
pub(crate) unsafe fn check_prefix_lookup_child<K, V, N, const PREFIX_LEN: usize>(
inner_ptr: NodePtr<PREFIX_LEN, N>,
key: &Q,
key_bytes: &[u8],
current_depth: &mut usize,
) -> Option<OpaqueNodePtr<K, V, PREFIX_LEN>>
where
N: InnerNode<PREFIX_LEN, Key = K, Value = V>,
K: Borrow<Q> + AsBytes,
Q: AsBytes + ?Sized,
K: AsBytes,
{
// SAFETY: The lifetime produced from this is bounded to this scope and does not
// escape. Further, no other code mutates the node referenced, which is further
// enforced the "no concurrent reads or writes" requirement on the
// `search_unchecked` function.
let inner_node = unsafe { inner_ptr.as_ref() };
let match_prefix = inner_node.match_prefix(key.as_bytes(), *current_depth);
let match_prefix = inner_node.match_prefix(key_bytes, *current_depth);
match match_prefix {
MatchPrefixResult::Mismatch { .. } => None,
MatchPrefixResult::Match { matched_bytes } => {
// Since the prefix matched, advance the depth by the size of the prefix
*current_depth += matched_bytes;

let next_key_fragment = if *current_depth < key.as_bytes().len() {
key.as_bytes()[*current_depth]
let next_key_fragment = if *current_depth < key_bytes.len() {
key_bytes[*current_depth]
} else {
// the key has insufficient bytes, it is a prefix of an existing key. Thus, the
// key must not exist in the tree by the requirements of the insert function
Expand Down
10 changes: 4 additions & 6 deletions src/nodes/representation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

use crate::{rust_nightly_apis::assume, tagged_pointer::TaggedPointer, AsBytes};
use std::{
borrow::Borrow,
fmt,
hash::Hash,
iter::FusedIterator,
Expand Down Expand Up @@ -275,7 +274,7 @@ pub enum ConcreteNodePtr<K, V, const PREFIX_LEN: usize> {
LeafNode(NodePtr<PREFIX_LEN, LeafNode<K, V>>),
}

impl<K: AsBytes, V, const PREFIX_LEN: usize> fmt::Debug for ConcreteNodePtr<K, V, PREFIX_LEN> {
impl<K, V, const PREFIX_LEN: usize> fmt::Debug for ConcreteNodePtr<K, V, PREFIX_LEN> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
Self::Node4(arg0) => f.debug_tuple("Node4").field(arg0).finish(),
Expand Down Expand Up @@ -830,12 +829,11 @@ impl<K, V> LeafNode<K, V> {
}

/// Check that the provided full key is the same one as the stored key.
pub fn matches_full_key<Q>(&self, possible_key: &Q) -> bool
pub fn matches_full_key(&self, possible_key: &[u8]) -> bool
where
K: Borrow<Q> + AsBytes,
Q: AsBytes + ?Sized,
K: AsBytes,
{
self.key.borrow().as_bytes().eq(possible_key.as_bytes())
self.key.as_bytes().eq(possible_key)
}
}

Expand Down
12 changes: 6 additions & 6 deletions src/nodes/visitor/pretty_printer.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::{
visitor::{Visitable, Visitor},
AsBytes, InnerNode, NodeType, OpaqueNodePtr, TreeMap,
InnerNode, NodeType, OpaqueNodePtr, TreeMap,
};
use std::{
fmt::{Debug, Display},
Expand Down Expand Up @@ -32,7 +32,7 @@ impl<O: Write> DotPrinter<O> {
settings: DotPrinterSettings,
) -> Option<io::Result<()>>
where
K: Display + AsBytes,
K: Display,
V: Display,
{
tree.root.map(|root| {
Expand All @@ -53,7 +53,7 @@ impl<O: Write> DotPrinter<O> {
settings: DotPrinterSettings,
) -> io::Result<()>
where
K: Display + AsBytes,
K: Display,
V: Display,
{
let mut visitor = DotPrinter {
Expand Down Expand Up @@ -87,7 +87,7 @@ impl<O: Write> DotPrinter<O> {
inner_node: &N,
) -> io::Result<usize>
where
K: Display + AsBytes,
K: Display,
T: Display,
N: InnerNode<PREFIX_LEN, Key = K, Value = T>,
{
Expand Down Expand Up @@ -144,7 +144,7 @@ impl<O: Write> DotPrinter<O> {

impl<K, T, O, const PREFIX_LEN: usize> Visitor<K, T, PREFIX_LEN> for DotPrinter<O>
where
K: Display + AsBytes,
K: Display,
T: Display,
O: Write,
{
Expand Down Expand Up @@ -204,7 +204,7 @@ where

#[cfg(test)]
mod tests {
use crate::deallocate_tree;
use crate::{deallocate_tree, AsBytes};

use super::*;

Expand Down
6 changes: 3 additions & 3 deletions src/nodes/visitor/tree_stats.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,12 @@ impl TreeStatsCollector {
}

/// Iterate through the given tree and return the number of leaf nodes.
pub fn count_leaf_nodes<K: AsBytes, V, const PREFIX_LEN: usize>(
pub fn count_leaf_nodes<K, V, const PREFIX_LEN: usize>(
tree: &TreeMap<K, V, PREFIX_LEN>,
) -> usize {
struct LeafNodeCounter;

impl<K: AsBytes, V, const PREFIX_LEN: usize> Visitor<K, V, PREFIX_LEN> for LeafNodeCounter {
impl<K, V, const PREFIX_LEN: usize> Visitor<K, V, PREFIX_LEN> for LeafNodeCounter {
type Output = usize;

fn default_output(&self) -> Self::Output {
Expand Down Expand Up @@ -91,7 +91,7 @@ pub struct InnerNodeStats {
}

impl InnerNodeStats {
fn aggregate_data<K: AsBytes, V, const PREFIX_LEN: usize, N>(&mut self, t: &N)
fn aggregate_data<K, V, const PREFIX_LEN: usize, N>(&mut self, t: &N)
where
N: InnerNode<PREFIX_LEN, Key = K, Value = V>,
{
Expand Down
Loading

0 comments on commit ca5fcae

Please sign in to comment.