IRecordTriggers: per-record lifecycle callbacks for dispose, flush, evict, and disk read#1695
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR centralizes Tsavorite record cleanup by wiring storeFunctions.DisposeRecord into delete/expiration paths, removing scattered disposal calls from elision/free-list helpers and Garnet session functions, and adding targeted tests to validate “dispose-on-delete” behavior.
Changes:
- Invoke
storeFunctions.DisposeRecordvia allocatorDisposeRecord(..., Deleted)and callDisposeRecord(Deleted)at delete/expire decision sites inInternalDelete/InternalRMW. - Remove redundant
DisposeRecordcalls from record elision/free-list transfer helpers; skip tombstoned records during eviction disposal scans. - Remove manual heap disposal/cache-size tracking in Garnet delete/expire session paths; add Tsavorite tests validating one
DisposeRecord(Deleted)per delete/expire.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| libs/storage/Tsavorite/cs/test/DeleteDisposeTests.cs | Adds coverage asserting DisposeRecord(Deleted) fires exactly once for delete/expire flows. |
| libs/storage/Tsavorite/cs/src/core/Index/Tsavorite/Implementation/InternalRMW.cs | Centralizes disposal for ExpireAndStop/Resume and adjusts expiration reinit flow. |
| libs/storage/Tsavorite/cs/src/core/Index/Tsavorite/Implementation/InternalDelete.cs | Centralizes disposal immediately after in-place delete and before sealing. |
| libs/storage/Tsavorite/cs/src/core/Index/Tsavorite/Implementation/Helpers.cs | Removes disposal side-effects from elision/free-list helpers (chain mgmt only). |
| libs/storage/Tsavorite/cs/src/core/Allocator/SpanByteAllocatorImpl.cs | Calls storeFunctions.DisposeRecord on DisposeReason.Deleted before clearing optionals. |
| libs/storage/Tsavorite/cs/src/core/Allocator/ObjectAllocatorImpl.cs | Calls storeFunctions.DisposeRecord on delete; skips tombstones in eviction scan. |
| libs/server/Storage/Functions/UnifiedStore/RMWMethods.cs | Removes manual heap disposal on expiry; relies on Tsavorite delete-site disposal. |
| libs/server/Storage/Functions/UnifiedStore/DeleteMethods.cs | Removes manual heap disposal on delete; relies on Tsavorite delete-site disposal. |
| libs/server/Storage/Functions/ObjectStore/RMWMethods.cs | Removes manual heap disposal on expiry/remove-key; relies on Tsavorite delete-site disposal. |
| libs/server/Storage/Functions/ObjectStore/DeleteMethods.cs | Removes manual heap disposal on delete; relies on Tsavorite delete-site disposal. |
| libs/server/Storage/Functions/GarnetRecordDisposer.cs | Adds cache-size tracking hook on DisposeRecord(Deleted) and introduces holder for late tracker initialization. |
| libs/host/GarnetServer.cs | Wires GarnetRecordDisposer with cache-size tracker holder during store creation. |
| .gitignore | Ignores native bftree build output directory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
badrishc
force-pushed
the
badrishc/delete-dispose-infra
branch
4 times, most recently
from
85cff70 to
02d32d7
Compare
Centralize record disposal at delete/expiration sites: - hlog.DisposeRecord(Deleted) now calls storeFunctions.DisposeRecord before ClearHeapFields in both ObjectAllocatorImpl and SpanByteAllocatorImpl, giving the application a single callback for cache-size tracking, external resource cleanup, etc. - InternalDelete: single DisposeRecord(Deleted) immediately after InPlaceDeleter (mutable) and before Seal (tombstone). - HandleRecordElision / TryTransferToFreeList: remove all DisposeRecord calls — record is already cleaned at the delete site. - InternalRMW: add DisposeRecord(Deleted) for ExpireAndStop and ExpireAndResume at their respective sites, including inside ReinitializeExpiredRecord for the IPU path. Fix pre-existing CAS-failure bug where ClearSourceValueObject disposed the source before CAS (now deferred to post-CAS success via ClearValueIfHeap). Fix ReinitializeExpiredRecord to set tombstone on the source when reinitialize-in-place fails for IPU, so CreateNewRecordRMW uses InitialUpdater instead of CopyUpdater on the disposed source. - DisposeRecordsInRangeForEviction: skip tombstoned records — they were already disposed with DisposeReason.Deleted at the delete site. - Remove all manual heap disposal (AddHeapSize, DisposeValueObject, ClearValueIfHeap) from Garnet InPlaceDeleter and RMW expiration paths in ObjectStore and UnifiedStore session functions. - GarnetRecordDisposer.DisposeRecord handles heap-size tracking only for DisposeReason.Deleted. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
badrishc
force-pushed
the
badrishc/delete-dispose-infra
branch
from
02d32d7 to
4051014
Compare
Mechanical rename across the codebase to reflect the broadened responsibilities (dispose + flush callbacks). Renamed: - IRecordDisposer → IRecordTrigger - DefaultRecordDisposer → DefaultRecordTrigger - SpanByteRecordDisposer → SpanByteRecordTrigger - GarnetRecordDisposer → GarnetRecordTrigger - TRecordDisposer → TRecordTrigger - recordDisposer → recordTrigger - Test disposer types accordingly Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Generic infrastructure for per-record callbacks during page flush and disk reads. These are not BfTree-specific — any record type with external resources can use them. - Add OnFlushRecord(ref LogRecord) + CallOnFlush gate to IRecordTrigger Called on original in-memory records in OnPagesMarkedReadOnlyWorker before pages are flushed to disk. - Add OnDiskReadRecord(ref LogRecord) + CallOnDiskRead gate to IRecordTrigger Called at all 4 ClearBitsForDiskImages sites: Recovery.cs (page scan), AllocatorBase.cs (delta log + async disk read), AllocatorScan.cs (push scan). - Wire through IStoreFunctions and StoreFunctions - Add OnFlushRecordsInRange to ObjectAllocatorImpl - Default implementations: CallOnFlush/CallOnDiskRead => false - GarnetRecordTrigger: CallOnFlush/CallOnDiskRead => false (no BfTree on this branch) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rename all API members for clarity and consistency: - IRecordTrigger → IRecordTriggers (plural: collection of callbacks) - TRecordTrigger → TRecordTriggers, recordTrigger → recordTriggers - DisposeRecord → OnDispose - DisposeValueObject → OnDisposeValueObject - OnFlushRecord → OnFlush - OnDiskReadRecord → OnDiskRead - DisposeOnPageEviction → removed (replaced by CallOnEvict) - GarnetRecordTrigger.cs → GarnetRecordTriggers.cs - IRecordTrigger.cs → IRecordTriggers.cs Separate page eviction from disposal: - Add OnEvict(ref LogRecord) + CallOnEvict as a distinct lifecycle callback - Remove DisposeReason.PageEviction — eviction is not a disposal - EvictRecordsInRange calls OnEvict instead of OnDispose(PageEviction) - OnDispose only handles true disposal reasons (Deleted, CAS failures, etc.) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The blanket sed renamed AsyncIOContext.DisposeRecord() to OnDispose(), but this method disposes IO resources — it's unrelated to IRecordTriggers. Reverted to DisposeRecord() for AsyncIOContext and all its call sites. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
GarnetRecordTrigger → GarnetRecordTriggers DefaultRecordTrigger → DefaultRecordTriggers SpanByteRecordTrigger → SpanByteRecordTriggers TrackingRecordTrigger → TrackingRecordTriggers (test) ObjTrackingRecordTrigger → ObjTrackingRecordTriggers (test) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Make GarnetRecordTriggers a readonly struct with readonly CacheSizeTracker field (reference type survives struct copy, no defensive copies with readonly struct) - Add CacheSizeTracker() parameterless ctor + Initialize(store, ...) for late-bind - Creation order: new CacheSizeTracker() → new GarnetRecordTriggers(tracker) → new TsavoriteKV(...) → tracker.Initialize(store, ...) - Remove CacheSizeTrackerHolder wrapper class entirely Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
badrishc
changed the title
Consistent with EvictRecordsInRange naming. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 93 out of 94 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove Deleted-only gate — OnDispose is now invoked for all DisposeReason values (Deleted, CAS failures, reviv freelist, etc.) in both ObjectAllocatorImpl and SpanByteAllocatorImpl. The application filters by reason in its implementation as needed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
kevin-montrose
approved these changes
Apr 14, 2026
- Remove redundant 'public' modifiers from interface properties - Make DefaultRecordTriggers and SpanByteRecordTriggers readonly structs - Remove unnecessary 'unsafe' from SpanByteRecordTriggers.OnDisposeValueObject - Remove 'readonly' from struct members (redundant with readonly struct) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
badrishc
added a commit
that referenced
this pull request
Apr 14, 2026
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
badrishc
added a commit
that referenced
this pull request
Apr 17, 2026
… tracking Garnet's `CacheSizeTracker` used `LogAccessor.SubscribeEvictions` to drive a `LogSizeTracker.OnNext` observer that, on every page eviction, allocated a scan iterator and walked the page to sum `MemoryUtils.CalculateHeapMemorySize` over each non-null / non-closed record. This is heavyweight for a hot path (buffer-pool allocation, iterator bookkeeping, epoch resume/suspend, and a virtual dispatch per page). This PR migrates the per-page heap-size decrement onto the per-record `IRecordTriggers.OnEvict` hook introduced in #1695, which the object allocator already walks during `EvictRecordsInRange`. That lets us collapse the "scan iterator + observer + sum" path into a single per-record callback directly on the record we would have visited anyway. ### Tsavorite changes - Add `EvictionSource { MainLog, ReadCache }` and thread it through `IRecordTriggers.OnEvict`, `IStoreFunctions.OnEvict`, and `IAllocator.EvictRecordsInRange`. Garnet uses this to route decrements to the correct counter (`AddHeapSize` vs `AddReadCacheHeapSize`). - Add `AllocatorSettings.IsReadCache` and `AllocatorBase.IsReadCache`, set to `true` by `Tsavorite.cs` when constructing the read-cache allocator. This is the cleanest way to distinguish the two allocators at `OnPagesClosedWorker` time without relying on the `evictCallback` sentinel. - `AllocatorBase.EvictPageForRecovery` now also routes through the per-record `EvictRecordsInRange` when `storeFunctions.CallOnEvict` is set. The legacy `MemoryPageScan(observer)` path is preserved as a fallback for consumers that still use `SubscribeEvictions`. - Collapse `AllocatorBase`'s constructor to accept `AllocatorSettings` directly instead of unpacking individual fields at each concrete allocator (`ObjectAllocatorImpl`, `SpanByteAllocatorImpl`, `TsavoriteLogAllocatorImpl`). ### Garnet changes - `GarnetRecordTriggers.CallOnEvict` is now gated on `cacheSizeTracker != null`. `OnEvict(ref LogRecord, EvictionSource)` computes `MemoryUtils.CalculateHeapMemorySize(in logRecord)` and dispatches to `AddHeapSize(-size)` or `AddReadCacheHeapSize(-size)` based on the source. - `CacheSizeTracker.Initialize` replaces the two `SubscribeEvictions` calls with `SetLogSizeTracker` calls so the fast-path size tracking during `TryCopyToTail`, `TryCopyToReadCache`, and object-page growth (`UpdateSize`, `IncrementSize`) continues to work unchanged. ### Parity Behavior at every record state encountered during page eviction is bit-for-bit identical to the prior `SubscribeEvictions` path: | Record state | Old path (iterator) | New path (per-record) | Delta | | ------------------------------------------------ | ---------------------------------- | ---------------------------- | ----- | | Valid, !Sealed, !Tombstone, !IsNull | yielded → -CalculateHeapMemorySize | OnEvict → -Calculate... | same | | `IsNull` | skipped by iterator | skipped by filter | 0 | | `SkipOnScan` (Invalid or Sealed) | skipped by iterator | skipped by filter | 0 | | `Tombstone` (post-delete, already ValueIsInline) | yielded → 0 via `!Info.Tombstone` | skipped by filter → 0 | 0 | Tombstones converge via two independent mechanisms (the old relies on the `if (!Info.Tombstone)` guard inside `CalculateHeapMemorySize`; the new short-circuits in the filter), so both yield 0 contribution as required given the delete-site `OnDispose(Deleted)` already decremented the tracker. ### Testing All tests pass on net10.0 Debug: - `CacheSizeTrackerTests` 2/2 - `RespListTests.ListPushPopStressTest` (10 repetitions) ✓ - `RespListTests`, `RespHashTests`, `RespSetTests`, `RespSortedSetTests`: 467/467 - Tsavorite `DeleteDisposeTests` + `ReadCacheTests`: 228/228, 14 skipped - Clean build on both `Garnet.slnx` and `Tsavorite.slnx` (0 warnings, 0 errors). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
badrishc
added a commit
that referenced
this pull request
Apr 17, 2026
… tracking Garnet's `CacheSizeTracker` used `LogAccessor.SubscribeEvictions` to drive a `LogSizeTracker.OnNext` observer that, on every page eviction, allocated a scan iterator and walked the page to sum `MemoryUtils.CalculateHeapMemorySize` over each non-null / non-closed record. This is heavyweight for a hot path (buffer-pool allocation, iterator bookkeeping, epoch resume/suspend, and a virtual dispatch per page). This PR migrates the per-page heap-size decrement onto the per-record `IRecordTriggers.OnEvict` hook introduced in #1695, which the object allocator already walks during `EvictRecordsInRange`. That lets us collapse the "scan iterator + observer + sum" path into a single per-record callback directly on the record we would have visited anyway. ### Tsavorite changes - Add `EvictionSource { MainLog, ReadCache }` and thread it through `IRecordTriggers.OnEvict`, `IStoreFunctions.OnEvict`, and `IAllocator.EvictRecordsInRange`. Garnet uses this to route decrements to the correct counter (`AddHeapSize` vs `AddReadCacheHeapSize`). - Add `AllocatorSettings.IsReadCache` and `AllocatorBase.IsReadCache`, set to `true` by `Tsavorite.cs` when constructing the read-cache allocator. This is the cleanest way to distinguish the two allocators at `OnPagesClosedWorker` time without relying on the `evictCallback` sentinel. - `AllocatorBase.EvictPageForRecovery` now also routes through the per-record `EvictRecordsInRange` when `storeFunctions.CallOnEvict` is set. The legacy `MemoryPageScan(observer)` path is preserved as a fallback for consumers that still use `SubscribeEvictions`. - Collapse `AllocatorBase`'s constructor to accept `AllocatorSettings` directly instead of unpacking individual fields at each concrete allocator (`ObjectAllocatorImpl`, `SpanByteAllocatorImpl`, `TsavoriteLogAllocatorImpl`). ### Garnet changes - `GarnetRecordTriggers.CallOnEvict` is now gated on `cacheSizeTracker != null`. `OnEvict(ref LogRecord, EvictionSource)` computes `MemoryUtils.CalculateHeapMemorySize(in logRecord)` and dispatches to `AddHeapSize(-size)` or `AddReadCacheHeapSize(-size)` based on the source. - `CacheSizeTracker.Initialize` replaces the two `SubscribeEvictions` calls with `SetLogSizeTracker` calls so the fast-path size tracking during `TryCopyToTail`, `TryCopyToReadCache`, and object-page growth (`UpdateSize`, `IncrementSize`) continues to work unchanged. ### Parity Behavior at every record state encountered during page eviction is bit-for-bit identical to the prior `SubscribeEvictions` path: | Record state | Old path (iterator) | New path (per-record) | Delta | | ------------------------------------------------ | ---------------------------------- | ---------------------------- | ----- | | Valid, !Sealed, !Tombstone, !IsNull | yielded → -CalculateHeapMemorySize | OnEvict → -Calculate... | same | | `IsNull` | skipped by iterator | skipped by filter | 0 | | `SkipOnScan` (Invalid or Sealed) | skipped by iterator | skipped by filter | 0 | | `Tombstone` (post-delete, already ValueIsInline) | yielded → 0 via `!Info.Tombstone` | skipped by filter → 0 | 0 | Tombstones converge via two independent mechanisms (the old relies on the `if (!Info.Tombstone)` guard inside `CalculateHeapMemorySize`; the new short-circuits in the filter), so both yield 0 contribution as required given the delete-site `OnDispose(Deleted)` already decremented the tracker. ### Testing All tests pass on net10.0 Debug: - `CacheSizeTrackerTests` 2/2 - `RespListTests.ListPushPopStressTest` (10 repetitions) ✓ - `RespListTests`, `RespHashTests`, `RespSetTests`, `RespSortedSetTests`: 467/467 - Tsavorite `DeleteDisposeTests` + `ReadCacheTests`: 228/228, 14 skipped - Clean build on both `Garnet.slnx` and `Tsavorite.slnx` (0 warnings, 0 errors). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
badrishc
added a commit
that referenced
this pull request
Apr 17, 2026
… tracking Garnet's `CacheSizeTracker` used `LogAccessor.SubscribeEvictions` to drive a `LogSizeTracker.OnNext` observer that, on every page eviction, allocated a scan iterator and walked the page to sum `MemoryUtils.CalculateHeapMemorySize` over each non-null / non-closed record. This is heavyweight for a hot path (buffer-pool allocation, iterator bookkeeping, epoch resume/suspend, and a virtual dispatch per page). This PR migrates the per-page heap-size decrement onto the per-record `IRecordTriggers.OnEvict` hook introduced in #1695, which the object allocator already walks during `EvictRecordsInRange`. That lets us collapse the "scan iterator + observer + sum" path into a single per-record callback directly on the record we would have visited anyway. ### Tsavorite changes - Add `EvictionSource { MainLog, ReadCache }` and thread it through `IRecordTriggers.OnEvict`, `IStoreFunctions.OnEvict`, and `IAllocator.EvictRecordsInRange`. Garnet uses this to route decrements to the correct counter (`AddHeapSize` vs `AddReadCacheHeapSize`). - Add `AllocatorSettings.IsReadCache` and `AllocatorBase.IsReadCache`, set to `true` by `Tsavorite.cs` when constructing the read-cache allocator. This is the cleanest way to distinguish the two allocators at `OnPagesClosedWorker` time without relying on the `evictCallback` sentinel. - `AllocatorBase.EvictPageForRecovery` now also routes through the per-record `EvictRecordsInRange` when `storeFunctions.CallOnEvict` is set. The legacy `MemoryPageScan(observer)` path is preserved as a fallback for consumers that still use `SubscribeEvictions`. - Collapse `AllocatorBase`'s constructor to accept `AllocatorSettings` directly instead of unpacking individual fields at each concrete allocator (`ObjectAllocatorImpl`, `SpanByteAllocatorImpl`, `TsavoriteLogAllocatorImpl`). ### Garnet changes - `GarnetRecordTriggers.CallOnEvict` is now gated on `cacheSizeTracker != null`. `OnEvict(ref LogRecord, EvictionSource)` computes `MemoryUtils.CalculateHeapMemorySize(in logRecord)` and dispatches to `AddHeapSize(-size)` or `AddReadCacheHeapSize(-size)` based on the source. - `CacheSizeTracker.Initialize` replaces the two `SubscribeEvictions` calls with `SetLogSizeTracker` calls so the fast-path size tracking during `TryCopyToTail`, `TryCopyToReadCache`, and object-page growth (`UpdateSize`, `IncrementSize`) continues to work unchanged. ### Parity Behavior at every record state encountered during page eviction is bit-for-bit identical to the prior `SubscribeEvictions` path: | Record state | Old path (iterator) | New path (per-record) | Delta | | ------------------------------------------------ | ---------------------------------- | ---------------------------- | ----- | | Valid, !Sealed, !Tombstone, !IsNull | yielded → -CalculateHeapMemorySize | OnEvict → -Calculate... | same | | `IsNull` | skipped by iterator | skipped by filter | 0 | | `SkipOnScan` (Invalid or Sealed) | skipped by iterator | skipped by filter | 0 | | `Tombstone` (post-delete, already ValueIsInline) | yielded → 0 via `!Info.Tombstone` | skipped by filter → 0 | 0 | Tombstones converge via two independent mechanisms (the old relies on the `if (!Info.Tombstone)` guard inside `CalculateHeapMemorySize`; the new short-circuits in the filter), so both yield 0 contribution as required given the delete-site `OnDispose(Deleted)` already decremented the tracker. ### Testing All tests pass on net10.0 Debug: - `CacheSizeTrackerTests` 2/2 - `RespListTests.ListPushPopStressTest` (10 repetitions) ✓ - `RespListTests`, `RespHashTests`, `RespSetTests`, `RespSortedSetTests`: 467/467 - Tsavorite `DeleteDisposeTests` + `ReadCacheTests`: 228/228, 14 skipped - Clean build on both `Garnet.slnx` and `Tsavorite.slnx` (0 warnings, 0 errors). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Parity fix: preserve legacy silent-negative decrement ----------------------------------------------------- CI revealed 4 HyperLogLog LTM test failures (server crash on Debug assertion). Root cause: the legacy `LogSizeTracker.OnNext` decrement went straight through `heapSize.Increment(-size)` without an assertion, so if the tracked heap counter transiently undershot zero (which happens for main-log overflow records produced by RMW `CopyUpdater` — e.g. HLL sparse→dense — where no matching `+size` bump exists), the baseline silently tolerated it. The new path was routing through `CacheSizeTracker.AddHeapSize` → `LogSizeTracker.IncrementSize`, which asserts `heapSize.Total >= 0` on any negative delta. In Debug builds that `Debug.Assert` terminates the process (mapped to `DebugAssertException`), causing the Garnet server to crash and StackExchange.Redis to see `SocketClosed`. Fix — introduce an eviction-specific decrement that mirrors `OnNext` exactly: - `LogSizeTracker<T,A>.DecrementSizeOnEvict(long size)` — raw `heapSize.Increment(-size)` with no assertion and no resize signal. - `CacheSizeTracker.DecrementHeapSizeOnEvict(long size, EvictionSource)` — dispatches to the main-log or read-cache tracker. - `GarnetRecordTriggers.OnEvict` now calls `DecrementHeapSizeOnEvict` rather than `AddHeapSize(-size)` / `AddReadCacheHeapSize(-size)`. Heap-counter semantics on eviction are now bit-for-bit identical to the pre-change `SubscribeEvictions` path, including the transient-negative behaviour that existing workloads depend on. Single-page eviction walker robustness -------------------------------------- Tighten `ObjectAllocatorImpl.EvictRecordsInRange` to match the `ObjectScanIterator` guards that `SubscribeEvictions` relied on: - Clip the walk to the start page (`stopAddress = min(endAddress, next-page-start)`) so a multi-page range — not produced by the current callers but defensible in case one ever is — cannot walk off the end of the first page. - Bail on `offset == 0` in addition to `offset + allocatedSize > PageSize`, matching the iterator's "skip to next page header" guard (within a single-page walk the only sane action is to stop). - Document that the caller constrains the range to a single page (`OnPagesClosedWorker` clips per-page, `EvictPageForRecovery` passes one whole page) so the walker's single-page semantics are an intentional invariant, not an accidental simplification. Verified locally: `HyperLogLogPFADD_LTM(32|4096)`, `HyperLogLogTestPFMERGE_LTM_*` all pass; `CacheSizeTrackerTests`, `RespListTests.ListPushPopStressTest[x10]`, and the full `RespListTests`/`RespHashTests`/`RespSortedSetTests`/`RespSetTests` suites (467 tests) all pass.
badrishc
added a commit
that referenced
this pull request
Apr 17, 2026
… tracking Garnet's `CacheSizeTracker` used `LogAccessor.SubscribeEvictions` to drive a `LogSizeTracker.OnNext` observer that, on every page eviction, allocated a scan iterator and walked the page to sum `MemoryUtils.CalculateHeapMemorySize` over each non-null / non-closed record. This is heavyweight for a hot path (buffer-pool allocation, iterator bookkeeping, epoch resume/suspend, and a virtual dispatch per page). This PR migrates the per-page heap-size decrement onto the per-record `IRecordTriggers.OnEvict` hook introduced in #1695, which the object allocator already walks during `EvictRecordsInRange`. That lets us collapse the "scan iterator + observer + sum" path into a single per-record callback directly on the record we would have visited anyway. ### Tsavorite changes - Add `EvictionSource { MainLog, ReadCache }` and thread it through `IRecordTriggers.OnEvict`, `IStoreFunctions.OnEvict`, and `IAllocator.EvictRecordsInRange`. Garnet uses this to route decrements to the correct counter (`AddHeapSize` vs `AddReadCacheHeapSize`). - Add `AllocatorSettings.IsReadCache` and `AllocatorBase.IsReadCache`, set to `true` by `Tsavorite.cs` when constructing the read-cache allocator. This is the cleanest way to distinguish the two allocators at `OnPagesClosedWorker` time without relying on the `evictCallback` sentinel. - `AllocatorBase.EvictPageForRecovery` now also routes through the per-record `EvictRecordsInRange` when `storeFunctions.CallOnEvict` is set. The legacy `MemoryPageScan(observer)` path is preserved as a fallback for consumers that still use `SubscribeEvictions`. - Collapse `AllocatorBase`'s constructor to accept `AllocatorSettings` directly instead of unpacking individual fields at each concrete allocator (`ObjectAllocatorImpl`, `SpanByteAllocatorImpl`, `TsavoriteLogAllocatorImpl`). ### Garnet changes - `GarnetRecordTriggers.CallOnEvict` is now gated on `cacheSizeTracker != null`. `OnEvict(ref LogRecord, EvictionSource)` computes `MemoryUtils.CalculateHeapMemorySize(in logRecord)` and dispatches to `AddHeapSize(-size)` or `AddReadCacheHeapSize(-size)` based on the source. - `CacheSizeTracker.Initialize` replaces the two `SubscribeEvictions` calls with `SetLogSizeTracker` calls so the fast-path size tracking during `TryCopyToTail`, `TryCopyToReadCache`, and object-page growth (`UpdateSize`, `IncrementSize`) continues to work unchanged. ### Parity Behavior at every record state encountered during page eviction is bit-for-bit identical to the prior `SubscribeEvictions` path: | Record state | Old path (iterator) | New path (per-record) | Delta | | ------------------------------------------------ | ---------------------------------- | ---------------------------- | ----- | | Valid, !Sealed, !Tombstone, !IsNull | yielded → -CalculateHeapMemorySize | OnEvict → -Calculate... | same | | `IsNull` | skipped by iterator | skipped by filter | 0 | | `SkipOnScan` (Invalid or Sealed) | skipped by iterator | skipped by filter | 0 | | `Tombstone` (post-delete, already ValueIsInline) | yielded → 0 via `!Info.Tombstone` | skipped by filter → 0 | 0 | Tombstones converge via two independent mechanisms (the old relies on the `if (!Info.Tombstone)` guard inside `CalculateHeapMemorySize`; the new short-circuits in the filter), so both yield 0 contribution as required given the delete-site `OnDispose(Deleted)` already decremented the tracker. ### Testing All tests pass on net10.0 Debug: - `CacheSizeTrackerTests` 2/2 - `RespListTests.ListPushPopStressTest` (10 repetitions) ✓ - `RespListTests`, `RespHashTests`, `RespSetTests`, `RespSortedSetTests`: 467/467 - Tsavorite `DeleteDisposeTests` + `ReadCacheTests`: 228/228, 14 skipped - Clean build on both `Garnet.slnx` and `Tsavorite.slnx` (0 warnings, 0 errors). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Parity fix: preserve legacy silent-negative decrement ----------------------------------------------------- CI revealed 4 HyperLogLog LTM test failures (server crash on Debug assertion). Root cause: the legacy `LogSizeTracker.OnNext` decrement went straight through `heapSize.Increment(-size)` without an assertion, so if the tracked heap counter transiently undershot zero (which happens for main-log overflow records produced by RMW `CopyUpdater` — e.g. HLL sparse→dense — where no matching `+size` bump exists), the baseline silently tolerated it. The new path was routing through `CacheSizeTracker.AddHeapSize` → `LogSizeTracker.IncrementSize`, which asserts `heapSize.Total >= 0` on any negative delta. In Debug builds that `Debug.Assert` terminates the process (mapped to `DebugAssertException`), causing the Garnet server to crash and StackExchange.Redis to see `SocketClosed`. Fix — introduce an eviction-specific decrement that mirrors `OnNext` exactly: - `LogSizeTracker<T,A>.DecrementSizeOnEvict(long size)` — raw `heapSize.Increment(-size)` with no assertion and no resize signal. - `CacheSizeTracker.DecrementHeapSizeOnEvict(long size, EvictionSource)` — dispatches to the main-log or read-cache tracker. - `GarnetRecordTriggers.OnEvict` now calls `DecrementHeapSizeOnEvict` rather than `AddHeapSize(-size)` / `AddReadCacheHeapSize(-size)`. Heap-counter semantics on eviction are now bit-for-bit identical to the pre-change `SubscribeEvictions` path, including the transient-negative behaviour that existing workloads depend on. Single-page eviction walker robustness -------------------------------------- Tighten `ObjectAllocatorImpl.EvictRecordsInRange` to match the `ObjectScanIterator` guards that `SubscribeEvictions` relied on: - Clip the walk to the start page (`stopAddress = min(endAddress, next-page-start)`) so a multi-page range — not produced by the current callers but defensible in case one ever is — cannot walk off the end of the first page. - Bail on `offset == 0` in addition to `offset + allocatedSize > PageSize`, matching the iterator's "skip to next page header" guard (within a single-page walk the only sane action is to stop). - Document that the caller constrains the range to a single page (`OnPagesClosedWorker` clips per-page, `EvictPageForRecovery` passes one whole page) so the walker's single-page semantics are an intentional invariant, not an accidental simplification. Verified locally: `HyperLogLogPFADD_LTM(32|4096)`, `HyperLogLogTestPFMERGE_LTM_*` all pass; `CacheSizeTrackerTests`, `RespListTests.ListPushPopStressTest[x10]`, and the full `RespListTests`/`RespHashTests`/`RespSortedSetTests`/`RespSetTests` suites (467 tests) all pass.
badrishc
added a commit
that referenced
this pull request
Apr 17, 2026
… tracking Garnet's `CacheSizeTracker` used `LogAccessor.SubscribeEvictions` to drive a `LogSizeTracker.OnNext` observer that, on every page eviction, allocated a scan iterator and walked the page to sum `MemoryUtils.CalculateHeapMemorySize` over each non-null / non-closed record. This is heavyweight for a hot path (buffer-pool allocation, iterator bookkeeping, epoch resume/suspend, and a virtual dispatch per page). This PR migrates the per-page heap-size decrement onto the per-record `IRecordTriggers.OnEvict` hook introduced in #1695, which the object allocator already walks during `EvictRecordsInRange`. That lets us collapse the "scan iterator + observer + sum" path into a single per-record callback directly on the record we would have visited anyway. ### Tsavorite changes - Add `EvictionSource { MainLog, ReadCache }` and thread it through `IRecordTriggers.OnEvict`, `IStoreFunctions.OnEvict`, and `IAllocator.EvictRecordsInRange`. Garnet uses this to route decrements to the correct counter (`AddHeapSize` vs `AddReadCacheHeapSize`). - Add `AllocatorSettings.IsReadCache` and `AllocatorBase.IsReadCache`, set to `true` by `Tsavorite.cs` when constructing the read-cache allocator. This is the cleanest way to distinguish the two allocators at `OnPagesClosedWorker` time without relying on the `evictCallback` sentinel. - `AllocatorBase.EvictPageForRecovery` now also routes through the per-record `EvictRecordsInRange` when `storeFunctions.CallOnEvict` is set. The legacy `MemoryPageScan(observer)` path is preserved as a fallback for consumers that still use `SubscribeEvictions`. - Collapse `AllocatorBase`'s constructor to accept `AllocatorSettings` directly instead of unpacking individual fields at each concrete allocator (`ObjectAllocatorImpl`, `SpanByteAllocatorImpl`, `TsavoriteLogAllocatorImpl`). - Tighten `ObjectAllocatorImpl.EvictRecordsInRange` to match `ObjectScanIterator`'s single-page invariant: clip `stopAddress` to the start page, bail on `offset == 0`, and document that both callers (`OnPagesClosedWorker`, `EvictPageForRecovery`) hand single-page ranges. ### Garnet changes - `GarnetRecordTriggers.CallOnEvict` is gated on `cacheSizeTracker is not null && cacheSizeTracker.IsInitialized` so the hook stays off when no memory budget is configured (the tracker is always constructed due to the late-bound store reference, so a raw null-check would always be true). - `GarnetRecordTriggers.OnEvict(ref LogRecord, EvictionSource)` computes `MemoryUtils.CalculateHeapMemorySize(in logRecord)` and dispatches to `AddHeapSize(-size)` or `AddReadCacheHeapSize(-size)` based on the source. This goes through the standard asserting path — the counter must never undershoot zero. - `CacheSizeTracker.Initialize` replaces the two `SubscribeEvictions` calls with `SetLogSizeTracker` calls so the fast-path size tracking during `TryCopyToTail`, `TryCopyToReadCache`, and object-page growth (`UpdateSize`, `IncrementSize`) continues to work unchanged. ### MainStore heap-tracking fix (root cause of negative counter) Routing the eviction decrement through the asserting `AddHeapSize` path surfaced a pre-existing gap: `MainStore` RMW paths never emitted a positive heap-size bump when a record's value was overflow (large inline value spilled to a heap-allocated byte array). Only `Upsert`/in-place-writer paths and the object / unified stores tracked heap on creation. The legacy observer path masked this silently because `OnNext` did a raw decrement with no assertion — the counter quietly went negative on every HLL sparse→dense transition (≈-12 KB per key) and accumulated drift elsewhere. `MainStore/RMWMethods.cs` now emits balanced heap accounting: - `PostInitialUpdater`: `AddHeapSize(+logRecord.CalculateHeapMemorySize())` for the freshly-created record (includes both key and value overflow). - `PostCopyUpdater`: `AddHeapSize(+dst.CalculateHeapMemorySize() - src.CalculateHeapMemorySize())`. The destination is new (+dst); the source becomes sealed and is skipped by both the old iterator and the new `EvictRecordsInRange` walker, so its previously-counted contribution must be subtracted now to keep the counter balanced once `dst` eventually evicts. - `InPlaceUpdater`: snapshot `GetValueHeapMemorySize()` pre-/post-worker and add the delta on `Succeeded`, so value-heap changes (e.g. APPEND growing a large string into overflow) are tracked. The `HyperLogLogPFADD_LTM` suite is the regression that surfaces this — HLL sparse→dense goes through CopyUpdater and allocates ≈12 KB of overflow for the dense representation. With the fix the counter stays balanced and the assertion `heapSize.Total >= 0` holds throughout. ### Parity Behavior at every record state encountered during page eviction is bit-for-bit identical to the prior `SubscribeEvictions` path: | Record state | Old path (iterator) | New path (per-record) | Delta | | ------------------------------------------------ | ---------------------------------- | ---------------------------- | ----- | | Valid, !Sealed, !Tombstone, !IsNull | yielded → -CalculateHeapMemorySize | OnEvict → -Calculate... | same | | `IsNull` | skipped by iterator | skipped by filter | 0 | | `SkipOnScan` (Invalid or Sealed) | skipped by iterator | skipped by filter | 0 | | `Tombstone` (post-delete, already ValueIsInline) | yielded → 0 via `!Info.Tombstone` | skipped by filter → 0 | 0 | Tombstones converge via two independent mechanisms (the old relies on the `if (!Info.Tombstone)` guard inside `CalculateHeapMemorySize`; the new short-circuits in the filter), so both yield 0 contribution as required given the delete-site `OnDispose(Deleted)` already decremented the tracker. ### Testing All on net10.0 Debug: - `HyperLogLogTests` (incl. `HyperLogLogPFADD_LTM{32,4096}`, `HyperLogLogTestPFMERGE_LTM_*`) ✓ - `CacheSizeTrackerTests` ✓ - `RespListTests.ListPushPopStressTest` (10 repetitions) ✓ - `RespListTests`, `RespHashTests`, `RespSetTests`, `RespSortedSetTests`, `RespEtagTests`, `RespBitmapTests`: 367/367 ✓ - Clean build on both `Garnet.slnx` and `Tsavorite.slnx` (0 warnings, 0 errors), `dotnet format --verify-no-changes` clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
badrishc
added a commit
that referenced
this pull request
Apr 17, 2026
… tracking Garnet's `CacheSizeTracker` used `LogAccessor.SubscribeEvictions` to drive a `LogSizeTracker.OnNext` observer that, on every page eviction, allocated a scan iterator and walked the page to sum `MemoryUtils.CalculateHeapMemorySize` over each non-null / non-closed record. This is heavyweight for a hot path (buffer-pool allocation, iterator bookkeeping, epoch resume/suspend, and a virtual dispatch per page). This PR migrates the per-page heap-size decrement onto the per-record `IRecordTriggers.OnEvict` hook introduced in #1695, which the object allocator already walks during `EvictRecordsInRange`. That collapses the "scan iterator + observer + sum" path into a single per-record callback directly on the record we would have visited anyway. ### Tsavorite changes - Add `EvictionSource { MainLog, ReadCache }` and thread it through `IRecordTriggers.OnEvict`, `IStoreFunctions.OnEvict`, and `IAllocator.EvictRecordsInRange`. Garnet uses this to route decrements to the correct counter (`AddHeapSize` vs `AddReadCacheHeapSize`). - Change `IRecordTriggers.CallOnEvict` / `IStoreFunctions.CallOnEvict` from a property to `CallOnEvict(EvictionSource)` so the allocator can skip the per-record eviction walk entirely when the application has no work on that side (e.g. a read-cache-only budget should not force walks of the main-log allocator). - Add `AllocatorSettings.IsReadCache` and `AllocatorBase.IsReadCache`, set to `true` by `Tsavorite.cs` when constructing the read-cache allocator. This is the cleanest way to distinguish the two allocators at `OnPagesClosedWorker` time without relying on the `evictCallback` sentinel, and is used to pass the correct `EvictionSource` to `CallOnEvict`. - `AllocatorBase.EvictPageForRecovery` routes through the per-record `EvictRecordsInRange` when `storeFunctions.CallOnEvict(source)` is set. The legacy `MemoryPageScan(observer)` path is preserved as a fallback for consumers that still use `SubscribeEvictions`. - Collapse `AllocatorBase`'s constructor to accept `AllocatorSettings` directly instead of unpacking individual fields at each concrete allocator (`ObjectAllocatorImpl`, `SpanByteAllocatorImpl`, `TsavoriteLogAllocatorImpl`). - Tighten `ObjectAllocatorImpl.EvictRecordsInRange` to match `ObjectScanIterator`'s single-page invariant: clip `stopAddress` to the start page, bail on `offset == 0`, and document that both callers (`OnPagesClosedWorker`, `EvictPageForRecovery`) hand single-page ranges. ### Garnet changes - `GarnetRecordTriggers.CallOnEvict(EvictionSource)` returns true only for the sides that are actually configured (`mainLogTracker` and/or `readCacheTracker` non-null), avoiding pointless per-record walks on the untracked allocator. - `GarnetRecordTriggers.OnEvict(ref LogRecord, EvictionSource)` computes `MemoryUtils.CalculateHeapMemorySize(in logRecord)` and dispatches to `AddHeapSize(-size)` or `AddReadCacheHeapSize(-size)` based on the source. This goes through the standard asserting path — the counter must never undershoot zero. - `CacheSizeTracker.Initialize` replaces the two `SubscribeEvictions` calls with `SetLogSizeTracker` calls so the fast-path size tracking during `TryCopyToTail`, `TryCopyToReadCache`, and object-page growth (`UpdateSize`, `IncrementSize`) continues to work unchanged. ### MainStore heap-tracking fix (root cause of negative counter) Routing the eviction decrement through the asserting `AddHeapSize` path surfaced a pre-existing gap: `MainStore` record-creation paths never emitted a positive heap-size bump when a record's key or value spilled to overflow (large inline field backed by a heap-allocated byte array). The legacy observer path masked this silently because `OnNext` did a raw decrement with no assertion — the counter quietly went negative on every HLL sparse→dense transition (≈-12 KB per key) and accumulated drift elsewhere. `MainStore` now emits balanced heap accounting at all create/update/delete entry points that go through a typed function callback rather than the shared `SessionFunctionsUtils` writer (which already tracks deltas): - `RMWMethods.PostInitialUpdater`: `+logRecord.CalculateHeapMemorySize()` for the freshly-created record. - `RMWMethods.PostCopyUpdater`: `+dstLogRecord.CalculateHeapMemorySize()` for the new record only. The source is not subtracted — the `TSourceLogRecord` may be an in-memory main-log record (whose heap is tracked in `mainLogTracker` and will leak upward by a bounded sealed- source amount, parity with `ObjectStore`'s `ClearSource=false` branch and the legacy observer path), a read-cache record (heap lives in `readCacheTracker`, unrelated to this write), or a pending-IO `DiskLogRecord` (heap never counted in any tracker). Subtracting unconditionally would undercount in the last two cases and drive the counter negative. - `RMWMethods.InPlaceUpdater`: `GetValueHeapMemorySize()` pre/post delta on `Succeeded`, so value-heap changes (e.g. APPEND, SETRANGE, or any path that triggers `ReallocateValueOverflow` / `ConvertInlineToOverflow` / `ConvertOverflowToInline`) are tracked. - `UpsertMethods.PostInitialWriter` (all three overloads): `+logRecord.CalculateHeapMemorySize()` for SET-style inserts that create a new record through the Upsert path. - `DeleteMethods.InPlaceDeleter`: `-logRecord.CalculateHeapMemorySize()` before the wrapper sets Tombstone. After tombstone is set, `CalculateHeapMemorySize` short- circuits to zero and `EvictRecordsInRange` skips the record, so without this decrement the creation-side increments would leak for every DEL of an overflow record. The `HyperLogLogPFADD_LTM` suite is the regression that first surfaced this — HLL sparse→dense goes through CopyUpdater and allocates ≈12 KB of overflow for the dense representation. With the fix the counter stays balanced and the assertion `heapSize.Total >= 0` holds throughout. ### Parity Behavior at every record state encountered during page eviction is bit-for-bit identical to the prior `SubscribeEvictions` path: | Record state | Old path (iterator) | New path (per-record) | Delta | | ------------------------------------------------ | ---------------------------------- | ---------------------------- | ----- | | Valid, !Sealed, !Tombstone, !IsNull | yielded → -CalculateHeapMemorySize | OnEvict → -Calculate... | same | | `IsNull` | skipped by iterator | skipped by filter | 0 | | `SkipOnScan` (Invalid or Sealed) | skipped by iterator | skipped by filter | 0 | | `Tombstone` (post-delete, already ValueIsInline) | yielded → 0 via `!Info.Tombstone` | skipped by filter → 0 | 0 | ### Hot-path cost Fast paths (SET / GET / INCR on inline-sized values) pay only a few aggressive-inlined inline-bit checks; `GetValueHeapMemorySize` on an inline record early-returns 0 before any tracker call, and a `heap != 0` guard skips the `AddHeapSize` dispatch entirely. Tracker work occurs only when a record genuinely created, resized, or freed an overflow/object allocation — which was already a heavyweight event. ### Testing All on net10.0 Debug: - `HyperLogLogTests` (incl. `HyperLogLogPFADD_LTM{32,4096}`, `HyperLogLogTestPFMERGE_LTM_*`) ✓ - `CacheSizeTrackerTests` ✓ - `RespListTests` (incl. `ListPushPopStressTest` ×10), `RespHashTests`, `RespSetTests`, `RespSortedSetTests`, `RespEtagTests`, `RespBitmapTests`, `RespTests.Set*`, `RespTests.Del*`: 517/517 ✓ - Clean build on both `Garnet.slnx` and `Tsavorite.slnx` (0 warnings, 0 errors), `dotnet format --verify-no-changes` clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
badrishc
added a commit
that referenced
this pull request
Apr 19, 2026
* Replace SubscribeEvictions with IRecordTriggers.OnEvict for heap-size tracking Garnet's `CacheSizeTracker` used `LogAccessor.SubscribeEvictions` to drive a `LogSizeTracker.OnNext` observer that, on every page eviction, allocated a scan iterator and walked the page to sum `MemoryUtils.CalculateHeapMemorySize` over each non-null / non-closed record. This is heavyweight for a hot path (buffer-pool allocation, iterator bookkeeping, epoch resume/suspend, and a virtual dispatch per page). This PR migrates the per-page heap-size decrement onto the per-record `IRecordTriggers.OnEvict` hook introduced in #1695, which the object allocator already walks during `EvictRecordsInRange`. That collapses the "scan iterator + observer + sum" path into a single per-record callback directly on the record we would have visited anyway. ### Tsavorite changes - Add `EvictionSource { MainLog, ReadCache }` and thread it through `IRecordTriggers.OnEvict`, `IStoreFunctions.OnEvict`, and `IAllocator.EvictRecordsInRange`. Garnet uses this to route decrements to the correct counter (`AddHeapSize` vs `AddReadCacheHeapSize`). - Change `IRecordTriggers.CallOnEvict` / `IStoreFunctions.CallOnEvict` from a property to `CallOnEvict(EvictionSource)` so the allocator can skip the per-record eviction walk entirely when the application has no work on that side (e.g. a read-cache-only budget should not force walks of the main-log allocator). - Add `AllocatorSettings.IsReadCache` and `AllocatorBase.IsReadCache`, set to `true` by `Tsavorite.cs` when constructing the read-cache allocator. This is the cleanest way to distinguish the two allocators at `OnPagesClosedWorker` time without relying on the `evictCallback` sentinel, and is used to pass the correct `EvictionSource` to `CallOnEvict`. - `AllocatorBase.EvictPageForRecovery` routes through the per-record `EvictRecordsInRange` when `storeFunctions.CallOnEvict(source)` is set. The legacy `MemoryPageScan(observer)` path is preserved as a fallback for consumers that still use `SubscribeEvictions`. - Collapse `AllocatorBase`'s constructor to accept `AllocatorSettings` directly instead of unpacking individual fields at each concrete allocator (`ObjectAllocatorImpl`, `SpanByteAllocatorImpl`, `TsavoriteLogAllocatorImpl`). - Tighten `ObjectAllocatorImpl.EvictRecordsInRange` to match `ObjectScanIterator`'s single-page invariant: clip `stopAddress` to the start page, bail on `offset == 0`, and document that both callers (`OnPagesClosedWorker`, `EvictPageForRecovery`) hand single-page ranges. ### Garnet changes - `GarnetRecordTriggers.CallOnEvict(EvictionSource)` returns true only for the sides that are actually configured (`mainLogTracker` and/or `readCacheTracker` non-null), avoiding pointless per-record walks on the untracked allocator. - `GarnetRecordTriggers.OnEvict(ref LogRecord, EvictionSource)` computes `MemoryUtils.CalculateHeapMemorySize(in logRecord)` and dispatches to `AddHeapSize(-size)` or `AddReadCacheHeapSize(-size)` based on the source. This goes through the standard asserting path — the counter must never undershoot zero. - `CacheSizeTracker.Initialize` replaces the two `SubscribeEvictions` calls with `SetLogSizeTracker` calls so the fast-path size tracking during `TryCopyToTail`, `TryCopyToReadCache`, and object-page growth (`UpdateSize`, `IncrementSize`) continues to work unchanged. ### MainStore heap-tracking fix (root cause of negative counter) Routing the eviction decrement through the asserting `AddHeapSize` path surfaced a pre-existing gap: `MainStore` record-creation paths never emitted a positive heap-size bump when a record's key or value spilled to overflow (large inline field backed by a heap-allocated byte array). The legacy observer path masked this silently because `OnNext` did a raw decrement with no assertion — the counter quietly went negative on every HLL sparse→dense transition (≈-12 KB per key) and accumulated drift elsewhere. `MainStore` now emits balanced heap accounting at all create/update/delete entry points that go through a typed function callback rather than the shared `SessionFunctionsUtils` writer (which already tracks deltas): - `RMWMethods.PostInitialUpdater`: `+logRecord.CalculateHeapMemorySize()` for the freshly-created record. - `RMWMethods.PostCopyUpdater`: `+dstLogRecord.CalculateHeapMemorySize()` for the new record only. The source is not subtracted — the `TSourceLogRecord` may be an in-memory main-log record (whose heap is tracked in `mainLogTracker` and will leak upward by a bounded sealed- source amount, parity with `ObjectStore`'s `ClearSource=false` branch and the legacy observer path), a read-cache record (heap lives in `readCacheTracker`, unrelated to this write), or a pending-IO `DiskLogRecord` (heap never counted in any tracker). Subtracting unconditionally would undercount in the last two cases and drive the counter negative. - `RMWMethods.InPlaceUpdater`: `GetValueHeapMemorySize()` pre/post delta on `Succeeded`, so value-heap changes (e.g. APPEND, SETRANGE, or any path that triggers `ReallocateValueOverflow` / `ConvertInlineToOverflow` / `ConvertOverflowToInline`) are tracked. - `UpsertMethods.PostInitialWriter` (all three overloads): `+logRecord.CalculateHeapMemorySize()` for SET-style inserts that create a new record through the Upsert path. - `DeleteMethods.InPlaceDeleter`: `-logRecord.CalculateHeapMemorySize()` before the wrapper sets Tombstone. After tombstone is set, `CalculateHeapMemorySize` short- circuits to zero and `EvictRecordsInRange` skips the record, so without this decrement the creation-side increments would leak for every DEL of an overflow record. The `HyperLogLogPFADD_LTM` suite is the regression that first surfaced this — HLL sparse→dense goes through CopyUpdater and allocates ≈12 KB of overflow for the dense representation. With the fix the counter stays balanced and the assertion `heapSize.Total >= 0` holds throughout. ### Parity Behavior at every record state encountered during page eviction is bit-for-bit identical to the prior `SubscribeEvictions` path: | Record state | Old path (iterator) | New path (per-record) | Delta | | ------------------------------------------------ | ---------------------------------- | ---------------------------- | ----- | | Valid, !Sealed, !Tombstone, !IsNull | yielded → -CalculateHeapMemorySize | OnEvict → -Calculate... | same | | `IsNull` | skipped by iterator | skipped by filter | 0 | | `SkipOnScan` (Invalid or Sealed) | skipped by iterator | skipped by filter | 0 | | `Tombstone` (post-delete, already ValueIsInline) | yielded → 0 via `!Info.Tombstone` | skipped by filter → 0 | 0 | ### Hot-path cost Fast paths (SET / GET / INCR on inline-sized values) pay only a few aggressive-inlined inline-bit checks; `GetValueHeapMemorySize` on an inline record early-returns 0 before any tracker call, and a `heap != 0` guard skips the `AddHeapSize` dispatch entirely. Tracker work occurs only when a record genuinely created, resized, or freed an overflow/object allocation — which was already a heavyweight event. ### Testing All on net10.0 Debug: - `HyperLogLogTests` (incl. `HyperLogLogPFADD_LTM{32,4096}`, `HyperLogLogTestPFMERGE_LTM_*`) ✓ - `CacheSizeTrackerTests` ✓ - `RespListTests` (incl. `ListPushPopStressTest` ×10), `RespHashTests`, `RespSetTests`, `RespSortedSetTests`, `RespEtagTests`, `RespBitmapTests`, `RespTests.Set*`, `RespTests.Del*`: 517/517 ✓ - Clean build on both `Garnet.slnx` and `Tsavorite.slnx` (0 warnings, 0 errors), `dotnet format --verify-no-changes` clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(object-store): eliminate redundant Clone() in ObjectStore PostCopyUpdater For in-memory CopyUpdate, Tsavorite's HeapObjectBase.CacheSerializedObjectData cloned src.ValueObject into dst.ValueObject and Garnet's PostCopyUpdater then cloned again, overwriting the first clone. Every ObjectStore CopyUpdate was paying for two Clone() calls and one immediate GC. Fix by making CacheSerializedObjectData the single point of cloning for all sources (memory and pending-IO DiskLogRecord), and stripping the redundant clone in Garnet's PostCopyUpdater. Tsavorite changes (IHeapObject / HeapObjectBase / InternalRMW): * Drop the unused 'ref LogRecord srcLogRecord' parameter from CacheSerializedObjectData; its only uses were an identity assert and a 'srcLogRecord.ValueObject'-as-'this' redirect. * Add an explicit 'bool srcIsOnMemoryLog' parameter so the method can safely run for DiskLogRecord sources (which cannot be expressed as ref LogRecord). When false, perform the clone and return immediately - skipping the serialization state machine, which is meaningless for ephemeral disk sources (the (v) data is already persisted on disk from a prior flush, 'this' is about to be disposed up the pending chain, and ClearSourceValueObject is ignored by InternalRMW for non-memory sources). * In InternalRMW, drop the 'isMemoryLogRecord' guard around the call, so cloning is always delegated to CacheSerializedObjectData regardless of source kind. Garnet changes (ObjectStore RMWMethods): * PostCopyUpdater now reads 'dstLogRecord.ValueObject' directly - the clone is always performed upstream. No conditional clone path remains here. The single surviving clone site still runs after the successful CAS, preserving CopyUpdater's deferred-allocation-until-CAS invariant. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * refactor(object-store): split PCU heap tracking into creation + disposal sites Before: ObjectStore PostCopyUpdater conditionally emitted +new-old when rmwInfo.ClearSourceValueObject was true, otherwise +new. This coupled PCU to a Tsavorite-internal signal and combined two concerns (creation of (v+1) and removal of (v)) into one branching call site. After: mirror MainStore's pattern by splitting into two unconditional sites. * PostCopyUpdater always emits +value.HeapMemorySize for the (v+1) creation. Matches PostInitialUpdater / PostInitialWriter already doing +new.heap. * OnDisposeValueObject(DisposeReason.CopyUpdated) emits -valueObject.HeapMemorySize for the (v) removal. This callback fires from InternalRMW.ClearValueIfHeap exactly when the source is cleared eagerly (ClearSourceValueObject=true and isMemoryLogRecord), which is the literal 'source freed now' signal. Checkpoint/disk paths that leave the source alive do not reach the dispose site; their decrement is emitted later by OnEvict when the sealed source page evicts (via CalculateHeapMemorySize, which includes ValueObject.HeapMemorySize). Net tracker state is bit-for-bit identical to the previous conditional form, but each site now has a single arithmetic direction and no awareness of Tsavorite's internal ClearSourceValueObject flag. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Remove OnDisposeValueObject and disposer lambdas Simplify the record-lifecycle contract for IHeapObject values: - Remove IRecordTriggers.OnDisposeValueObject; OnDispose(ref LogRecord, reason) is now the single app-facing hook. Garnet's impl merges the Deleted and CopyUpdated branches into one heap-size decrement. - Drop the Action<IHeapObject> disposer lambda threaded through ObjectIdMap.Free, LogField.ClearObjectIdAndConvertToInline, LogRecord.ClearValueIfHeap/ClearHeapFields/Dispose, DiskLogRecord ctors, PendingContext.CopyFrom, ConditionalCopyToTail, scan iterators, and ISourceLogRecord.ClearValueIfHeap. - ObjectIdMap.Free(int) is now strictly slot reclamation; it never disposes. - DiskLogRecord.Dispose() calls ValueObject?.Dispose() itself, covering all its owners (PendingContext, AsyncIOContext, scan iterators, cluster migration / replication streaming, deserialized-from-disk). - InternalRMW CopyUpdated path routes through storeFunctions.OnDispose(ref srcMemLogRecord, CopyUpdated) followed by srcMemLogRecord.ClearValueIfHeap(). - ObjectAllocatorImpl.OnDispose(ref DiskLogRecord) becomes a no-op. - Update DeleteDisposeTests to the new single-hook surface. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Add OnDisposeDiskRecord trigger for symmetric DiskLogRecord disposal Eliminates the asymmetry where DiskLogRecord.Dispose() unconditionally called ValueObject.Dispose() while on-log LogRecord disposal routed through the IRecordTriggers.OnDispose trigger. All record disposal now goes through IRecordTriggers, giving Garnet full control over whether to dispose the value object. - Add IRecordTriggers.OnDisposeDiskRecord(ref DiskLogRecord, DisposeReason) as a required interface member with explicit no-op overrides in DefaultRecordTriggers, SpanByteRecordTriggers, and GarnetRecordTriggers (avoids DIM/interface-dispatch overhead via JIT monomorphization). - Forward through IStoreFunctions/StoreFunctions to IAllocator, renamed IAllocator.OnDispose(ref DiskLogRecord) -> OnDisposeDiskRecord. ObjectAllocatorImpl forwards to storeFunctions (was no-op); SpanByteAllocator stays no-op (no IHeapObject); TsavoriteLogAllocator throws NotImplementedException. - Remove ValueObject?.Dispose() from DiskLogRecord.Dispose() — callers now fire the trigger before Dispose. Fixes latent bug in scan-iterator DiskLogRecord wrappers that shared ValueObject references with the still-alive on-log record. - Fire the trigger at all DiskLogRecord disposal sites: AllocatorBase (happy-path + retry/catch), AllocatorScan.GetFromDiskAndPushToReader, TsavoriteThread.InternalCompletePendingRequestFromContext, and the four cluster migrate/replication command sites. - Update test IRecordTriggers impls (TrackingRecordTriggers, ObjTrackingRecordTriggers) to satisfy the new interface member. All 102 relevant Tsavorite tests and 472 Garnet object-store tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Add RecordLifecycleTests covering IRecordTriggers call counts and dispose accounting Adds 12 tests that precisely assert when and how many times each IRecordTriggers callback fires across the full record lifecycle. Complements the existing DeleteDisposeTests with coverage of the new OnDisposeDiskRecord hook and the refactored DisposeReason / EvictionSource discriminators introduced in this PR. Trackers: - TrackedObjectValue (subclass of TestObjectValue) counts per-instance IHeapObject.Dispose invocations so tests can catch double-dispose across any combination of on-log, scan, pending-read, and eviction paths. - LifecycleTracker counts OnDispose/OnDisposeDiskRecord per DisposeReason, OnEvict per EvictionSource, plus OnFlush / OnDiskRead — with toggleable CallOn* flags so tests can verify gating. Covered behaviours: - OnDispose(CopyUpdated) fires exactly once on immutable RMW; value dispose count is exactly 1 when handler opts in. - OnDispose(Deleted) fires exactly once on delete; no OnDisposeDiskRecord. - In-memory scan fires OnDisposeDiskRecord(DeserializedFromDisk) exactly N times for N records and does NOT dispose shared value objects. - Disk scan fires the same hook exactly K times and disposes exactly K values when opt-in, with no double-dispose. - Pending read from disk fires exactly one OnDisposeDiskRecord, no OnDispose, no OnEvict. - Page eviction fires OnEvict(MainLog) exactly (live - tombstoned) times with no re-firing of OnDispose(Deleted) for tombstones. - CallOnFlush / CallOnDiskRead / per-source CallOnEvict gating fully suppress their respective callbacks when false. - Read cache eviction fires OnEvict(ReadCache) and not OnEvict(MainLog). - No value object is ever disposed more than once across an upsert → delete → re-upsert → evict → pending-read cycle. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(unified-store): remove legacy ClearSourceValueObject branch from PCU The earlier refactor in eeb6a3b split ObjectStore PCU heap-size tracking into two sites (PCU emits +new unconditionally, OnDispose(CopyUpdated) emits -old when the source is cleared eagerly). The same split was needed in UnifiedStore, but its PostCopyUpdater was missed. Result: on an EXPIRE/PERSIST that went through CopyUpdate for an object record (e.g. SADD followed by EXPIRE), UnifiedStore.PCU still emitted +new - old while GarnetRecordTriggers.OnDispose(CopyUpdated) also emitted -old, double-decrementing the tracked heap size. A subsequent OnDispose(Deleted) from ReinitializeExpiredRecord then drove the counter below zero and tripped the Debug.Assert in LogSizeTracker.IncrementSize (HeapSize.Total should be >= 0). Fix: mirror ObjectStore — PCU emits +new.HeapMemorySize unconditionally, and the matching -old is emitted at the removal site (OnDispose(CopyUpdated) or OnEvict for checkpoint/disk paths). Fixes the four CI failures surfaced on the replace-subscribe-eviction branch: - RespTests.ReAddExpiredKey - RespTransactionProcTests.TransactionObjectExpiryProcTest - RespCustomCommandTests.CustomObjectCommandTest2 - RespSortedSetGeoTests.CanUseGeoSearchStoreWithDeleteKeyWhenSourceNotFound Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * style: fix trailing newline in RecordLifecycleTests.cs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: handle all heap types in OnDispose(Deleted) and harden DiskLogRecord disposal Three fixes from independent design reviews: 1. GarnetRecordTriggers.OnDispose(Deleted): use CalculateHeapMemorySize instead of only ValueObject.HeapMemorySize. This ensures overflow key/value bytes on MainStore string records are correctly decremented on RMW expire paths (ExpireAndResume, ExpireAndStop) where InPlaceDeleter does not run. CalculateHeapMemorySize returns 0 for tombstoned records, so this is a natural no-op on the mutable Delete() path where MainStore.InPlaceDeleter already subtracted before tombstone was set. 2. Cluster migration/replication: prevent double-trigger of OnDisposeDiskRecord by resetting diskLogRecord to default after the normal-path dispose, and guarding the finally/catch block with IsSet. Previously, a non-noop trigger implementation could receive a callback on a default/already-disposed record. 3. IRecordTriggers.OnDisposeDiskRecord: make it a default interface method (no-op) instead of requiring explicit implementation. This reduces the implementation burden on consumers who don't need disk-record lifecycle hooks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: visit sealed source records during eviction to prevent overflow heap leak EvictRecordsInRange previously skipped all SkipOnScan records (Sealed OR Invalid). This leaked tracked heap for sealed-but-valid source records from mutable-region CopyUpdates and immutable-region deletes, whose overflow key/value bytes were never decremented from the tracker. Fix: skip only Invalid records (already disposed/elided) and Tombstoned records (already decremented at delete site). Sealed-but-Valid records are now visited by OnEvict, which correctly picks up their remaining heap contribution (overflow bytes, and value objects kept alive during checkpoint). For records where OnDispose(Deleted) already cleared the value (immutable delete), CalculateHeapMemorySize returns only key overflow (if any) — no double-decrement because OnDispose decremented the full amount when the record was not yet tombstoned, and ClearHeapFields(clearKey=false) zeroed the value slot afterward. OnEvict sees only what remains (key overflow if present, typically 0 for inline keys). Updated PageEvictionFiresOnEvictForEveryLiveRecord test to use bounds assertions reflecting that sealed source records from immutable-region deletes are now correctly visited. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * refactor: move tombstone after OnDispose so triggers see pre-tombstone state Session functions (ISessionFunctions) should not be responsible for heap-size tracking on destruction paths — that's IRecordTriggers' job. Two places were setting Tombstone BEFORE OnDispose(Deleted) fired, causing CalculateHeapMemorySize to return 0 and preventing the trigger from decrementing the tracked heap: 1. SessionFunctionsWrapper.InPlaceDeleter: set Tombstone+Dirty before returning to InternalDelete, which then called OnDispose(Deleted). Fixed by moving SetTombstone+SetDirtyAndModified to InternalDelete after OnDispose. 2. SessionFunctionsWrapper.InPlaceUpdater (ExpireAndStop): set Tombstone before returning to InternalRMW, which then called OnDispose(Deleted). Fixed by moving SetTombstone+SetDirtyAndModified to InternalRMW after OnDispose. This removes the manual cacheSizeTracker decrement from MainStore.InPlaceDeleter (it was a workaround for the wrong ordering) and makes GarnetRecordTriggers.OnDispose the single destruction-side decrement path for all record types. Also tightened PageEvictionFiresOnEvictForEveryLiveRecord test to use precise counts by observing TailAddress movement to distinguish mutable vs immutable deletes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * refactor: internalize CopyUpdated value-object accounting into Tsavorite The CopyUpdated path is a partial clear (value-object slot only, key stays on the record). Expecting IRecordTriggers implementers to know this nuance — that they should subtract only ValueObject.HeapMemorySize and not the full CalculateHeapMemorySize — is error-prone. Move the accounting entirely into Tsavorite's InternalRMW: - Decrement logSizeTracker by ValueObject.HeapMemorySize before clearing - Call IHeapObject.Dispose() on the freed value object - Then ClearValueIfHeap() nulls the ObjectIdMap slot Remove OnDispose(CopyUpdated) from the trigger call site — the trigger is no longer involved in CopyUpdate accounting. GarnetRecordTriggers.OnDispose now only handles DisposeReason.Deleted. The IRecordTriggers doc is updated to reflect that CopyUpdated is handled internally. The trigger contract is now simple: - OnDispose(Deleted): subtract CalculateHeapMemorySize (full record heap) - OnEvict: subtract CalculateHeapMemorySize (whatever remains at eviction) - Creation sites (PostInitialUpdater/Writer/CopyUpdater, InPlaceUpdater): add the new/changed heap Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * chore: clean up stale comments across PR - EvictRecordsInRange: update XML doc and skip-condition comments to reflect that sealed records are now visited (not skipped) and InPlaceDeleter no longer does heap tracking - GarnetRecordTriggers.OnDispose: simplify comment now that tombstone is always set AFTER OnDispose in all paths - Remove legacy SubscribeEvictions reference from EvictRecordsInRange doc Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: use value-only heap in OnDispose(Deleted) to prevent key-overflow double-subtract OnDispose(Deleted) now uses GetValueHeapMemorySize() instead of CalculateHeapMemorySize(). This prevents double-subtracting key overflow bytes for immutable-region deletes where: 1. OnDispose(Deleted) fires on the source (subtracts full heap including key) 2. ClearHeapFields(clearKey=false) keeps key alive for chain traversal 3. Source is sealed (not tombstoned) — OnEvict visits it later 4. CalculateHeapMemorySize returns key overflow → subtracted again With the fix: - OnDispose(Deleted) subtracts value-only heap (value overflow + value object) - For mutable deletes: tombstoned → OnEvict skips → key overflow is a bounded phantom freed by GC when the page is freed (same as pre-IRecordTriggers) - For immutable deletes: sealed → OnEvict visits → subtracts key overflow once - Net: no double-decrement, bounded key-overflow phantom on mutable deletes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * refactor: move all destruction-side heap accounting into Tsavorite Tsavorite now handles ALL heap-size decrements internally via logSizeTracker: - OnDispose(Deleted): decrements value heap before ClearHeapFields - EvictRecordsInRange: decrements key overflow for ALL records (including tombstoned), decrements value heap for non-tombstoned records - CopyUpdated: decrements value-object heap (already internalized earlier) GarnetRecordTriggers.OnDispose and OnEvict are now no-ops for accounting. CallOnEvict returns false. The trigger callbacks remain available for app-level resource cleanup (e.g. IDisposable.Dispose on value objects that hold external resources), but Garnet does not use them. The heap-tracking contract is now cleanly split: - Session functions: emit +value at creation sites (only the app knows the heap size of a newly created value) - Tsavorite: emit -key and -value at all destruction/eviction sites (the record is in hand, Tsavorite can read the sizes directly) EvictRecordsInRange is now called whenever logSizeTracker is set OR CallOnEvict is true, ensuring internal accounting runs even when the application opts out of the OnEvict trigger. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: track key overflow at CAS success sites for balanced accounting Key overflow was only tracked (+key) for internal TryCopyToTail/TryCopyToReadCache copies via logSizeTracker.UpdateSize. Records created by session functions (RMW, Upsert, Delete) never added +key, but EvictRecordsInRange subtracted -key at eviction — causing the tracker to undercount for overflow-key records. Fix: add logSizeTracker.IncrementSize(+key) at the CAS success site in InternalRMW, InternalUpsert, and InternalDelete. This pairs with the -key emitted by EvictRecordsInRange for all records (including tombstoned). The heap-tracking responsibility is now cleanly split: - Tsavorite: all key accounting (+key at CAS, -key at eviction) and all destruction-side value accounting (-value at OnDispose/eviction/CopyUpdated) - Session functions: +value at creation sites only Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * refactor: move creation-site value heap tracking from ISF into Tsavorite Previously, every ISessionFunctions implementation (MainStore, ObjectStore, UnifiedStore, VectorStore) had to manually call cacheSizeTracker.AddHeapSize in PostInitialUpdater, PostInitialWriter, and PostCopyUpdater. This was error-prone — VectorSessionFunctions was missing these calls entirely, causing heap tracking to go negative on delete (server crash). Move all creation-site +value tracking into Tsavorite's CAS success sites and ReinitializeExpiredRecord: - InternalRMW: +value after PostInitialUpdater and PostCopyUpdater - InternalUpsert: +value after PostInitialWriter - ReinitializeExpiredRecord: +value after PostInitialUpdater (IPU path) ISF implementations now only handle ±delta for in-place updates (InPlaceUpdater/InPlaceWriter), where only the app knows the before/after sizes. All other heap tracking is fully internal to Tsavorite. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: use HeapMemorySize instead of TotalSize in GetValueHeapMemorySize GetValueHeapMemorySize() for overflow values was returning OverflowByteArray.TotalSize (Array.Length) while CalculateHeapMemorySize() used OverflowByteArray.HeapMemorySize (Array.Length + 24 for .NET byte array object overhead). This 24-byte-per-record mismatch caused the heap tracker to go negative for overflow-value records (e.g. HLL dense) because +value at creation used TotalSize but -value at eviction used HeapMemorySize. Fix: change GetValueHeapMemorySize() to use HeapMemorySize, matching CalculateHeapMemorySize(). Verified all other heap sizing paths (KeyOverflow, ValueObject, CAS sites, eviction, OnDispose, IPU delta) already use HeapMemorySize consistently. Fixes HyperLogLogPFADD_LTM, HyperLogLogTestPFMERGE_LTM_DenseToDense, and HyperLogLogTestPFMERGE_LTM_SparseToDense test failures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * chore: remove cacheSizeTracker from GarnetRecordTriggers GarnetRecordTriggers no longer does any heap-size accounting — all tracking is internal to Tsavorite via logSizeTracker. Remove the cacheSizeTracker field, constructor parameter, and all heap-accounting comments. The struct is now a pure no-op placeholder for future app-level resource cleanup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * chore: simplify CallOnEvict to parameterless property, polish comments - Change CallOnEvict from method with EvictionSource parameter to a simple bool property. The per-source gating is no longer needed since Tsavorite handles all heap accounting internally. The EvictionSource is still passed to OnEvict for informational purposes. - Update IRecordTriggers doc: remove heap-accounting implementation details from OnDispose/OnDisposeDiskRecord/OnEvict docs — these are now Tsavorite- internal. The trigger API is purely for app-level resource cleanup. - Update InternalDelete/InternalRMW/SessionFunctionsWrapper comments to reference internal heap accounting instead of trigger-based accounting. - Simplify RecordLifecycleTests CallOnEvict gating test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * chore: make CallOnFlush/CallOnEvict/CallOnDiskRead default to false via DIM All three gate properties on IRecordTriggers now have default implementations returning false. Implementations that don't need any trigger callbacks (like GarnetRecordTriggers) no longer need to explicitly declare them — the empty struct body suffices. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * perf: skip GetValueHeapMemorySize for inline values in MainStore IPU Add a ValueIsInline bit check before and after InPlaceUpdaterWorker to avoid the expensive GetValueHeapMemorySize call (which chases pointers through ObjectIdMap) on the common inline-value path. Only measure heap when the value was or became non-inline (overflow/object transition). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: add +value for in-chain revivification and +key for tombstoned recovery Two bugs found by GPT-5.4 review: 1. In-chain revivification (TryRevivifyInChain in InternalRMW and InternalUpsert) revived a tombstoned record without re-adding its value heap to the tracker. The value was subtracted at OnDispose(Deleted) when the record was deleted, but never re-added when the tombstone was reused for a new value. Fix: add GetValueHeapMemorySize() after PostInitialUpdater/Writer in both paths. 2. Recovery page-read used CalculateHeapMemorySize (which returns 0 for tombstones) but eviction later subtracts key overflow for tombstoned records. Fix: for tombstoned records during recovery, explicitly add KeyOverflow.HeapMemorySize instead of relying on CalculateHeapMemorySize. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: account for elided/freelist heap and prevent SubscribeEvictions double-subtract Two bugs found by Goldeneye review: 1. Elided and RevivificationFreeList dispose paths cleared heap fields (ClearHeapFields with clearKey=true) without subtracting from the tracker. Eviction skips invalid records, so the heap was never decremented. Fix: OnDispose now subtracts CalculateHeapMemorySize for Elided and RevivificationFreeList reasons (with tombstone-aware key-only fallback). 2. SubscribeEvictions(LogSizeTracker) sets both onEvictionObserver AND logSizeTracker to the same object. OnPagesClosedWorker and EvictPageForRecovery then run both MemoryPageScan (observer.OnNext) and EvictRecordsInRange (logSizeTracker), double-subtracting. Fix: skip MemoryPageScan when onEvictionObserver IS logSizeTracker. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: apply sizeChange delta before HasRemoveKey early return in ObjectStore IPU ObjectStore InPlaceUpdaterWorker's HasRemoveKey path (e.g. SPOP/LPOP/HDEL/ZREM removing the last element) returned false with ExpireAndStop without applying the sizeChange from Operate. The mutation had already happened in-place (HeapMemorySize updated on the object), but the tracker never saw the negative delta. OnDispose(Deleted) then subtracted only the post-removal empty-collection size, leaving the pre-removal payload permanently over-counted. Fix: apply sizeChange to cacheSizeTracker before the early return so the tracker reflects the in-place mutation before ExpireAndStop disposes the record. Found by Opus 4.7 review. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Move IPU/IPW heap-size tracking from ISF into Tsavorite; remove Operate sizeChange Move in-place update heap delta tracking from session functions into Tsavorite's InternalRMW and InternalUpsert. Pre/post GetValueHeapMemorySize measurements around InPlaceUpdater and InPlaceWriter capture the delta and route it to logSizeTracker. This includes the ExpireAndStop path where the object is mutated before IPU returns false. Remove the out memorySizeChange parameter from IGarnetObject.Operate and all implementations (Hash, List, Set, SortedSet, CustomObjectBase). The size tracking is now fully handled by Tsavorite, so Operate no longer needs to compute or return heap deltas. Remove cacheSizeTracker calls from MainStore RMWMethods and SessionFunctionsUtils (TryCopyAndUpdateOverflowValue) since Tsavorite now handles these deltas internally. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: avoid double heap-decrement in ReinitializeExpiredRecord IPU path When ReinitializeExpiredRecord runs inside InPlaceUpdater, the outer pre/post delta tracking in InternalRMW already captures the net heap change. ReinitializeExpiredRecord was also calling OnDispose(Deleted) which decremented the tracker AND cleared the value via ClearHeapFields, causing the outer pre/post to see postHeap=0 and apply a second subtraction. This made the tracker go negative, triggering a Debug.Assert crash in LogSizeTracker. Fix: for the IPU path, skip the tracker decrement but keep the disposal side-effects (trigger + ClearHeapFields). Also remove the explicit +valueHeap after PostInitialUpdater since the outer pre/post captures that too. The CU path is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: badrishc <badrishc@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
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.
Summary
Introduces
IRecordTriggers, a per-record lifecycle callback interface onStoreFunctionsthat centralizes record disposal at delete/expiration sites and adds new callbacks for page flush, eviction, and disk read. This is generic Tsavorite + Garnet infrastructure — not tied to any specific data type.IRecordTriggers API
OnDispose(ref LogRecord, DisposeReason)OnFlush(ref LogRecord)CallOnFlushOnPagesMarkedReadOnlyWorker)OnEvict(ref LogRecord)CallOnEvictOnDiskRead(ref LogRecord)CallOnDiskReadOnDisposeValueObject(IHeapObject, DisposeReason)All callbacks default to no-op. Gate properties (
CallOnFlush,CallOnEvict,CallOnDiskRead) prevent overhead when unused.Dispose lifecycle changes (Tsavorite)
hlog.OnDispose(Deleted)now callsstoreFunctions.OnDisposebeforeClearHeapFieldsin bothObjectAllocatorImplandSpanByteAllocatorImplInternalDelete: singleOnDispose(Deleted)immediately after InPlaceDeleter (mutable) and before Seal (tombstone)HandleRecordElision/TryTransferToFreeList: removed all OnDispose calls — record already cleaned at delete siteInternalRMW:OnDispose(Deleted)for ExpireAndStop and ExpireAndResume at all paths (IPU, NCU, CU), including insideReinitializeExpiredRecordClearSourceValueObjectdisposed source before CAS (now deferred to post-CAS)ReinitializeExpiredRecord: sets tombstone on failure for IPU soCreateNewRecordRMWuses InitialUpdaterEvictRecordsInRange: callsOnEvict(separate fromOnDispose); skips tombstoned recordsDisposeReason.PageEviction— eviction is a distinct lifecycle event viaOnEvictFlush and disk-read callbacks (Tsavorite)
OnFlushRecordsInRange: iterates original in-memory records inOnPagesMarkedReadOnlyWorkerbefore flush, gated byCallOnFlushOnDiskRead: wired at all 4ClearBitsForDiskImagessites (Recovery.cs, AllocatorBase.cs delta log + async read, AllocatorScan.cs push scan), gatedby
CallOnDiskReadGarnet changes
GarnetRecordTriggers(readonly struct): implementsIRecordTriggerswithCacheSizeTrackerfor heap-size tracking on delete. All gate propertiesfalseon this branch (no external resources).CacheSizeTrackerHolderwrapper —CacheSizeTrackeruses late-boundInitialize(store, ...)to break the circular dependency.InPlaceDeleterand RMW expiration paths.Naming
IRecordDisposer→IRecordTriggers(interface)DefaultRecordTrigger→DefaultRecordTriggers, etc. (all implementations plural)DisposeRecord→OnDispose,DisposeValueObject→OnDisposeValueObjectDisposeOnPageEviction→ removed (replaced byCallOnEvict+OnEvict)Tests
18 DeleteDisposeTests (8 SpanByte + 10 Object) covering delete mutable/immutable, ExpireAndStop, ExpireAndResume, IPU-fail paths. All assert exact counts for both
OnDispose(Deleted)andOnDisposeValueObject(Deleted).