Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 32 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/fennecs/expressions/MatchBits.cs">
<violation number="1" location="src/fennecs/expressions/MatchBits.cs:164">
P2: Strict batch removals can reject valid `Remove<T>(Match.Any)` operations when the query uses `Has<TBase>(Match.Family)`. The `SecondaryKind.Any` branch checks only exact `FamilyIdPresent(typeId)` instead of inheritance coverage, so derived types are missed.</violation>
</file>
<file name="src/fennecs.tests/FamilyMatchTests.cs">
<violation number="1" location="src/fennecs.tests/FamilyMatchTests.cs:237">
P2: The new Family guard does not cover Job. This test asserts that `stream.Job((ref Animal _) => { })` on a Match.Family stream throws InvalidOperationException, but unlike For/Raw/Blit, the Job methods (Stream<C0>.Job / Job<U> in both Stream.tt and Stream.generated.cs) never call AssertNotFamily() — they only call AssertNoWildcards. Because a Family stream's archetypes contain derived storages, CrossJoin<C0> matches nothing and Job silently runs zero work, so this assertion fails and the suite goes red. Add AssertNotFamily() to every Job overload (in the .tt template, then regenerate) so Job behaves like the other writable surfaces, keeping the writable-ref promise consistent.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
There was a problem hiding this comment.
3 issues found across 20 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/fennecs.tests/MatchBitsTests.cs">
<violation number="1" location="src/fennecs.tests/MatchBitsTests.cs:5">
P3: The Animal/Fox/Fennec/Rock inheritance fixture added here duplicates the identical private nested-class hierarchy already defined in FamilyMatchTests.cs. Consider extracting these types into a shared test fixture so the inheritance oracle stays in one place when the hierarchy changes.</violation>
</file>
<file name="src/fennecs/expressions/MatchBits.cs">
<violation number="1" location="src/fennecs/expressions/MatchBits.cs:334">
P2: Family checks now re-hash the base type for every archetype comparison, which adds avoidable work in the query hot path. Precomputing the Family bloom pattern at clause compile time and reusing it in MatchesFamily would keep IsSupersetOf/Overlaps cheaper.</violation>
</file>
<file name="src/fennecs/Aspect.cs">
<violation number="1" location="src/fennecs/Aspect.cs:324">
P1: Query compilation can return a cached query built for a different mask when two masks share the same hash, producing incorrect matches. Using a collision-safe cache key (or validating full mask equality inside each hash bucket) avoids silent wrong-query reuse.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| // (cached queries were already validated when first compiled) | ||
| foreach (var type in mask.HasTypes.Concat(mask.NotTypes).Concat(mask.AnyTypes)) | ||
| // Return cached query if available. | ||
| if (_queryCache.TryGetValue(copy.GetHashCode(), out var query)) |
There was a problem hiding this comment.
P1: Query compilation can return a cached query built for a different mask when two masks share the same hash, producing incorrect matches. Using a collision-safe cache key (or validating full mask equality inside each hash bucket) avoids silent wrong-query reuse.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/fennecs/Aspect.cs, line 324:
<comment>Query compilation can return a cached query built for a different mask when two masks share the same hash, producing incorrect matches. Using a collision-safe cache key (or validating full mask equality inside each hash bucket) avoids silent wrong-query reuse.</comment>
<file context>
@@ -318,37 +318,46 @@ internal void DisposeArchetype(Archetype archetype)
- // (cached queries were already validated when first compiled)
- foreach (var type in mask.HasTypes.Concat(mask.NotTypes).Concat(mask.AnyTypes))
+ // Return cached query if available.
+ if (_queryCache.TryGetValue(copy.GetHashCode(), out var query))
{
- if (type.TypeId == LanguageType.EntityId) continue;
</file context>
| // Family test: the base type itself as a plain Component, or bloom + precise derived-type confirm. | ||
| private bool MatchesFamily(TypeID baseId) => | ||
| Plain.Get(baseId) | ||
| || (Family.MayContain(KeyBloom.Of(FamilyRaw(LanguageType.Resolve(baseId)))) && ConfirmFamily(baseId)); |
There was a problem hiding this comment.
P2: Family checks now re-hash the base type for every archetype comparison, which adds avoidable work in the query hot path. Precomputing the Family bloom pattern at clause compile time and reusing it in MatchesFamily would keep IsSupersetOf/Overlaps cheaper.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/fennecs/expressions/MatchBits.cs, line 334:
<comment>Family checks now re-hash the base type for every archetype comparison, which adds avoidable work in the query hot path. Precomputing the Family bloom pattern at clause compile time and reusing it in MatchesFamily would keep IsSupersetOf/Overlaps cheaper.</comment>
<file context>
@@ -326,17 +321,17 @@ private bool ConfirmFamily(TypeID baseId)
- Plain.Get(baseId) || (Family.MayContain(pattern) && ConfirmFamily(baseId));
+ private bool MatchesFamily(TypeID baseId) =>
+ Plain.Get(baseId)
+ || (Family.MayContain(KeyBloom.Of(FamilyRaw(LanguageType.Resolve(baseId)))) && ConfirmFamily(baseId));
</file context>
|
|
||
| public class MatchBitsTests | ||
| { | ||
| private class Animal; |
There was a problem hiding this comment.
P3: The Animal/Fox/Fennec/Rock inheritance fixture added here duplicates the identical private nested-class hierarchy already defined in FamilyMatchTests.cs. Consider extracting these types into a shared test fixture so the inheritance oracle stays in one place when the hierarchy changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/fennecs.tests/MatchBitsTests.cs, line 5:
<comment>The Animal/Fox/Fennec/Rock inheritance fixture added here duplicates the identical private nested-class hierarchy already defined in FamilyMatchTests.cs. Consider extracting these types into a shared test fixture so the inheritance oracle stays in one place when the hierarchy changes.</comment>
<file context>
@@ -2,6 +2,14 @@ namespace fennecs.tests;
public class MatchBitsTests
{
+ private class Animal;
+
+ private class Fox : Animal;
</file context>
There was a problem hiding this comment.
5 issues found across 37 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/fennecs/generators/Stream.generated.cs">
<violation number="1" location="src/fennecs/generators/Stream.generated.cs:1123">
P3: In `EnumerateRead`, every element and every slot calls `join.Get<C0>(0, i)`, which internally re-invokes `Storage.GetAs<C0>` -> `ReadAs<C0>` and rebuilds a covariant-cast `ReadOnlySpan` (`new((C0[])(object)_data, 0, Count)`) plus an array cast on each call. The sibling `ForRead` path in the same PR hoists this once per permutation via `join.Span<C0>(0)`. Hoist the spans once per permutation here too so enumeration does the cast/span construction O(arity) instead of O(Count * arity).</violation>
</file>
<file name="src/fennecs/LanguageType.cs">
<violation number="1" location="src/fennecs/LanguageType.cs:174">
P2: When a new type is registered concurrently, `Ids.Add` publishes its ID before this call, and the unlocked `Identify` fast path can return it while `AncestorTable[Id]` is still null. A concurrent archetype can then snapshot no family bloom for that component, so later `Match.Family` queries miss it; publish the ID only after all metadata is initialized.</violation>
</file>
<file name="src/fennecs/generators/Stream.tt">
<violation number="1" location="src/fennecs/generators/Stream.tt:131">
P2: When a non-Family stream calls `Blit(value, Match.Family)`, this guard does not reject the Family destination and `Fill` silently leaves derived component storages unchanged. Reject `Match.Family` in the `Blit` match argument, since Family component access is read-only.</violation>
</file>
<file name="src/fennecs.benchmarks/ECS/ArchetypeMatchingBenchmarks.cs">
<violation number="1" location="src/fennecs.benchmarks/ECS/ArchetypeMatchingBenchmarks.cs:82">
P3: `Setup()` leaks the three `Mask` instances because `Mask` implements `IDisposable` and must return pooled resources through `Dispose()`. Dispose each mask after creating its `MaskBits`.</violation>
</file>
<file name="src/fennecs/expressions/TypeBits.cs">
<violation number="1" location="src/fennecs/expressions/TypeBits.cs:45">
P2: `LoadOrZero` returns all-zero for any 256-bit block that is not *entirely* in bounds, so a trailing partial block of `a`/`b`/`c` is silently discarded. `ContainsAll2`, `ContainsAll3`, `Intersects2`, and `Intersects3` run their SIMD loops to `required`/`probe` length and never clamp to the smallest operand's block boundary, so with any operand that is not block-aligned (length not a multiple of `WordsPerBlock`) those methods silently return wrong answers when AVX is enabled, while the scalar fallback (AVX disabled) returns correct answers — divergent behavior that is hard to diagnose. The code is only correct today because every `TypeBits` is built via `AllocateFor` (always a multiple of 4 words) or `Empty` (`[]`); that invariant is documented nowhere on the struct and is not enforced in the constructor. Note `ContainsAll`/`Intersects` themselves are safe because they clamp to `shared`.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| } | ||
|
|
||
| StoreFlags(Id, ComputeFlags<T>()); | ||
| StoreAncestors(Id, typeof(T)); |
There was a problem hiding this comment.
P2: When a new type is registered concurrently, Ids.Add publishes its ID before this call, and the unlocked Identify fast path can return it while AncestorTable[Id] is still null. A concurrent archetype can then snapshot no family bloom for that component, so later Match.Family queries miss it; publish the ID only after all metadata is initialized.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/fennecs/LanguageType.cs, line 174:
<comment>When a new type is registered concurrently, `Ids.Add` publishes its ID before this call, and the unlocked `Identify` fast path can return it while `AncestorTable[Id]` is still null. A concurrent archetype can then snapshot no family bloom for that component, so later `Match.Family` queries miss it; publish the ID only after all metadata is initialized.</comment>
<file context>
@@ -132,6 +171,7 @@ static LanguageType()
}
StoreFlags(Id, ComputeFlags<T>());
+ StoreAncestors(Id, typeof(T));
}
}
</file context>
| new(this, [.. components], [], <#= string.Join(", ", Enumerable.Repeat("null", n)) #>); | ||
| public FilteredStream<<#= types #>> Has(params Comp[] components) | ||
| { | ||
| AssertNotFamily(); |
There was a problem hiding this comment.
P2: When a non-Family stream calls Blit(value, Match.Family), this guard does not reject the Family destination and Fill silently leaves derived component storages unchanged. Reject Match.Family in the Blit match argument, since Family component access is read-only.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/fennecs/generators/Stream.tt, line 131:
<comment>When a non-Family stream calls `Blit(value, Match.Family)`, this guard does not reject the Family destination and `Fill` silently leaves derived component storages unchanged. Reject `Match.Family` in the `Blit` match argument, since Family component access is read-only.</comment>
<file context>
@@ -117,30 +126,40 @@ namespace fennecs
- new(this, [.. components], [], <#= string.Join(", ", Enumerable.Repeat("null", n)) #>);
+ public FilteredStream<<#= types #>> Has(params Comp[] components)
+ {
+ AssertNotFamily();
+ return new(this, [.. components], [], <#= string.Join(", ", Enumerable.Repeat("null", n)) #>);
+ }
</file context>
| internal static void Set(ulong[] words, TypeID typeId) => words[typeId >> 6] |= 1ul << typeId; | ||
|
|
||
|
|
||
| private static Vector256<ulong> LoadOrZero(ulong[] words, int offset) => |
There was a problem hiding this comment.
P2: LoadOrZero returns all-zero for any 256-bit block that is not entirely in bounds, so a trailing partial block of a/b/c is silently discarded. ContainsAll2, ContainsAll3, Intersects2, and Intersects3 run their SIMD loops to required/probe length and never clamp to the smallest operand's block boundary, so with any operand that is not block-aligned (length not a multiple of WordsPerBlock) those methods silently return wrong answers when AVX is enabled, while the scalar fallback (AVX disabled) returns correct answers — divergent behavior that is hard to diagnose. The code is only correct today because every TypeBits is built via AllocateFor (always a multiple of 4 words) or Empty ([]); that invariant is documented nowhere on the struct and is not enforced in the constructor. Note ContainsAll/Intersects themselves are safe because they clamp to shared.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/fennecs/expressions/TypeBits.cs, line 45:
<comment>`LoadOrZero` returns all-zero for any 256-bit block that is not *entirely* in bounds, so a trailing partial block of `a`/`b`/`c` is silently discarded. `ContainsAll2`, `ContainsAll3`, `Intersects2`, and `Intersects3` run their SIMD loops to `required`/`probe` length and never clamp to the smallest operand's block boundary, so with any operand that is not block-aligned (length not a multiple of `WordsPerBlock`) those methods silently return wrong answers when AVX is enabled, while the scalar fallback (AVX disabled) returns correct answers — divergent behavior that is hard to diagnose. The code is only correct today because every `TypeBits` is built via `AllocateFor` (always a multiple of 4 words) or `Empty` (`[]`); that invariant is documented nowhere on the struct and is not enforced in the constructor. Note `ContainsAll`/`Intersects` themselves are safe because they clamp to `shared`.</comment>
<file context>
@@ -0,0 +1,305 @@
+ internal static void Set(ulong[] words, TypeID typeId) => words[typeId >> 6] |= 1ul << typeId;
+
+
+ private static Vector256<ulong> LoadOrZero(ulong[] words, int offset) =>
+ offset + WordsPerBlock <= words.Length
+ ? Vector256.LoadUnsafe(ref MemoryMarshal.GetArrayDataReference(words), (nuint)offset)
</file context>
| { | ||
| for (var i=0; i<table.Count; i++) | ||
| { | ||
| yield return (table[i], join.Get<C0>(0, i), join.Get<C1>(1, i)); |
There was a problem hiding this comment.
P3: In EnumerateRead, every element and every slot calls join.Get<C0>(0, i), which internally re-invokes Storage.GetAs<C0> -> ReadAs<C0> and rebuilds a covariant-cast ReadOnlySpan (new((C0[])(object)_data, 0, Count)) plus an array cast on each call. The sibling ForRead path in the same PR hoists this once per permutation via join.Span<C0>(0). Hoist the spans once per permutation here too so enumeration does the cast/span construction O(arity) instead of O(Count * arity).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/fennecs/generators/Stream.generated.cs, line 1123:
<comment>In `EnumerateRead`, every element and every slot calls `join.Get<C0>(0, i)`, which internally re-invokes `Storage.GetAs<C0>` -> `ReadAs<C0>` and rebuilds a covariant-cast `ReadOnlySpan` (`new((C0[])(object)_data, 0, Count)`) plus an array cast on each call. The sibling `ForRead` path in the same PR hoists this once per permutation via `join.Span<C0>(0)`. Hoist the spans once per permutation here too so enumeration does the cast/span construction O(arity) instead of O(Count * arity).</comment>
<file context>
@@ -852,6 +1107,25 @@ public void Raw<U>(U uniform, MemoryUniformAction<U, C0, C1> action)
+ {
+ for (var i=0; i<table.Count; i++)
+ {
+ yield return (table[i], join.Get<C0>(0, i), join.Get<C1>(1, i));
+ if (table.Version != snapshot) throw new InvalidOperationException("Collection was modified during iteration.");
+ }
</file context>
| _expanded = _archetypes.Select(archetype => Expand(archetype.Signature)).ToArray(); | ||
|
|
||
| // Composite: plain Has + plain Not + Entity-wildcard Has. | ||
| var composite = new Mask() |
There was a problem hiding this comment.
P3: Setup() leaks the three Mask instances because Mask implements IDisposable and must return pooled resources through Dispose(). Dispose each mask after creating its MaskBits.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/fennecs.benchmarks/ECS/ArchetypeMatchingBenchmarks.cs, line 82:
<comment>`Setup()` leaks the three `Mask` instances because `Mask` implements `IDisposable` and must return pooled resources through `Dispose()`. Dispose each mask after creating its `MaskBits`.</comment>
<file context>
@@ -0,0 +1,197 @@
+ _expanded = _archetypes.Select(archetype => Expand(archetype.Signature)).ToArray();
+
+ // Composite: plain Has + plain Not + Entity-wildcard Has.
+ var composite = new Mask()
+ .Has(TypeExpression.Of<Position>(Match.Plain))
+ .Has(TypeExpression.Of<Grouping>(Match.Entity))
</file context>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
feat: new bitset / vector based matching engine for Signature and Queries
feat: Family matching for OOP style component inheritance
Summary by cubic
Replaced wildcard-expanded matching with a bitset + bloom engine for faster archetype/query matching, and added inheritance-aware matching via
Match.Familywith read-only stream support. Also fixed the NuGet package logo link.New Features
Match.Family: match plain components of a type or any derived type; usable in Has/Not/Any and filters; read-only access viaForRead/enumeration (writable runners,Raw/Blit, and filtered views throw).ReadAs/GetAs) and a read-join path to serve Family streams.Refactors
MaskBits/ClauseBits; Archetypes carryArchetypeBits; generators and query caching/locking updated accordingly.TypeBits,MatchBits, and Family matching.Written for commit a8289d2. Summary will update on new commits.