Skip to content

Preserve keyed identity when converting cache changes to a list - #1192

Open
dwcullop wants to merge 2 commits into
mainfrom
u/dacullop/main/removekey-keyed-identity
Open

dwcullop wants to merge 2 commits into
mainfrom
u/dacullop/main/removekey-keyed-identity

Conversation

@dwcullop

Copy link
Copy Markdown
Member

Problem

RemoveKey() projected each cache change straight into a list change with no memory of where items lived. Equal-valued items, or multiple references to the same object, collapsed into each other because downstream list consumers had to fall back on item equality to locate an entry. Removals and refreshes for keys whose position had never been supplied produced unusable indexes, corrupting any bound list.

Fixes #1182

Fix

RemoveKey() now tracks positions by cache key, per subscription, before discarding the key from the output:

  • A List<ItemWithIndex<TKey>> holds only the positions actually observed, with a Dictionary<TKey, int> mirror so refresh lookups are O(1) instead of scanning.
  • Supplied add/update/remove/move indexes are always preserved and used to maintain the map; appends infer keys.Count only while the observed history is complete.
  • Update stays a Remove/Add pair and Refresh stays a self-Replace, matching existing behavior; the difference is that the position now comes from the key rather than from value equality.
  • Partial streams are handled explicitly: an unindexed removal or update of an untracked key, an insert beyond the known extent, or a conflicting supplied index clears the inferred state and reverts to unspecified indexes. Later indexed changes re-establish known positions. Nothing throws, and no wrapped or invented index is ever emitted.

Because positions key off the cache key, two entries with equal values (or the same object reference added twice under different keys) remain distinct, which is the core of the reported bug.

Tradeoff

This replaces a stateless Select projection with per-subscription state: one list entry and one dictionary entry per observed key, allocated on subscribe rather than per changeset. In exchange the per-changeset path allocates a single right-sized ChangeSet<TObject> (changes.Count + changes.Updates) instead of walking an enumerator, and refresh lookups drop from a linear scan to a dictionary probe. For the pathological equal-value cases that motivated the issue, the previous code was not merely slower, it was wrong. The memory cost scales with distinct keys observed, which is bounded by the cache the operator is reading from.

Validation

Covered by focused fixtures added alongside the fix: RemoveKeyFixture.Identity.cs exercises distinct positioning for equal values and shared references, and RemoveKeyFixture.Compatibility.cs pins the existing Update/Refresh/Moved change-reason shapes and partial-stream behavior so the change is verified non-breaking for current consumers. Documentation for the operator's change-reason table was updated in the cache and list instruction files.

@dwcullop

Copy link
Copy Markdown
Member Author

P1: Bulk removal introduces quadratic processing.

Location: ObservableCacheEx.RemoveKey.cs, lines 192-220.

Each removal walks the surviving suffix and rewrites its list positions and dictionary entries. Clearing a cache in insertion order therefore performs a quadratic number of index updates.

Measured with source.Connect().RemoveKey().Subscribe(), with no downstream list materializer:

Entries cleared PR Base commit
13,727 1,424 ms 3.66 ms
27,454 5,148 ms 3.23 ms
54,908 22,307 ms 4.73 ms

These are local measurements, not a statistical benchmark, but the suffix-update loop explains the scaling. A single synchronous cache clear now blocks for seconds at ordinary collection sizes.

Suggested fix: avoid maintaining every absolute index after each individual removal. Batch position maintenance for the changeset or use an ordered key index with efficient rank lookup and removal. Preserve the individual output reasons and their sequential indexes rather than changing the public output to a Clear. Add bulk-clear and mixed-removal benchmarks so improving refresh lookup does not hide the removal cost.

@dwcullop

Copy link
Copy Markdown
Member Author

P1: Partial streams can remove the wrong list item.

Location: ObservableCacheEx.RemoveKey.cs, lines 128-130, with canInferAppendIndex initialized to true at line 60.

The inferred index assumes this subscription has observed the complete collection. A Preview() subscription does not include existing items, so keys.Count is not the absolute append position in a pre-populated target list.

Reproduced against the PR and its base commit:

  1. Populate a source cache and target list with 7455.
  2. Subscribe through source.Preview().RemoveKey().Clone(target).
  3. Add 9938, then remove 9938.

The PR leaves [9938], having removed the original item. The base correctly leaves [7455]. The newly observed addition is tracked at index zero even though it was appended after the existing item.

Suggested fix: distinguish complete-history streams from partial streams before inferring absolute positions. Preserve unspecified indexes when completeness is not established, while retaining keyed positions for complete snapshots. The generic changeset stream does not identify whether its first batch is a snapshot, so this needs an explicit contract or separate path rather than another heuristic based on the first observed change. Add a regression covering a pre-populated target, a late Preview() subscription, and an add/remove pair before any unknown-key event occurs.

@dwcullop

Copy link
Copy Markdown
Member Author

Follow-up on the design, separate from the two defect comments above.

The contract question I raised (complete stream versus partial history) has an answer that simplifies this considerably: RemoveKey() can assume each subscription starts from an empty collection and sees every change applied to it. An already-populated cache fits that model too, since its contents arrive through Connect() as additions to the subscriber''s initially empty state.

Under that contract, the operator needs only enough state to translate key identity into list position. Adds establish positions, updates/removes/refreshes locate the occurrence by key, moves adjust it. Equal values stay distinguishable because the key is retained internally until the outgoing change has an index. No value storage is duplicated, and no inference is required.

That means:

  • The identity tracking itself is justified. Erasing the key before resolving a position is information loss, and the existing fallback in RemoveKeyEnumerator (unindexed self-replace) plus Filter.Static''s IndexOfOptional(change.Item.Current) lookup is exactly where equal-valued entries get confused. Fixing that needs state.
  • The partial-history machinery is not. Inferred append positions, sparse positions, invalidation and recovery, and conflicting-index handling all exist to survive a history the contract does not have to accept. They cannot reliably supply information that was never in the stream, which is what the Preview() reproduction demonstrates. I would drop that layer and drop the claim of partial-stream compatibility rather than extend it.
  • The bulk-removal cost still needs addressing. Starting empty does not make it acceptable to rewrite every surviving dictionary entry after each removal, which is what produced the 22-second Clear() measurement.

Net: a small complete-history adapter that maintains ordered keys and positions, with a structure chosen so structural edits are not O(n) each. Deciding the contract first is what shrinks the implementation.

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.

[Bug]: RemoveKey loses entry identity for equal values and produces invalid list indexes

1 participant