Commit 695d6ab
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`).
- 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>1 parent 0740ff9 commit 695d6ab
File tree
16 files changed
+141
-37
lines changed- libs
- server/Storage
- Functions
- MainStore
- SizeTracker
- storage/Tsavorite/cs/src/core
- Allocator
- Index
- StoreFunctions
- Tsavorite
16 files changed
+141
-37
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
7 | 7 | | |
8 | 8 | | |
9 | 9 | | |
10 | | - | |
| 10 | + | |
| 11 | + | |
11 | 12 | | |
12 | 13 | | |
13 | 14 | | |
14 | 15 | | |
15 | | - | |
| 16 | + | |
16 | 17 | | |
17 | 18 | | |
18 | 19 | | |
| |||
29 | 30 | | |
30 | 31 | | |
31 | 32 | | |
32 | | - | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
33 | 40 | | |
34 | 41 | | |
35 | 42 | | |
| |||
49 | 56 | | |
50 | 57 | | |
51 | 58 | | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
52 | 80 | | |
53 | 81 | | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
398 | 398 | | |
399 | 399 | | |
400 | 400 | | |
| 401 | + | |
| 402 | + | |
| 403 | + | |
| 404 | + | |
| 405 | + | |
| 406 | + | |
401 | 407 | | |
402 | 408 | | |
403 | 409 | | |
| |||
409 | 415 | | |
410 | 416 | | |
411 | 417 | | |
| 418 | + | |
412 | 419 | | |
413 | 420 | | |
414 | 421 | | |
| |||
419 | 426 | | |
420 | 427 | | |
421 | 428 | | |
| 429 | + | |
| 430 | + | |
| 431 | + | |
422 | 432 | | |
423 | 433 | | |
424 | 434 | | |
| |||
1701 | 1711 | | |
1702 | 1712 | | |
1703 | 1713 | | |
| 1714 | + | |
| 1715 | + | |
| 1716 | + | |
| 1717 | + | |
| 1718 | + | |
| 1719 | + | |
| 1720 | + | |
1704 | 1721 | | |
1705 | 1722 | | |
1706 | 1723 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
65 | 65 | | |
66 | 66 | | |
67 | 67 | | |
68 | | - | |
69 | | - | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
70 | 71 | | |
71 | 72 | | |
72 | 73 | | |
| |||
77 | 78 | | |
78 | 79 | | |
79 | 80 | | |
80 | | - | |
| 81 | + | |
81 | 82 | | |
82 | 83 | | |
83 | 84 | | |
84 | 85 | | |
85 | 86 | | |
86 | 87 | | |
87 | | - | |
| 88 | + | |
88 | 89 | | |
89 | 90 | | |
90 | 91 | | |
| |||
Lines changed: 27 additions & 6 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
205 | 205 | | |
206 | 206 | | |
207 | 207 | | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
208 | 214 | | |
209 | 215 | | |
210 | 216 | | |
| |||
553 | 559 | | |
554 | 560 | | |
555 | 561 | | |
556 | | - | |
557 | | - | |
| 562 | + | |
| 563 | + | |
558 | 564 | | |
| 565 | + | |
| 566 | + | |
| 567 | + | |
| 568 | + | |
| 569 | + | |
| 570 | + | |
559 | 571 | | |
560 | 572 | | |
561 | 573 | | |
| |||
1403 | 1415 | | |
1404 | 1416 | | |
1405 | 1417 | | |
1406 | | - | |
| 1418 | + | |
| 1419 | + | |
| 1420 | + | |
| 1421 | + | |
| 1422 | + | |
| 1423 | + | |
| 1424 | + | |
| 1425 | + | |
| 1426 | + | |
| 1427 | + | |
1407 | 1428 | | |
1408 | | - | |
1409 | | - | |
| 1429 | + | |
| 1430 | + | |
1410 | 1431 | | |
1411 | 1432 | | |
1412 | 1433 | | |
| |||
1490 | 1511 | | |
1491 | 1512 | | |
1492 | 1513 | | |
1493 | | - | |
| 1514 | + | |
1494 | 1515 | | |
1495 | 1516 | | |
1496 | 1517 | | |
| |||
Lines changed: 6 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
23 | 23 | | |
24 | 24 | | |
25 | 25 | | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
26 | 32 | | |
27 | 33 | | |
28 | 34 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
117 | 117 | | |
118 | 118 | | |
119 | 119 | | |
120 | | - | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
121 | 124 | | |
122 | 125 | | |
Lines changed: 1 addition & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
146 | 146 | | |
147 | 147 | | |
148 | 148 | | |
149 | | - | |
| 149 | + | |
150 | 150 | | |
151 | 151 | | |
Lines changed: 23 additions & 13 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
74 | 74 | | |
75 | 75 | | |
76 | 76 | | |
77 | | - | |
| 77 | + | |
78 | 78 | | |
79 | 79 | | |
80 | 80 | | |
| |||
346 | 346 | | |
347 | 347 | | |
348 | 348 | | |
| 349 | + | |
| 350 | + | |
| 351 | + | |
| 352 | + | |
| 353 | + | |
349 | 354 | | |
350 | | - | |
| 355 | + | |
351 | 356 | | |
352 | | - | |
353 | | - | |
354 | | - | |
| 357 | + | |
| 358 | + | |
| 359 | + | |
| 360 | + | |
355 | 361 | | |
| 362 | + | |
| 363 | + | |
356 | 364 | | |
357 | | - | |
| 365 | + | |
358 | 366 | | |
359 | 367 | | |
360 | 368 | | |
361 | 369 | | |
362 | 370 | | |
363 | | - | |
| 371 | + | |
| 372 | + | |
| 373 | + | |
364 | 374 | | |
365 | 375 | | |
366 | | - | |
367 | | - | |
368 | 376 | | |
369 | | - | |
| 377 | + | |
370 | 378 | | |
371 | 379 | | |
372 | | - | |
373 | | - | |
| 380 | + | |
| 381 | + | |
| 382 | + | |
| 383 | + | |
374 | 384 | | |
375 | 385 | | |
376 | 386 | | |
377 | 387 | | |
378 | 388 | | |
379 | 389 | | |
380 | 390 | | |
381 | | - | |
| 391 | + | |
382 | 392 | | |
383 | 393 | | |
384 | 394 | | |
| |||
Lines changed: 1 addition & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
145 | 145 | | |
146 | 146 | | |
147 | 147 | | |
148 | | - | |
| 148 | + | |
149 | 149 | | |
150 | 150 | | |
Lines changed: 1 addition & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
15 | 15 | | |
16 | 16 | | |
17 | 17 | | |
18 | | - | |
| 18 | + | |
19 | 19 | | |
20 | 20 | | |
21 | 21 | | |
| |||
0 commit comments