Skip to content

feat: Removed RefCell from indexes#799

Open
RobertJacobsonCDC wants to merge 1 commit intomainfrom
RobertJacobsonCDC_remove_refcell_for_index
Open

feat: Removed RefCell from indexes#799
RobertJacobsonCDC wants to merge 1 commit intomainfrom
RobertJacobsonCDC_remove_refcell_for_index

Conversation

@RobertJacobsonCDC
Copy link
Collaborator

This branch reworks entity-property indexing by removing interior mutability from index storage and switching from lazy (query-time) to eager (write-time) index maintenance.

The advantages

Performance

It turns out that lazy indexing wasn't the performance advantage we thought it was. In fact, this refactor shows a consistent but very small performance improvement by indexing eagerly. This alone is probably not good enough justification for the change. I am posting my local benchmark results separately.

Simpler code

Removing RefCell from PropertyIndex makes indexing explicit: reads take shared references, writes take mutable references, and the compiler enforces the distinction — no more runtime borrow guards or interior-mutable control flow. For example, there is less API noise from lazy-era plumbing: we can remove now-unused context passthrough parameters. Index maintenance is also consolidated: old/new value updates now happen in one place rather than being split across partial-event creation and emission. The result is code that's simpler to follow, review, and maintain.

EntitySet and EntitySetIterator can be Clone

In the current (lazy/RefCell) implementation of EntitySet and EntitySetIterator , the two types cannot implement Clone, because some source set types wrap a Ref holding a reference to an index set, which cannot be Clone. (See #786.) Removing indexes from RefCell allow these types to hold a &IndexSet instead, which is trivially Copy.

The Price: Unfortunate use of unsafe

The cost of removing RefCell is the introduction of two uses of unsafe:

  1. ContextEntitiesExt::add_entity
  2. ContextEntitiesExt::index_property

In both cases, unsafe is used to call property_store.index_unindexed_entities_for_all_properties(&*context_ptr). The problem boils down to the fact that we want to mutate the index in the property value store while also supplying an immutable reference to context so that we can compute derived properties with P::compute_derived(context, entity_id). Rust actually allows a version of this, "partial borrows," for the simple case of a multi-field struct:

struct Player {
    health: i32,
    name: String,
    scores: Vec<i32>,
}

fn main() {
    let mut p = Player {
      health: 100,
      name: "Alice".to_string(),
      scores: vec![10, 20]
  };

    let name = &p.name;          // immutable borrow of `name`
    let scores = &mut p.scores;  // mutable borrow of `scores` — OK!
    scores.push(30);
    println!("{}", name);        // still valid
}

This doesn't work across method boundaries: methods borrow the entire struct, even though in our case we are mutating a different part of context than what is being read.

Implementation

FullIndex and ValueCountIndex are removed from RefCell in PropertyIndex, so index mutation now requires &mut self and index access returns plain references instead of Ref guards. This eliminates runtime borrow checks and simplifies reference types through query iteration.

Index consistency is now maintained at write time rather than via lazy catch-up during queries. Entity creation writes initial values and immediately syncs all enabled indexes for that type. Property updates use a two-phase flow: snapshot previous values, write the new value, then update indexes (remove old / add new) during partial-event emission.

Cleanup:

  • stale comments/docs, including the set_property algorithm notes
  • dropped unused context: &Context parameters from index set/count lookup helpers and updated all call sites
  • maintenance of indexes upon property change is now done in a single clean step (remove old, insert new) instead of spread out across property_store.create_partial_property_change and partial_property_change.emit_in_context
  • updated description of indexing in the Ixa Book

@RobertJacobsonCDC RobertJacobsonCDC linked an issue Feb 27, 2026 that may be closed by this pull request
Copy link
Collaborator

@k88hudson-cfa k88hudson-cfa left a comment

Choose a reason for hiding this comment

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

This looks OK to me / passes under tree borrows, I'd prefer if we could find a way to get rid of the unsafe or isolate it in data structures that are stable and well-tested

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Evaluate eager vs. lazy indexing strategy

2 participants