Skip to content

Add incremental FilteredList dispatch and batch sync paths#8

Merged
VPDPersonal merged 11 commits into
mainfrom
fix/observable-incremental-and-review
May 22, 2026
Merged

Add incremental FilteredList dispatch and batch sync paths#8
VPDPersonal merged 11 commits into
mainfrom
fix/observable-incremental-and-review

Conversation

@VPDPersonal

@VPDPersonal VPDPersonal commented May 22, 2026

Copy link
Copy Markdown
Owner

Summary

  • FilteredList<T> now dispatches source changes incrementally instead of full-rebuilding on every event. Handles Add/Remove (single + batch), Replace and Move (single), with and without filter/comparer. Reset and batch Replace/Move still fall back to a full Update().
  • ObservableList<T> gains RemoveRange(startIndex, count) plus an OnRemovedRange hook. Synchronizers (Dict / HashSet / List) propagate batch Add/Remove instead of throwing NotImplementedException; Queue / Stack / Dict use NotSupportedException for actions the underlying collection cannot emit.
  • CollectionChangedEvent<T> is now re-entrancy-safe: the subscriber list is copy-on-written when add/remove happens during an in-flight Invoke, and a lazy HashSet<Handler> tracks unsubscribes-during-invoke for O(1) skip of removed handlers.
  • 9 fixes from self-review baked into the same commit: infinite recursion in ObservableStackSync.OnPoppedRange / ObservableQueueSync.OnDequeuedRange overrides, wrong value in ObservableDictionarySync.OnReplaced (passed newItem.Value to OnRemoved), FilteredList.ApplyMoveSingle tie-break drift under comparer, non-injective converter guard in ObservableHashSetSync, RemoveRange negative-count validation, FilteredList 3-arg ctor subscribe-before-init window.
  • New tests: AutoUpdate_* suite for FilteredList (including a randomised parity-vs-full-rebuild test across all four filter × comparer combinations × 200 ops), batch RemoveRange tests, copy-on-write event tests, InsertRange/RemoveRange coverage for CreateSyncList, FilteredList auto-update and SplitByEvents. Performance benchmark scaffold lives under Tests/Observable/Performance/, gated by ASPID_COLLECTIONS_PERFORMANCE_TESTING.
  • Aspid script icon added at Editor/Resources/Icons/aspid_icon_medium_green_1022x1011.png; runtime and test .cs.meta files repointed to it.
  • Release workflow now strips CLAUDE.md / CLAUDE.md.meta from the published UPM tree.
  • CHANGELOG.md updated under [Unreleased] to cover all of the above; CLAUDE.md synced with the current asmdefs, directory layout and package version.

Notes for review

  • The diff includes ~30 .meta files with only an icon GUID change — Unity script-icon reassignment to the new Aspid icon, no logical impact.
  • ApplyMoveSingle now always reseats the moved entry — previously it short-circuited for the comparer case, which silently desynced source-index tie-break (visible only for reference types whose comparer returns 0 for distinct instances). The randomised parity test passes because it asserts on int values; consumers with custom comparers + reference types are the ones who would have hit the divergence.

Test plan

  • Run Aspid.Collections.Observable.Tests in Unity Test Runner (EditMode) — full suite.
  • Spot-check AutoUpdate_ParityWithFullRebuild_RandomSequence.
  • Optional: enable ASPID_COLLECTIONS_PERFORMANCE_TESTING and run FilteredListPerformanceTests.

- FilteredList: dispatch source changes incrementally (Add/Remove single + batch, Replace/Move single) for both filter-only and filter+comparer modes; falls back to full Update() for Reset and batch Replace/Move.
- ObservableList: new RemoveRange(start, count) with OnRemovedRange hook; synchronizers (List/Dict/HashSet) propagate batch Add/Remove instead of throwing NotImplementedException; Queue/Stack/Dict use NotSupportedException for actions the underlying collection cannot emit.
- CollectionChangedEvent: copy-on-write subscriber list during Invoke so handlers may subscribe/unsubscribe without disturbing in-flight iteration; lazy HashSet tracks unsubscribes during invoke for O(1) skip.
- Fix bugs surfaced by self-review: infinite recursion in ObservableStackSync.OnPoppedRange / ObservableQueueSync.OnDequeuedRange overrides (renamed private helpers), wrong value in ObservableDictionarySync.OnReplaced, FilteredList move tie-break drift under comparer, non-injective converter handling in HashSetSync, RemoveRange negative-count validation, FilteredList 3-arg ctor subscribe-before-init window.
- Tests: AutoUpdate suite for FilteredList, RemoveRange tests for ObservableList, copy-on-write event tests; performance benchmark scaffold under Tests/Observable/Performance.
@VPDPersonal VPDPersonal added type: feature New feature or capability status: work-in-progress Draft / not ready for review needs-changelog README/CHANGELOG update required before release area: runtime Runtime / player code labels May 22, 2026
- Document Added / Changed / Fixed for the [Unreleased] section: FilteredList incremental dispatch, ObservableList.RemoveRange, batch synchronizer paths, copy-on-write CollectionChangedEvent, and the 9 self-review fixes.
@VPDPersonal VPDPersonal removed the needs-changelog README/CHANGELOG update required before release label May 22, 2026
- CreateSyncList: covers InsertRange propagation at the correct index
- FilteredList auto-update: InsertRange with and without a comparer
- SplitByEvents: added/removed callbacks fire on InsertRange and RemoveRange
- Bump documented package version to 1.0.2 and reconcile Unity target with package.json (2021.3 manifest minimum vs 2022.3+ dev target).
- Expand directory tree with Extensions/ subfolders, ObservableCollectionEvents, and Tests/Observable/Performance/.
- Add Aspid.Collections.Observable.PerformanceTests.asmdef to the assemblies table and a Gotcha covering its ASPID_COLLECTIONS_PERFORMANCE_TESTING define gate.
- Point readers at CHANGELOG.md for in-flight work in the Unreleased block.
- Add Editor/Resources/Icons/aspid_icon_medium_green_1022x1011.png with folder metas
- Repoint MonoImporter.icon GUID in every runtime and test .cs.meta to the new icon
- Refresh PerformanceTests asmdef formatting (Unity auto-save)
@VPDPersonal VPDPersonal marked this pull request as ready for review May 22, 2026 21:14
@VPDPersonal VPDPersonal added status: needs-review Ready for review and removed status: work-in-progress Draft / not ready for review labels May 22, 2026
@VPDPersonal

Copy link
Copy Markdown
Owner Author

@claude once

@claude

claude Bot commented May 22, 2026

Copy link
Copy Markdown

Claude finished @VPDPersonal's task in 5m 8s —— View job


Code Review

  • Read key source files
  • Review CollectionChangedEvent.cs (re-entrancy safety)
  • Review FilteredList.cs (incremental dispatch)
  • Review ObservableList.cs (RemoveRange)
  • Review synchronizer files
  • Review test files
  • Post findings

Overall this is a well-structured PR with good test coverage. The incremental dispatch logic is correct, the COW event system is sound, and the self-review fixes are all appropriate. A few specific issues worth addressing before merge:


1. Spurious list fork in CollectionChangedEvent.remove — efficiency bug

Runtime/Observable/CollectionChangedEvent.cs, lines 45–57

if (_invokeDepth > 0)
{
    var forked = new List<NotifyCollectionChangedEventHandler<T>>(_handlers);

    if (forked.Remove(v))
        (_removedDuringInvoke ??= new HashSet<...>()).Add(v);
    _handlers = forked;   // ← always executes, even when Remove returned false
}

_handlers = forked is unconditional. If v is not in _handlers, we still allocate a copy and swap in the identical list. This is harmless for correctness (the in-flight foreach holds its own reference), but every unsuccessful remove during an invoke allocates unnecessarily. Fix: only reassign _handlers when the handler was actually found:

if (_invokeDepth > 0)
{
    var forked = new List<NotifyCollectionChangedEventHandler<T>>(_handlers);
    if (forked.Remove(v))
    {
        (_removedDuringInvoke ??= new HashSet<...>()).Add(v);
        _handlers = forked;   // only fork when something changed
    }
}

Fix this →


2. RemoveRangestartIndex not validated, zero-count fires event

Runtime/Observable/ObservableList.cs, lines 169–183

public void RemoveRange(int startIndex, int count)
{
    if (count < 0) throw new ArgumentOutOfRangeException(nameof(count));
    lock (SyncRoot)
    {
        var items = new T[count];
        for (var i = 0; i < count; i++)
            items[i] = _list[startIndex + i];   // ← throws IndexOutOfRangeException if startIndex < 0
        ...
        Invoke(NotifyCollectionChangedEventArgs<T>.Remove(items, startIndex));   // fires even when count==0
    }
}

Two concerns:

a) A negative startIndex propagates as IndexOutOfRangeException from _list[startIndex + i], not the more informative ArgumentOutOfRangeException(nameof(startIndex)). Add an explicit guard alongside the count < 0 check.

b) count == 0 fires a batch-Remove event with an empty OldItems (tested in RemoveRange_ZeroCount_RaisesEmptyBatchRemoveEvent). This is intentional per the test, but every downstream subscriber — FilteredList, ObservableListSync, SplitByEvents — will execute its update path for zero net change. Consider short-circuiting at count == 0 instead.

Fix this →


3. ObservableStackSync / ObservableQueueSync batch Remove passes wrong items to lifecycle callbacks

Runtime/Observable/Synchronizer/ObservableStackSync.cs, line 72

case NotifyCollectionChangedAction.Remove:
{
    if (args.IsSingleItem) Pop();
    else PopRange(new TTo[args.OldItems!.Count]);   // ← blank array
}

PopRange(dest) is called with a freshly-allocated zero-value array. If ObservableStack.PopRange fills dest with the items it actually pops (acting as an output buffer), then OnPoppedRange(dest) / HandlePopped(dest) correctly disposes or callbacks those items. If PopRange does NOT write into dest before firing the hook, however, the _isDisposable path disposes default/null values instead of the real items. Same pattern in ObservableQueueSync line 72. Without seeing ObservableStack.PopRange's contract clearly documented, this deserves a comment or assertion to make the invariant explicit.


4. FilteredList chain loses incremental dispatch

Runtime/Observable/Filtered/FilteredList.cs, lines 67–71

case IReadOnlyFilteredList<T> filteredList:
    filteredList.CollectionChanged += Update;   // full rebuild
    break;
case IReadOnlyObservableList<T> observableList:
    observableList.CollectionChanged += OnCollectionChanged;   // incremental
    break;

When a FilteredList wraps another FilteredList, the downstream subscribes to the coarse Action? CollectionChanged event and falls back to a full Update() on every upstream change. This is correct, but it means multi-level chains (common when filtering then sorting) lose the incremental path entirely — the O(n log n) rebuild runs on every upstream mutation. This is worth a comment or a doc note so future contributors know why the chaining perf drops off. No code change required, just visibility.


5. FindViewPosition is O(n) when a comparer is set

Runtime/Observable/Filtered/FilteredList.cs, lines 379–385

// comparer path: linear scan
for (var index = 0; index < _indexes.Count; index++)
{
    if (_indexes[index] == sourceIndex)
        return index;
}

Acknowledged in the performance test comment ("a future switch to an auxiliary reverse index would show up as a large win"). ApplyRemoveBatch calls this in a loop, making batch removes O(n²) when a comparer is active. The performance test only covers single-item removes. It is worth adding a batch-remove comparer benchmark to make the regression surface visible — it won't block this PR, but it closes the measurement gap.


6. Performance benchmark — source not disposed in SetUp/CleanUp teardown

Tests/Observable/Performance/FilteredListPerformanceTests.cs, lines 85–99

Measure.Method(() => source!.RemoveAt(size / 2))
    .SetUp(() =>
    {
        source = new ObservableList<int>();
        ...
        filtered = new FilteredList<int>(source);
    })
    .CleanUp(() => filtered.Dispose())   // ← source leaked

filtered is disposed but source is not. ObservableList.Dispose() clears the list and removes subscriptions. Leaking source in a tight benchmark loop may distort GC pause measurements across samples. Add source.Dispose() to .CleanUp.

Fix this →


Minor observations (no change required)

  • ThrowValueNullReferenceException (CollectionChangedEvent.cs line 95): the method body throws, making the call-site throw unreachable. The pattern works, but [System.Diagnostics.CodeAnalysis.DoesNotReturn] + void return type would be more idiomatic and avoid the misleading signature.
  • Single-subscriber fast path (lines 67–73): the comment is accurate — the fast path intentionally skips _invokeDepth tracking. If a single-subscriber handler fires a re-entrant Invoke that grows the subscriber list, the re-entrant call does NOT use COW for that inner invoke. This is safe by design (the outer invoke has already finished iterating) but is non-obvious; the comment covers it.
  • ObservableHashSetSync.AddOne: if _sync.Add(fromItem, item) were to throw (e.g., fromItem already present due to a duplicate source event), item would be in the hashset but not in _sync, leaving them inconsistent. In practice the source shouldn't emit duplicate Adds, but a combined Add+Rollback guard would make it bulletproof.

The incremental dispatch correctness (especially ApplyMoveSingle reseating and the reversed-loop batch remove) looks solid. The randomised parity test is exactly the right way to cover that surface. Items 1 and 2a are the ones I'd fix before merge; the rest are refinements.

claude and others added 4 commits May 22, 2026 21:27
When the remove accessor for CollectionChanged forked _handlers during an
active invoke but the handler was not present, the fork was still assigned
back to _handlers. This silently discarded any concurrent additions made
to the original list (e.g. via a fresh fork from add) and left an
unchanged copy in its place. Move the assignment inside the Remove check
so we only swap when the handler was actually removed.
The CleanUp lambda in Remove_Middle_ComparerIsSlowerThanNoComparer only
disposed the FilteredList wrapper; the underlying ObservableList stayed
alive across measurement iterations. Dispose the source too so each
iteration starts from a clean state.

https://claude.ai/code/session_0133w4Rt2GPd43rjC6UG5Dfg
Throw ArgumentOutOfRangeException before taking the lock, matching the
existing count guard, so callers get a deterministic exception instead
of an IndexOutOfRangeException from the per-item read at items[i] =
_list[startIndex + i].
Validate negative startIndex in ObservableList.RemoveRange
@VPDPersonal VPDPersonal merged commit 0c8a484 into main May 22, 2026
1 check passed
@VPDPersonal VPDPersonal deleted the fix/observable-incremental-and-review branch May 22, 2026 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: runtime Runtime / player code status: needs-review Ready for review type: feature New feature or capability

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants