Skip to content

Commit 53389b6

Browse files
badrishcCopilot
badrishc
and
Copilot
committed
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>
1 parent fe94d95 commit 53389b6

7 files changed

Lines changed: 71 additions & 98 deletions

File tree

‎libs/server/Storage/Functions/GarnetRecordTriggers.cs‎

Lines changed: 13 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -31,42 +31,22 @@ public GarnetRecordTriggers(CacheSizeTracker cacheSizeTracker)
3131
public bool CallOnFlush => false;
3232

3333
/// <inheritdoc/>
34-
// Drives per-record heap-size decrement on page eviction. Mirrors the work the
35-
// legacy SubscribeEvictions → LogSizeTracker.OnNext observer path used to perform
36-
// (see CacheSizeTracker.Initialize for the wiring change). Gated per source so
37-
// that enabling only a main-log or only a read-cache memory budget does not
38-
// force a per-record eviction walk on the other allocator.
39-
public bool CallOnEvict(EvictionSource source)
40-
{
41-
if (cacheSizeTracker is null)
42-
return false;
43-
return source == EvictionSource.ReadCache
44-
? cacheSizeTracker.readCacheTracker is not null
45-
: cacheSizeTracker.mainLogTracker is not null;
46-
}
34+
// Heap-size accounting on eviction is now handled internally by Tsavorite's
35+
// EvictRecordsInRange via logSizeTracker. The OnEvict trigger is only needed
36+
// for app-level resource cleanup, which Garnet does not require.
37+
public bool CallOnEvict(EvictionSource source) => false;
4738

4839
/// <inheritdoc/>
4940
public bool CallOnDiskRead => false;
5041

5142
/// <inheritdoc/>
5243
public void OnDispose(ref LogRecord logRecord, DisposeReason reason)
5344
{
54-
if (cacheSizeTracker is null)
55-
return;
56-
57-
if (reason == DisposeReason.Deleted)
58-
{
59-
// Decrement only the VALUE-side heap (value overflow bytes or value-object heap).
60-
// Key overflow is NOT subtracted here because ClearHeapFields(clearKey=false) keeps the
61-
// key alive for hash-chain traversal. Key overflow is handled separately:
62-
// - Mutable delete: record is tombstoned after OnDispose → OnEvict skips it → key overflow
63-
// is a bounded phantom (freed by GC when the page is freed, same as pre-IRecordTriggers).
64-
// - Immutable delete: source is sealed (not tombstoned) → OnEvict visits it and subtracts
65-
// the remaining key overflow via CalculateHeapMemorySize.
66-
var size = logRecord.GetValueHeapMemorySize();
67-
if (size != 0)
68-
cacheSizeTracker.AddHeapSize(-size);
69-
}
45+
// Heap-size accounting for destruction is handled internally by Tsavorite
46+
// (logSizeTracker). This trigger is available for app-level resource cleanup
47+
// (e.g. calling IDisposable.Dispose on value objects that hold external resources).
48+
// Garnet's IHeapObject implementations (Hash/List/Set/SortedSet) hold no external
49+
// resources, so this is a no-op.
7050
}
7151

7252
/// <inheritdoc/>
@@ -81,22 +61,10 @@ public void OnDisposeDiskRecord(ref DiskLogRecord logRecord, DisposeReason reaso
8161
/// <inheritdoc/>
8262
public void OnEvict(ref LogRecord logRecord, EvictionSource source)
8363
{
84-
if (cacheSizeTracker is null)
85-
return;
86-
87-
// Decrement heap size by this record's heap contribution. Uses the same sizing
88-
// helper that LogSizeTracker.OnNext used to sum over an evicted iterator. Routes
89-
// through the standard AddHeapSize/AddReadCacheHeapSize path so the assertion
90-
// guarding against negative totals remains in force; creation sites on the main
91-
// log (RMW PostInitialUpdater/PostCopyUpdater and in-place grow/shrink) must emit
92-
// a matching positive bump so the account stays balanced.
93-
var size = MemoryUtils.CalculateHeapMemorySize(in logRecord);
94-
if (size == 0)
95-
return;
96-
if (source == EvictionSource.ReadCache)
97-
cacheSizeTracker.AddReadCacheHeapSize(-size);
98-
else
99-
cacheSizeTracker.AddHeapSize(-size);
64+
// Heap-size accounting for eviction is handled internally by Tsavorite
65+
// (logSizeTracker decrements both key overflow and value heap in EvictRecordsInRange).
66+
// This trigger is available for app-level resource cleanup on eviction.
67+
// Garnet has no app-level eviction cleanup, so this is a no-op.
10068
}
10169
}
10270
}

‎libs/server/Storage/Functions/MainStore/RMWMethods.cs‎

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -399,9 +399,9 @@ public readonly void PostInitialUpdater(ref LogRecord logRecord, in RecordSizeIn
399399
rmwInfo.UserData |= NeedAofLog; // Mark that we need to write to AOF
400400
}
401401

402-
// Account for any heap contribution (key/value overflow) of the newly created record;
403-
// balances the decrement emitted by GarnetRecordTriggers.OnEvict on page eviction.
404-
var heap = logRecord.CalculateHeapMemorySize();
402+
// Account for value-side overflow heap of the newly created record.
403+
// Key overflow is tracked internally by Tsavorite via logSizeTracker.
404+
var heap = logRecord.GetValueHeapMemorySize();
405405
if (heap != 0)
406406
functionsState.cacheSizeTracker?.AddHeapSize(heap);
407407
}
@@ -1712,15 +1712,9 @@ public readonly bool PostCopyUpdater<TSourceLogRecord>(in TSourceLogRecord srcLo
17121712
if (functionsState.appendOnlyFile != null)
17131713
rmwInfo.UserData |= NeedAofLog; // Mark that we need to write to AOF
17141714

1715-
// Account for the new record's heap contribution so that the matching OnEvict decrement
1716-
// balances. We do NOT subtract srcLogRecord's heap: the source may be a main-log record
1717-
// (whose heap is tracked in mainLogTracker and will leak upward only by a bounded sealed-
1718-
// source amount — parity with ObjectStore's ClearSource=false branch and the legacy
1719-
// SubscribeEvictions path), a read-cache record (heap lives in readCacheTracker, unrelated
1720-
// to this write), or a pending-IO DiskLogRecord (heap never counted in any tracker).
1721-
// Subtracting unconditionally would undercount in the last two cases and could drive the
1722-
// counter negative.
1723-
var newHeap = dstLogRecord.CalculateHeapMemorySize();
1715+
// Account for the new record's value-side heap contribution.
1716+
// Key overflow is tracked internally by Tsavorite via logSizeTracker.
1717+
var newHeap = dstLogRecord.GetValueHeapMemorySize();
17241718
if (newHeap != 0)
17251719
functionsState.cacheSizeTracker?.AddHeapSize(newHeap);
17261720
return true;

‎libs/server/Storage/Functions/MainStore/UpsertMethods.cs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -46,9 +46,9 @@ public void PostInitialWriter(ref LogRecord logRecord, in RecordSizeInfo sizeInf
4646
if (functionsState.appendOnlyFile != null)
4747
upsertInfo.UserData |= NeedAofLog; // Mark that we need to write to AOF
4848

49-
// Account for overflow key/value heap on the freshly-inserted record so the matching
50-
// OnEvict decrement balances.
51-
var heap = logRecord.CalculateHeapMemorySize();
49+
// Account for value-side overflow heap on the freshly-inserted record.
50+
// Key overflow is tracked internally by Tsavorite via logSizeTracker.
51+
var heap = logRecord.GetValueHeapMemorySize();
5252
if (heap != 0)
5353
functionsState.cacheSizeTracker?.AddHeapSize(heap);
5454
}
@@ -68,7 +68,7 @@ public void PostInitialWriter<TSourceLogRecord>(ref LogRecord logRecord, in Reco
6868
upsertInfo.UserData |= NeedAofLog; // Mark that we need to write to AOF
6969
}
7070

71-
var heap = logRecord.CalculateHeapMemorySize();
71+
var heap = logRecord.GetValueHeapMemorySize();
7272
if (heap != 0)
7373
functionsState.cacheSizeTracker?.AddHeapSize(heap);
7474
}

‎libs/server/Storage/Functions/ObjectStore/UpsertMethods.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ public void PostInitialWriter(ref LogRecord logRecord, in RecordSizeInfo sizeInf
5050
upsertInfo.UserData |= NeedAofLog; // Mark that we need to write to AOF
5151

5252
if (logRecord.Info.RecordHasObjects)
53-
functionsState.cacheSizeTracker?.AddHeapSize(MemoryUtils.CalculateHeapMemorySize(in logRecord));
53+
functionsState.cacheSizeTracker?.AddHeapSize(logRecord.GetValueHeapMemorySize());
5454
}
5555

5656
/// <inheritdoc />
@@ -62,7 +62,7 @@ public void PostInitialWriter(ref LogRecord logRecord, in RecordSizeInfo sizeInf
6262
upsertInfo.UserData |= NeedAofLog; // Mark that we need to write to AOF
6363

6464
if (logRecord.Info.RecordHasObjects)
65-
functionsState.cacheSizeTracker?.AddHeapSize(MemoryUtils.CalculateHeapMemorySize(in logRecord));
65+
functionsState.cacheSizeTracker?.AddHeapSize(logRecord.GetValueHeapMemorySize());
6666
}
6767

6868
/// <inheritdoc />
@@ -75,7 +75,7 @@ public void PostInitialWriter<TSourceLogRecord>(ref LogRecord logRecord, in Reco
7575
upsertInfo.UserData |= NeedAofLog; // Mark that we need to write to AOF
7676
}
7777
if (logRecord.Info.RecordHasObjects)
78-
functionsState.cacheSizeTracker?.AddHeapSize(MemoryUtils.CalculateHeapMemorySize(in logRecord));
78+
functionsState.cacheSizeTracker?.AddHeapSize(logRecord.GetValueHeapMemorySize());
7979
}
8080

8181
/// <inheritdoc />

‎libs/server/Storage/Functions/UnifiedStore/UpsertMethods.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ public void PostInitialWriter(ref LogRecord logRecord, in RecordSizeInfo sizeInf
6666
upsertInfo.UserData |= NeedAofLog; // Mark that we need to write to AOF
6767

6868
if (logRecord.Info.RecordHasObjects)
69-
functionsState.cacheSizeTracker?.AddHeapSize(MemoryUtils.CalculateHeapMemorySize(in logRecord));
69+
functionsState.cacheSizeTracker?.AddHeapSize(logRecord.GetValueHeapMemorySize());
7070
}
7171

7272
/// <inheritdoc />
@@ -79,7 +79,7 @@ public void PostInitialWriter(ref LogRecord logRecord, in RecordSizeInfo sizeInf
7979
upsertInfo.UserData |= NeedAofLog; // Mark that we need to write to AOF
8080

8181
if (logRecord.Info.RecordHasObjects)
82-
functionsState.cacheSizeTracker?.AddHeapSize(MemoryUtils.CalculateHeapMemorySize(in logRecord));
82+
functionsState.cacheSizeTracker?.AddHeapSize(logRecord.GetValueHeapMemorySize());
8383
}
8484

8585
/// <inheritdoc />
@@ -92,7 +92,7 @@ public void PostInitialWriter<TSourceLogRecord>(ref LogRecord logRecord, in Reco
9292
upsertInfo.UserData |= NeedAofLog; // Mark that we need to write to AOF
9393

9494
if (logRecord.Info.RecordHasObjects)
95-
functionsState.cacheSizeTracker?.AddHeapSize(MemoryUtils.CalculateHeapMemorySize(in logRecord));
95+
functionsState.cacheSizeTracker?.AddHeapSize(logRecord.GetValueHeapMemorySize());
9696
}
9797

9898
/// <inheritdoc />

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

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1419,21 +1419,19 @@ internal void EvictPageForRecovery(long page)
14191419
var end = GetLogicalAddressOfStartOfPage(page + 1);
14201420

14211421
var source = IsReadCache ? EvictionSource.ReadCache : EvictionSource.MainLog;
1422-
if (storeFunctions.CallOnEvict(source))
1422+
1423+
// Per-record eviction walk handles internal heap accounting (key + value via
1424+
// logSizeTracker) and optionally notifies the application via OnEvict.
1425+
if (logSizeTracker is not null || storeFunctions.CallOnEvict(source))
14231426
{
1424-
// New per-record path: OnEvict handles heap-size decrement per record. Parity
1425-
// with the runtime eviction path in OnPagesClosedWorker.
14261427
_wrapper.EvictRecordsInRange(start, end, source);
14271428
}
1428-
else if (logSizeTracker is not null)
1429+
else if (onEvictionObserver is not null)
14291430
{
1430-
// Legacy observer path: materialize an iterator and push heap-size decrement to
1431-
// the LogSizeTracker.OnNext observer (kept for consumers still using SubscribeEvictions).
1432-
MemoryPageScan(start, end, logSizeTracker);
1431+
// Legacy observer path for consumers still using SubscribeEvictions.
1432+
MemoryPageScan(start, end, onEvictionObserver);
14331433
}
14341434

1435-
// TODO: Currently we don't call OnDispose or OnDisposeValueObject on eviction; we defer to the OnEvictionObserver
1436-
// and do nothing if that is not supplied. Should we add our own observer if they don't supply one?
14371435
_wrapper.FreePage(page);
14381436
}
14391437

@@ -1510,9 +1508,11 @@ private void OnPagesClosedWorker()
15101508
if (onEvictionObserver is not null)
15111509
MemoryPageScan(start, end, onEvictionObserver);
15121510

1513-
// Notify application of records being evicted — allows cleanup of external resources.
1511+
// Per-record eviction walk: handles internal heap-size accounting (key overflow
1512+
// and value heap via logSizeTracker) and optionally notifies the application
1513+
// via OnEvict for app-level cleanup.
15141514
var evictSource = IsReadCache ? EvictionSource.ReadCache : EvictionSource.MainLog;
1515-
if (storeFunctions.CallOnEvict(evictSource))
1515+
if (logSizeTracker is not null || storeFunctions.CallOnEvict(evictSource))
15161516
_wrapper.EvictRecordsInRange(start, end, evictSource);
15171517

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

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

Lines changed: 31 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -328,6 +328,16 @@ internal void OnDispose(ref LogRecord logRecord, DisposeReason disposeReason)
328328
{
329329
if (logRecord.IsSet)
330330
{
331+
// For Deleted records, decrement the value-side heap from the tracker before clearing.
332+
// Key overflow is handled at eviction time in EvictRecordsInRange. Tombstone has not
333+
// been set yet when this is called, so GetValueHeapMemorySize returns the correct value.
334+
if (disposeReason == DisposeReason.Deleted)
335+
{
336+
var valueHeap = logRecord.GetValueHeapMemorySize();
337+
if (valueHeap != 0)
338+
logSizeTracker?.IncrementSize(-valueHeap);
339+
}
340+
331341
storeFunctions.OnDispose(ref logRecord, disposeReason);
332342

333343
logRecord.ClearHeapFields(disposeReason != DisposeReason.Deleted);
@@ -354,8 +364,6 @@ internal void OnDisposeDiskRecord(ref DiskLogRecord logRecord, DisposeReason dis
354364
/// </summary>
355365
internal void EvictRecordsInRange(long startAddress, long endAddress, EvictionSource source)
356366
{
357-
// Clip to a single page: we don't cross page boundaries in one call, but be defensive in case the caller
358-
// ever passes a multi-page range. The start-page PageHeader is skipped via GetFirstValidLogicalAddressOnPage.
359367
var startPage = GetPage(startAddress);
360368
var firstValidAddress = GetFirstValidLogicalAddressOnPage(startPage);
361369
var address = startAddress < firstValidAddress ? firstValidAddress : startAddress;
@@ -368,35 +376,38 @@ internal void EvictRecordsInRange(long startAddress, long endAddress, EvictionSo
368376
var logRecord = new LogRecord(physicalAddress, objectPages[GetPageIndexForAddress(address)].objectIdMap);
369377
var allocatedSize = logRecord.AllocatedSize;
370378

371-
// Guard against corrupt / zero-size records causing infinite loops. Also stop if the record would
372-
// straddle the end of the page; ObjectScanIterator skips to the next page in that case, but our caller
373-
// constrains the range to a single page so there is nothing further to visit here.
374379
if (allocatedSize <= 0)
375380
break;
376381
var offset = GetOffsetOnPage(address);
377382
if (offset == 0 || offset + allocatedSize > PageSize)
378383
break;
379384

380-
// Skip null, invalid, and tombstoned records:
381-
// - IsNull: empty record slot, nothing to account for.
382-
// - Invalid: record was elided from the hash chain and already disposed
383-
// (OnDispose(Deleted/Elided) or transferred to the revivification freelist).
384-
// - Tombstone: heap was already decremented at the delete site via
385-
// OnDispose(Deleted).
386-
//
387-
// Sealed-but-Valid records are NOT skipped. A Sealed source record from a
388-
// mutable-region CopyUpdate still owns its overflow key/value bytes (and
389-
// possibly its value object if a checkpoint prevented eager clearing via
390-
// ClearSourceValueObject). Skipping them would leak their heap contribution
391-
// in the tracker.
392-
if (logRecord.Info.IsNull || logRecord.Info.Invalid || logRecord.Info.Tombstone)
385+
// Skip null and invalid records (elided/disposed, heap already cleaned up).
386+
if (logRecord.Info.IsNull || logRecord.Info.Invalid)
393387
{
394388
address += allocatedSize;
395389
continue;
396390
}
397391

398-
// Notify the application that this record is being evicted from memory.
399-
storeFunctions.OnEvict(ref logRecord, source);
392+
// Decrement the record's heap contribution in a single call.
393+
// For non-tombstoned records, CalculateHeapMemorySize returns key + value heap.
394+
// For tombstoned records, it returns 0 (by design), but the key overflow is still
395+
// alive — value was already decremented at the delete site via OnDispose.
396+
long heapSize;
397+
if (!logRecord.Info.Tombstone)
398+
{
399+
heapSize = logRecord.CalculateHeapMemorySize();
400+
401+
if (storeFunctions.CallOnEvict(source))
402+
storeFunctions.OnEvict(ref logRecord, source);
403+
}
404+
else
405+
{
406+
heapSize = logRecord.Info.KeyIsOverflow ? logRecord.KeyOverflow.HeapMemorySize : 0;
407+
}
408+
409+
if (heapSize != 0)
410+
logSizeTracker?.IncrementSize(-heapSize);
400411

401412
address += allocatedSize;
402413
}

0 commit comments

Comments
 (0)