Commit 7c62bf1
committed
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>
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.1 parent 0740ff9 commit 7c62bf1
16 files changed
Lines changed: 150 additions & 37 deletions
File tree
- libs
- server/Storage
- Functions
- SizeTracker
- storage/Tsavorite/cs/src/core
- Allocator
- Index
- Common
- StoreFunctions
- Tsavorite
| 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 | + | |
52 | 74 | | |
53 | 75 | | |
| 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 | | |
| |||
118 | 119 | | |
119 | 120 | | |
120 | 121 | | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
121 | 141 | | |
122 | 142 | | |
123 | 143 | | |
| |||
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 | | |
| |||
Lines changed: 1 addition & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
144 | 144 | | |
145 | 145 | | |
146 | 146 | | |
147 | | - | |
| 147 | + | |
148 | 148 | | |
149 | 149 | | |
0 commit comments