Skip to content
Draft
Changes from 4 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
179 changes: 159 additions & 20 deletions core-relations/src/containers/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use std::{
any::{Any, TypeId},
hash::{Hash, Hasher},
ops::Deref,
sync::{Arc, Mutex},
};

use crate::numeric_id::{DenseIdMap, IdVec, NumericId, define_id};
Expand Down Expand Up @@ -201,7 +202,149 @@ struct ContainerEnv<C: Eq + Hash> {
to_id: DashMap<C, Value>,
to_container: DashMap<Value, (usize /* hash code */, usize /* map */)>,
/// Map from a Value to the set of ids of containers that contain that value.
val_index: LazyMapOfIndexSet,
}
#[derive(Clone)]
struct LazyMapOfIndexSet {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: consider renaming to just LazyValIndex or LazyContainerIndex?

val_index: DashMap<Value, IndexSet<Value>>,
// keys and value to insert
// if user want to insert same value for all keys in IndexSet<Value>, LazyMap will put them
// in pending_insert and do the insertion for single key and remove this key in pending_insert when user want to read LazyMap
pending_operations: Arc<Mutex<Vec<(IndexSet<Value>, InsertOrRemove)>>>,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: it does not need to be an IndexSet, and a HashSet should be good enough?

}
enum InsertOrRemove {
Insert(Value),
Remove(Value),
}

const LAZY_BOUND: usize = 30;
use dashmap::mapref::one::{Ref, RefMut};
#[allow(dead_code)]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think we should be able to remove the dead code? Unused functions can be re-implemented later easily if we want to.

impl LazyMapOfIndexSet {
/// Creates a new, empty `LazyMapOfIndexSet`.
pub fn new() -> Self {
Self {
val_index: DashMap::default(),
pending_operations: Default::default(),
}
}

/// Returns the number of elements in the map.
pub fn len(&self) -> usize {
self.val_index.len()
}

/// Returns `true` if the map contains no elements.
pub fn is_empty(&self) -> bool {
self.val_index.is_empty()
}

/// Returns the number of elements the map can hold without reallocating.
pub fn capacity(&self) -> usize {
self.val_index.capacity()
}

/// Inserts a key-value pair into the map.
/// If the map did not have this key present, `None` is returned.
/// If the map did have this key present, the value is updated, and the old value is returned.
pub fn insert(&mut self, key: Value, value: IndexSet<Value>) -> Option<IndexSet<Value>> {
self.flush_pending_operations_for_key(&key);
self.val_index.insert(key, value)
}

/// Removes a key from the map, returning the value at the key if the key was previously in the map.
pub fn remove(&mut self, key: &Value) -> Option<IndexSet<Value>> {
self.flush_pending_operations_for_key(key);
self.val_index.remove(key).map(|(_, v)| v)
}

/// Removes a key from the map, returning the stored key and value if the key was previously in the map.
pub fn remove_entry(&mut self, key: &Value) -> Option<(Value, IndexSet<Value>)> {
self.flush_pending_operations_for_key(key);
self.val_index.remove(key)
}

/// Gets the given key's corresponding entry in the map for in-place manipulation.
pub fn entry(&self, key: Value) -> dashmap::Entry<'_, Value, IndexSet<Value>> {
self.flush_pending_operations_for_key(&key);
self.val_index.entry(key)
}

/// Returns a reference to the value corresponding to the key.
pub fn get(&mut self, key: &Value) -> Option<Ref<'_, Value, IndexSet<Value>>> {
self.flush_pending_operations_for_key(key);
self.val_index.get(key)
}

/// Returns a mutable reference to the value corresponding to the key.
pub fn get_mut(&mut self, key: &Value) -> Option<RefMut<'_, Value, IndexSet<Value>>> {
self.flush_pending_operations_for_key(key);
self.val_index.get_mut(key)
}

/// Returns `true` if the map contains a value for the specified key.
pub fn contains_key(&self, key: &Value) -> bool {
self.val_index.contains_key(key)
}

/// Lazily inserts a value for all keys in the given index set.
/// The actual insertion will be performed when the map is next accessed.
pub fn insert_for_all_keys(&self, keys: IndexSet<Value>, value: Value) {
if keys.len() < LAZY_BOUND {
for key in keys {
self.val_index.entry(key).or_default().insert(value);
}
} else {
self.pending_operations
.lock()
.unwrap()
.push((keys, InsertOrRemove::Insert(value)));
}
}

/// Lazily removes a value for all keys in the given index set.
pub fn remove_for_all_keys(&self, keys: IndexSet<Value>, value: Value) {
if keys.len() < LAZY_BOUND {

@yihozhang yihozhang Oct 23, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Need to flush all the updates before eagerly removing?

update: I think you also need to do flush_pending_operations_for_key for eager insertion, not just eager removal

for key in keys {
if let Some(mut index) = self.val_index.get_mut(&key) {
index.swap_remove(&value);
}
}
} else {
self.pending_operations
.lock()
.unwrap()
.push((keys, InsertOrRemove::Remove(value)));
}
}

/// Flushes all pending lazy insertions to the underlying map.
fn flush_pending_operations_for_key(&self, key: &Value) {
let mut pending_ops = self.pending_operations.lock().unwrap();
if !pending_ops.is_empty() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this if statement is redundant?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Currently, pending_operations never shrinks, even when the actual val_index is small.

We can maybe do a pending_ops.retain(|(keys, op)| !keys.is_empty()) when we find there are too many empty sets during the enumeration

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Plausible, added

for (keys, op) in pending_ops.iter_mut() {
if keys.contains(key) {
match op {
InsertOrRemove::Insert(v) => {
self.val_index.entry(*key).or_default().insert(*v);
}
InsertOrRemove::Remove(v) => {
if let Some(mut index) = self.val_index.get_mut(key) {
index.swap_remove(v);
}
}
}
}
keys.swap_remove(key);
}
}
}
}

impl Default for LazyMapOfIndexSet {
fn default() -> Self {
Self::new()
}
}

impl<C: ContainerValue> DynamicContainerEnv for ContainerEnv<C> {
Expand Down Expand Up @@ -242,7 +385,7 @@ impl<C: ContainerValue> ContainerEnv<C> {
counter,
to_id: DashMap::default(),
to_container: DashMap::default(),
val_index: DashMap::default(),
val_index: Default::default(),
}
}

Expand Down Expand Up @@ -274,9 +417,8 @@ impl<C: ContainerValue> ContainerEnv<C> {
dashmap::Entry::Vacant(vac) => {
// Common case: insert the mapping in to_id and update the index.
vac.insert(value);
for val in container.iter() {
self.val_index.entry(val).or_default().insert(value);
}
self.val_index
.insert_for_all_keys(container.iter().collect(), value);
value
}
dashmap::Entry::Occupied(occ) => {
Expand All @@ -302,18 +444,16 @@ impl<C: ContainerValue> ContainerEnv<C> {
self.to_container.remove(&old_val);
self.to_container.insert(result, (hc as usize, target_map));
*occ.get_mut() = result;
for val in occ.key().iter() {
let mut index = self.val_index.entry(val).or_default();
index.swap_remove(&old_val);
index.insert(result);
}
self.val_index
.remove_for_all_keys(occ.key().iter().collect(), old_val);
self.val_index
.insert_for_all_keys(occ.key().iter().collect(), result);
}
}
dashmap::Entry::Vacant(vacant_entry) => {
self.to_container.insert(value, (hc as usize, target_map));
for val in vacant_entry.key().iter() {
self.val_index.entry(val).or_default().insert(value);
}
self.val_index
.insert_for_all_keys(vacant_entry.key().iter().collect(), value);
vacant_entry.insert(value);
}
}
Expand Down Expand Up @@ -500,18 +640,17 @@ impl<C: ContainerValue> ContainerEnv<C> {
self.to_container.remove(&old_val);
self.to_container.insert(result, (hc as usize, target_map));
*val_slot.get_mut() = result;
for val in container.iter() {
let mut index = self.val_index.entry(val).or_default();
index.swap_remove(&old_val);
index.insert(result);
}
self.val_index

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

swap_remove is always used together with an insert, so maybe we can have an update_for_all_keys that takes both the old value and the new value. This way you don't need to materialize the container twice.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As another optimization, currently we always materialize the container (container.iter().collect()), but when the container only has a fewer entries than (LAZY_BOUND), the materialized container is immediately discarded. Can we make insert/remove_for_all_keys take an iterator instead of an IndexSet?

@MilkBlock MilkBlock Oct 23, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

2 ways to implement this.

pub trait ContainerValue: Hash + Eq + Clone + Send + Sync + 'static {
    /// Rebuild an additional container in place according the the given [`Rebuilder`].
    ///
    /// If this method returns `false` then the container must not have been modified (i.e. it must
    /// hash to the same value, and compare equal to a copy of itself before the call).
    fn rebuild_contents(&mut self, rebuilder: &dyn Rebuilder) -> bool;

    /// Iterate over the contents of the container.
    ///
    /// Note that containers can be more structured than just a sequence of values. This iterator
    /// is used to populate an index that in turn is used to speed up rebuilds. If a value in the
    /// container is eligible for a rebuild and it is not mentioned by this iterator, the outer
    /// [`Containers`] registry may skip rebuilding this container.
    fn iter(&self) -> impl Iterator<Item = Value> + '_;
}
  1. trait revision: add fn len()to this trait
  2. make iterator sized_iterator

Maybe we could consider it in next PR this may require many changes.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we just change the interface of iter to return an ExtractSizeIterator?

.remove_for_all_keys(container.iter().collect(), old_val);
self.val_index
.insert_for_all_keys(container.iter().collect(), result);
}
}
Err(slot) => {
self.to_container.insert(val, (hc as usize, target_map));
for v in container.iter() {
self.val_index.entry(v).or_default().insert(val);
}
self.val_index
.insert_for_all_keys(container.iter().collect(), val);

// SAFETY: We just got this slot from `find_or_find_insert_slot`
// and we have not mutated the map at all since then.
unsafe {
Expand Down