Skip to content

Commit 7531edc

Browse files
badrishcCopilot
andcommitted
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 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>
1 parent 8113f56 commit 7531edc

15 files changed

Lines changed: 100 additions & 28 deletions

libs/server/Storage/Functions/GarnetRecordTriggers.cs

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,13 @@ namespace Garnet.server
77
{
88
/// <summary>
99
/// Record lifecycle triggers for Garnet's unified store. Handles per-record cleanup
10-
/// on delete via <see cref="IRecordTriggers.OnDispose"/>.
10+
/// on delete via <see cref="IRecordTriggers.OnDispose"/> and per-record heap-size
11+
/// accounting on page eviction via <see cref="IRecordTriggers.OnEvict"/>.
1112
/// </summary>
1213
public readonly struct GarnetRecordTriggers : IRecordTriggers
1314
{
1415
/// <summary>
15-
/// Cache size tracker for heap size accounting on delete.
16+
/// Cache size tracker for heap size accounting on delete and eviction.
1617
/// Created before the store and initialized after via <see cref="CacheSizeTracker.Initialize"/>.
1718
/// </summary>
1819
internal readonly CacheSizeTracker cacheSizeTracker;
@@ -29,7 +30,10 @@ public GarnetRecordTriggers(CacheSizeTracker cacheSizeTracker)
2930
public bool CallOnFlush => false;
3031

3132
/// <inheritdoc/>
32-
public bool CallOnEvict => false;
33+
// Drives per-record heap-size decrement on page eviction. Mirrors the work the
34+
// legacy SubscribeEvictions → LogSizeTracker.OnNext observer path used to perform
35+
// (see CacheSizeTracker.Initialize for the wiring change).
36+
public bool CallOnEvict => cacheSizeTracker != null;
3337

3438
/// <inheritdoc/>
3539
public bool CallOnDiskRead => false;
@@ -49,5 +53,24 @@ public void OnDispose(ref LogRecord logRecord, DisposeReason reason)
4953
cacheSizeTracker?.AddHeapSize(-logRecord.ValueObject.HeapMemorySize);
5054
}
5155
}
56+
57+
/// <inheritdoc/>
58+
public void OnEvict(ref LogRecord logRecord, EvictionSource source)
59+
{
60+
if (cacheSizeTracker is null)
61+
return;
62+
63+
// Decrement heap size by this record's heap contribution. Uses the same sizing
64+
// helper that LogSizeTracker.OnNext used to sum over an evicted iterator, so the
65+
// total decrement for any evicted page range is identical to the pre-change path.
66+
var size = MemoryUtils.CalculateHeapMemorySize(in logRecord);
67+
if (size == 0)
68+
return;
69+
70+
if (source == EvictionSource.ReadCache)
71+
cacheSizeTracker.AddReadCacheHeapSize(-size);
72+
else
73+
cacheSizeTracker.AddHeapSize(-size);
74+
}
5275
}
5376
}

libs/server/Storage/SizeTracker/CacheSizeTracker.cs

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -65,8 +65,9 @@ public CacheSizeTracker(TsavoriteKV<StoreFunctions, StoreAllocator> store, long
6565
=> Initialize(store, targetSize, readCacheTargetSize, loggerFactory);
6666

6767
/// <summary>
68-
/// Initialize the tracker with a store. Subscribes to eviction notifications.
69-
/// Called after store creation when using the parameterless constructor.
68+
/// Initialize the tracker with a store. Wires the <see cref="LogSizeTracker"/> as the fast-path
69+
/// size tracker for copy-to-tail / copy-to-readcache. Per-record heap-size decrement on page
70+
/// eviction is driven by <see cref="GarnetRecordTriggers.OnEvict"/>.
7071
/// </summary>
7172
public void Initialize(TsavoriteKV<StoreFunctions, StoreAllocator> store, long targetSize, long readCacheTargetSize, ILoggerFactory loggerFactory = null)
7273
{
@@ -77,14 +78,14 @@ public void Initialize(TsavoriteKV<StoreFunctions, StoreAllocator> store, long t
7778
{
7879
mainLogTracker = new LogSizeTracker<StoreFunctions, StoreAllocator>(store.Log, targetSize,
7980
targetSize / HighTargetSizeDeltaFraction, targetSize / LowTargetSizeDeltaFraction, loggerFactory?.CreateLogger("MainLogSizeTracker"));
80-
store.Log.SubscribeEvictions(mainLogTracker);
81+
store.Log.SetLogSizeTracker(mainLogTracker);
8182
}
8283

8384
if (store.ReadCache != null && readCacheTargetSize > 0)
8485
{
8586
readCacheTracker = new LogSizeTracker<StoreFunctions, StoreAllocator>(store.ReadCache, readCacheTargetSize,
8687
readCacheTargetSize / HighTargetSizeDeltaFraction, readCacheTargetSize / LowTargetSizeDeltaFraction, loggerFactory?.CreateLogger("ReadCacheSizeTracker"));
87-
store.ReadCache.SubscribeEvictions(readCacheTracker);
88+
store.ReadCache.SetLogSizeTracker(readCacheTracker);
8889
}
8990
}
9091

libs/storage/Tsavorite/cs/src/core/Allocator/AllocatorBase.cs

Lines changed: 27 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -205,6 +205,12 @@ protected string BaseToString(string fuaDetails = "")
205205
/// <summary>Observer for records getting evicted from memory (page closed). May be the same object as <see cref="logSizeTracker"/>.</summary>
206206
internal IObserver<ITsavoriteScanIterator> onEvictionObserver;
207207

208+
/// <summary>
209+
/// Whether this allocator is the read cache (as opposed to the main hybrid log).
210+
/// Set once at construction from <see cref="AllocatorSettings.IsReadCache"/>.
211+
/// </summary>
212+
internal readonly bool IsReadCache;
213+
208214
/// <summary>Log size tracker; called when an operation at the Tsavorite-internal level adds or removes heap memory size
209215
/// (e.g. copying to log tail or read cache, which do not call <see cref="ISessionFunctions{TInputOutput, TContext}"/>).
210216
/// May be the same object as <see cref="onEvictionObserver"/>.</summary>
@@ -553,9 +559,15 @@ internal void WriteInlinePageAsync<TContext>(IntPtr alignedSourceAddress, ulong
553559

554560
/// <summary>Instantiate base allocator implementation</summary>
555561
[MethodImpl(MethodImplOptions.NoInlining)]
556-
private protected AllocatorBase(LogSettings logSettings, TStoreFunctions storeFunctions, Func<object, TAllocator> wrapperCreator, Action<long, long> evictCallback,
557-
LightEpoch epoch, Action<CommitInfo> flushCallback, ILogger logger = null, ObjectIdMap transientObjectIdMap = null)
562+
private protected AllocatorBase(AllocatorSettings allocatorSettings, TStoreFunctions storeFunctions, Func<object, TAllocator> wrapperCreator,
563+
ILogger logger = null, ObjectIdMap transientObjectIdMap = null)
558564
{
565+
var logSettings = allocatorSettings.LogSettings;
566+
var evictCallback = allocatorSettings.evictCallback;
567+
var epoch = allocatorSettings.epoch;
568+
var flushCallback = allocatorSettings.flushCallback;
569+
IsReadCache = allocatorSettings.IsReadCache;
570+
559571
this.storeFunctions = storeFunctions;
560572
_wrapper = wrapperCreator(this);
561573

@@ -1403,10 +1415,19 @@ public void ShiftBeginAddress(long newBeginAddress, bool truncateLog, bool noFlu
14031415
/// <summary>Invokes eviction observer if set and then frees the page.</summary>
14041416
internal void EvictPageForRecovery(long page)
14051417
{
1406-
if (logSizeTracker is not null)
1418+
var start = GetLogicalAddressOfStartOfPage(page);
1419+
var end = GetLogicalAddressOfStartOfPage(page + 1);
1420+
1421+
if (storeFunctions.CallOnEvict)
1422+
{
1423+
// New per-record path: OnEvict handles heap-size decrement per record. Parity
1424+
// with the runtime eviction path in OnPagesClosedWorker.
1425+
_wrapper.EvictRecordsInRange(start, end, IsReadCache ? EvictionSource.ReadCache : EvictionSource.MainLog);
1426+
}
1427+
else if (logSizeTracker is not null)
14071428
{
1408-
var start = GetLogicalAddressOfStartOfPage(page);
1409-
var end = GetLogicalAddressOfStartOfPage(page + 1);
1429+
// Legacy observer path: materialize an iterator and push heap-size decrement to
1430+
// the LogSizeTracker.OnNext observer (kept for consumers still using SubscribeEvictions).
14101431
MemoryPageScan(start, end, logSizeTracker);
14111432
}
14121433

@@ -1490,7 +1511,7 @@ private void OnPagesClosedWorker()
14901511

14911512
// Notify application of records being evicted — allows cleanup of external resources.
14921513
if (storeFunctions.CallOnEvict)
1493-
_wrapper.EvictRecordsInRange(start, end);
1514+
_wrapper.EvictRecordsInRange(start, end, IsReadCache ? EvictionSource.ReadCache : EvictionSource.MainLog);
14941515

14951516
// If we are using a null storage device, we must also shift BeginAddress (leave it in-memory)
14961517
if (IsNullDevice)

libs/storage/Tsavorite/cs/src/core/Allocator/AllocatorSettings.cs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,12 @@ public struct AllocatorSettings
2323
/// <summary>The action to call on page eviction; used only for readcache</summary>
2424
internal Action<long, long> evictCallback;
2525

26+
/// <summary>
27+
/// Whether this allocator is the read cache (as opposed to the main hybrid log).
28+
/// Used to tag per-record eviction callbacks so applications can distinguish the source.
29+
/// </summary>
30+
internal bool IsReadCache;
31+
2632
/// <summary>The action to execute on flush completion; used only for <see cref="TsavoriteLog"/></summary>
2733
internal Action<CommitInfo> flushCallback;
2834

libs/storage/Tsavorite/cs/src/core/Allocator/IAllocator.cs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,9 @@ RecordSizeInfo GetDeleteRecordSize<TKey>(TKey key)
117117
/// <see cref="IRecordTriggers.OnEvict"/> hook for each valid, non-tombstoned record.
118118
/// Used during page eviction to allow cleanup of external resources.
119119
/// </summary>
120-
void EvictRecordsInRange(long startAddress, long endAddress);
120+
/// <param name="startAddress">Start logical address of the range.</param>
121+
/// <param name="endAddress">End logical address of the range (exclusive).</param>
122+
/// <param name="source">Identifies whether this eviction is from the main log or the read cache.</param>
123+
void EvictRecordsInRange(long startAddress, long endAddress, EvictionSource source);
121124
}
122125
}

libs/storage/Tsavorite/cs/src/core/Allocator/ObjectAllocator.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,6 @@ public readonly RecordSizeInfo GetDeleteRecordSize<TKey>(TKey key)
146146
public readonly void OnDispose(ref DiskLogRecord logRecord, DisposeReason disposeReason) => _this.OnDispose(ref logRecord, disposeReason);
147147

148148
/// <inheritdoc/>
149-
public readonly void EvictRecordsInRange(long startAddress, long endAddress) => _this.EvictRecordsInRange(startAddress, endAddress);
149+
public readonly void EvictRecordsInRange(long startAddress, long endAddress, EvictionSource source) => _this.EvictRecordsInRange(startAddress, endAddress, source);
150150
}
151151
}

libs/storage/Tsavorite/cs/src/core/Allocator/ObjectAllocatorImpl.cs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ internal struct ObjectPage
7474
public override string ToString() => BaseToString($" (LI {LastIssuedFlushedUntilAddress}, OG {OngoingFlushedUntilAddress}, No {NoFlushUntilAddress})");
7575

7676
public ObjectAllocatorImpl(AllocatorSettings settings, TStoreFunctions storeFunctions, Func<object, ObjectAllocator<TStoreFunctions>> wrapperCreator)
77-
: base(settings.LogSettings, storeFunctions, wrapperCreator, settings.evictCallback, settings.epoch, settings.flushCallback, settings.logger, transientObjectIdMap: new ObjectIdMap())
77+
: base(settings, storeFunctions, wrapperCreator, settings.logger, transientObjectIdMap: new ObjectIdMap())
7878
{
7979
objectLogDevice = settings.LogSettings.ObjectLogDevice;
8080

@@ -347,7 +347,7 @@ internal void OnDispose(ref DiskLogRecord logRecord, DisposeReason disposeReason
347347
/// Iterate records in the given logical address range and call <see cref="IStoreFunctions.OnEvict"/>
348348
/// on each valid, non-tombstoned record. Used during page eviction to allow cleanup of external resources.
349349
/// </summary>
350-
internal void EvictRecordsInRange(long startAddress, long endAddress)
350+
internal void EvictRecordsInRange(long startAddress, long endAddress, EvictionSource source)
351351
{
352352
// Ensure we start after the page header
353353
var page = GetPage(startAddress);
@@ -378,7 +378,7 @@ internal void EvictRecordsInRange(long startAddress, long endAddress)
378378
}
379379

380380
// Notify the application that this record is being evicted from memory.
381-
storeFunctions.OnEvict(ref logRecord);
381+
storeFunctions.OnEvict(ref logRecord, source);
382382

383383
address += allocatedSize;
384384
}

libs/storage/Tsavorite/cs/src/core/Allocator/SpanByteAllocator.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,6 @@ public readonly RecordSizeInfo GetDeleteRecordSize<TKey>(TKey key)
145145
public void OnDispose(ref DiskLogRecord logRecord, DisposeReason disposeReason) => _this.OnDispose(ref logRecord, disposeReason);
146146

147147
/// <inheritdoc/>
148-
public void EvictRecordsInRange(long startAddress, long endAddress) { }
148+
public void EvictRecordsInRange(long startAddress, long endAddress, EvictionSource source) { }
149149
}
150150
}

libs/storage/Tsavorite/cs/src/core/Allocator/SpanByteAllocatorImpl.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ internal sealed unsafe class SpanByteAllocatorImpl<TStoreFunctions> : AllocatorB
1515
private OverflowPool<PageUnit<Empty>> freePagePool;
1616

1717
public SpanByteAllocatorImpl(AllocatorSettings settings, TStoreFunctions storeFunctions, Func<object, SpanByteAllocator<TStoreFunctions>> wrapperCreator)
18-
: base(settings.LogSettings, storeFunctions, wrapperCreator, settings.evictCallback, settings.epoch, settings.flushCallback, settings.logger)
18+
: base(settings, storeFunctions, wrapperCreator, settings.logger)
1919
{
2020
freePagePool = new OverflowPool<PageUnit<Empty>>(4, p => { });
2121
pageHeaderSize = PageHeader.Size;

libs/storage/Tsavorite/cs/src/core/Allocator/TsavoriteLogAllocator.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,6 @@ public readonly RecordSizeInfo GetDeleteRecordSize<TKey>(TKey key)
144144
public void OnDispose(ref DiskLogRecord logRecord, DisposeReason disposeReason) => throw new NotImplementedException("Not implemented for TsavoriteLogAllocator");
145145

146146
/// <inheritdoc/>
147-
public void EvictRecordsInRange(long startAddress, long endAddress) { }
147+
public void EvictRecordsInRange(long startAddress, long endAddress, EvictionSource source) { }
148148
}
149149
}

0 commit comments

Comments
 (0)