You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The short version is: queries can now return a composable set object, not just an iterator. That gives us a cleaner API surface for set operations and lets us keep query execution lazy.
In this PR, EntitySet is used primarily as the representation of query results, but the type itself is intentionally more general than queries. It is a reusable abstraction for representing and composing sets of entities more generally.
New public API
EntitySet
EntitySet is a set-expression wrapper over entity IDs. In this PR, query APIs return it, but it is not query-specific. It represents a lazy set expression made from:
"source" leaves (population/indexed/property-derived/etc.), an invisible internal type
set operations (union, intersection, difference):
let alive_people = context.query((Alive(true),));let seniors = context.query((AgeGroup::Senior,));let alive_seniors = alive_people.intersection(seniors);
You can:
call contains without materializing a full result vector (generally very efficient)
iterate it with into_iter()
compose sets as set algebra
ContextEntitiesExt::query
query returns an EntitySet directly:
let people = context.query((Age(42),Vaccinated(true)));
(There is a micro-optimization to construct an EntitySetIterator directly instead of creating an EntitySet first in the case of query_result_iterator, which squeezes out a couple more percentage points of performance improvement in the tightest loops.)
Internal machinery
Internally, query evaluation is now split into two concepts:
EntitySet
owns the set expression tree
applies simplifications/reordering during construction
provides lazy contains and lazy iteration entrypoint
EntitySetIterator
an existing type, almost completely rewritten in this PR
executes the expression tree lazily
has dedicated execution variants for source, intersection, union, difference
performance micro-optimizations: specialized IntersectionSources path for source-only intersections, direct construction path to avoid constructing EntitySet first...
These are built on:
SourceSet / SourceSetIterator
Private leaf / building-block types
Provide a uniform interface over several internal low-level data-source representations used by query execution
This structure lets us keep API-level behavior straightforward while still being explicit about hot paths internally.
Why this abstraction
Better API
Returning EntitySet from query makes set semantics first-class. It is easier to reason about and easier to compose than forcing everything through immediate iteration. It also gives us a shared abstraction we can reuse outside query execution, instead of inventing ad hoc set wrappers for each subsystem.
It also provides a basis for implementing queries having OR, NOT, and predicate conditionals ((Age > 60)).
Clearer separation of concerns
Query represents what the query is. EntitySet represents what the result is. EntitySetIterator represents how to execute it. That split makes future optimization work less tangled. Representation and computation logic is moved out of ContextEntitiesExt and Query impls.
Performance
Careful benchmarking and optimization work resulted in better performance over the existing implementation across the board almost across the board. Skip the CI benchmarks and look at the local results I posted.
Future optimization opportunities like compilation of membership checking (and, by extension, iteration) to a BDD now have a clear path forward if and when we are ready for them.
Internals are set up for more targeted optimization without changing user-facing query syntax.
Limitations
Operations on EntitySet consume self
Set operations consume self, and EntitySet doesn't provide an iter method, it only implements IntoIterator (consuming self). The reason is that SourceSets derived from indexes have type Ref<IndexSet<_>>, which is neither copy nor clone. Once we construct and return the EntitySet, we don't have access to the RefCell that gave us the Ref instance, and so we can't get another reference to the index set.
For the same reason, EntitySet and EntitySetIterator do not implement Clone.
The good news is, now EntitySets are reasonably cheap to create. If you want more than one copy of the same set, just call context.query multiple times.
This is a difficult limitation to overcome without either major rearchitecture or unsafe. If we need to lift this restriction, the best path forward is probably unsafe. (The problem boils down to this: when we need to mutate an index for a derived property, we also need to give the derived property an immutable reference to context in order for it to compute its value. So we need both mutable and immutable access simultaneously.)
EntitySet / EntitySetIterator holds an immutable reference to context, is tied to context's lifetime
The underlying SourceSets are immutable views into the internal data structures owned by context. The lifetime 'a of &'a context is a generic parameter of SourceSet<'a, E: Entity> and thus of EntitySet<'a, E: Entity> / EntitySetIterator<'a, E: Entity>. The compiler statically enforces Rust lifetime and aliasing rules. The context cannot be mutated while an EntitySet / EntitySetIterator exists.
Client code can compute the set of entity IDs (or use some other pattern) if they need access to the result set while mutating context. We provide the EntitySet::to_owned_vec method to conveniently compute a Vec<EntityId<E>>.
Issues and Questions
Q: What should we do with with_query_result?
Right now with_query_result gives client code direct access to an immutable reference to an IndexSet. This method made sense at the time we implemented it, but we will likely have different kinds of indexes in the future which will have different internal container types. What's more, with_query_result is necessarily eager in the unindexed case, realizing the full result set. We do emit a warning in the unindexed case, but it's awkward that a method intended to give an optimized fast path also exposes a new slow path.
The with_query_result method still has a use case: it gives client code a scope for the lifetime of the result set—a lifetime during which context cannot be mutated. If we want to keep it, it makes sense to me to expose the result set as an EntitySet. Membership checking, iteration, and other operations should be virtually identical between IndexSet and EntitySet both in terms of performance and convenience. As far as I can tell, the only advantage of IndexSet over EntitySet is that IndexSet has a len method. But we could give EntitySet a method like EntitySet::try_len() that returns an Option<usize> that is None if EntitySet doesn't trivially know its length (without needing to compute anything). Client code can always do entity_set.into_iter().count(), which is fast if EntitySet::try_len() would return Some, but which consumesentity_set.
The benchmark comparisons from CI are all over the place. Below is the data from my local run. I ran it a couple of times, and the results were remarkably consistent, even in cases of small percent-change. As usual, one should be skeptical of small differences, of course.
The example-births-deaths benchmark is the most interesting to me. It suggests we do not have good benchmark coverage—there's something happening relevant to performance that isn't being measured by any existing benchmark.
Run a comparison on your local machine and compare. The suite takes ~10 minutes to run on my dev machine. This is long enough to make thermal throttling a problem, so keep an eye on that when you run it.
New benchmarks after a little bit of optimization. The improvement to example-births-deaths is somewhat artificial. I implemented a new fused sample-count algorithm. Our script to generate the nice table doesn't work if you add new benchmarks, so we don't have the same format.
Smaller (faster) time bolded. Relative speedup = main ÷ dev.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
This PR introduces a new public set abstraction,
EntitySet, and wires it intoContextEntitiesExtthrough a new method:ContextEntitiesExt::query<E, Q>(&self, query: Q) -> EntitySet<E>The short version is: queries can now return a composable set object, not just an iterator. That gives us a cleaner API surface for set operations and lets us keep query execution lazy.
In this PR,
EntitySetis used primarily as the representation of query results, but the type itself is intentionally more general than queries. It is a reusable abstraction for representing and composing sets of entities more generally.New public API
EntitySetEntitySetis a set-expression wrapper over entity IDs. In this PR, query APIs return it, but it is not query-specific. It represents a lazy set expression made from:union,intersection,difference):You can:
containswithout materializing a full result vector (generally very efficient)into_iter()ContextEntitiesExt::queryqueryreturns anEntitySetdirectly:query_result_iteratorremains available:(There is a micro-optimization to construct an
EntitySetIteratordirectly instead of creating anEntitySetfirst in the case ofquery_result_iterator, which squeezes out a couple more percentage points of performance improvement in the tightest loops.)Internal machinery
Internally, query evaluation is now split into two concepts:
EntitySetcontainsand lazy iteration entrypointEntitySetIteratorIntersectionSourcespath for source-only intersections, direct construction path to avoid constructingEntitySetfirst...These are built on:
SourceSet/SourceSetIteratorThis structure lets us keep API-level behavior straightforward while still being explicit about hot paths internally.
Why this abstraction
Better API
Returning
EntitySetfromquerymakes set semantics first-class. It is easier to reason about and easier to compose than forcing everything through immediate iteration. It also gives us a shared abstraction we can reuse outside query execution, instead of inventing ad hoc set wrappers for each subsystem.It also provides a basis for implementing queries having OR, NOT, and predicate conditionals (
(Age > 60)).Clearer separation of concerns
Queryrepresents what the query is.EntitySetrepresents what the result is.EntitySetIteratorrepresents how to execute it. That split makes future optimization work less tangled. Representation and computation logic is moved out ofContextEntitiesExtandQueryimpls.Performance
Careful benchmarking and optimization work resulted in better performance over the existing implementation
across the boardalmost across the board. Skip the CI benchmarks and look at the local results I posted.Future optimization opportunities like compilation of membership checking (and, by extension, iteration) to a BDD now have a clear path forward if and when we are ready for them.
Internals are set up for more targeted optimization without changing user-facing query syntax.
Limitations
Operations on
EntitySetconsumeselfSet operations consume
self, andEntitySetdoesn't provide anitermethod, it only implementsIntoIterator(consuming self). The reason is thatSourceSets derived from indexes have typeRef<IndexSet<_>>, which is neither copy nor clone. Once we construct and return theEntitySet, we don't have access to theRefCellthat gave us theRefinstance, and so we can't get another reference to the index set.For the same reason,
EntitySetandEntitySetIteratordo not implementClone.The good news is, now
EntitySets are reasonably cheap to create. If you want more than one copy of the same set, just callcontext.querymultiple times.This is a difficult limitation to overcome without either major rearchitecture or
unsafe. If we need to lift this restriction, the best path forward is probablyunsafe. (The problem boils down to this: when we need to mutate an index for a derived property, we also need to give the derived property an immutable reference tocontextin order for it to compute its value. So we need both mutable and immutable access simultaneously.)EntitySet/EntitySetIteratorholds an immutable reference tocontext, is tied tocontext's lifetimeThe underlying
SourceSets are immutable views into the internal data structures owned bycontext. The lifetime'aof&'a contextis a generic parameter ofSourceSet<'a, E: Entity>and thus ofEntitySet<'a, E: Entity>/EntitySetIterator<'a, E: Entity>. The compiler statically enforces Rust lifetime and aliasing rules. Thecontextcannot be mutated while anEntitySet/EntitySetIteratorexists.Client code can compute the set of entity IDs (or use some other pattern) if they need access to the result set while mutating
context. We provide theEntitySet::to_owned_vecmethod to conveniently compute aVec<EntityId<E>>.Issues and Questions
Q: What should we do with
with_query_result?Right now
with_query_resultgives client code direct access to an immutable reference to anIndexSet. This method made sense at the time we implemented it, but we will likely have different kinds of indexes in the future which will have different internal container types. What's more,with_query_resultis necessarily eager in the unindexed case, realizing the full result set. We do emit a warning in the unindexed case, but it's awkward that a method intended to give an optimized fast path also exposes a new slow path.The
with_query_resultmethod still has a use case: it gives client code a scope for the lifetime of the result set—a lifetime during whichcontextcannot be mutated. If we want to keep it, it makes sense to me to expose the result set as anEntitySet. Membership checking, iteration, and other operations should be virtually identical betweenIndexSetandEntitySetboth in terms of performance and convenience. As far as I can tell, the only advantage ofIndexSetoverEntitySetis thatIndexSethas alenmethod. But we could giveEntitySeta method likeEntitySet::try_len()that returns anOption<usize>that isNoneifEntitySetdoesn't trivially know its length (without needing to compute anything). Client code can always doentity_set.into_iter().count(), which is fast ifEntitySet::try_len()would returnSome, but which consumesentity_set.