Adjust v2 defaults#1805
Merged
Merged
Conversation
…ueOverflowThreshold=16k Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Addresses Copilot review comments on PR #1805: - ImportExportConfigLocal/Azure: default PageSize now "4m" (was "32m"); override changed to "8m" so the test continues to verify non-default tracking now that "4m" matches the default. - ValueOverflowThresholdParsing: default ValueOverflowThreshold now "16k" (16384 bytes), was "4k" (4096 bytes). Co-authored-by: Copilot <[email protected]>
TedHartMS
approved these changes
May 16, 2026
tiagonapoli
approved these changes
May 16, 2026
After a more critical review, 4 MB is hard to defend on a typical 64 GB log: - The flush-burst, ReadOnly-granularity, and blast-radius rationale is weak or wrong-directional (smaller pages age records into immutable faster, causing MORE RCU, not less). - pagePointers at 4 MB / 64 GB log = 128 KB, which spills out of L1d on every modern server CPU (32-48 KB L1d). - The YCSB result was on an in-memory dataset with no flush/eviction activity and was within noise across iterations. 16 MB is a defensible compromise: - Halves flush-completion latency and tail-page waste vs 32 MB. - 2x finer eviction granularity. - pagePointers (32 KB on 64 GB log) still fits L1d. - Only doubles per-page metadata vs 32 MB instead of 8x. ReadCachePageSize=1m and ValueOverflowThreshold=16k unchanged (those wins are clean and well-grounded). Co-authored-by: Copilot <[email protected]>
With a 1 GB ReadCacheMemorySize default and byte-granular eviction via LogSizeTracker, 256 read-cache pages (4 MB each) already provide ample eviction granularity (~0.4% memory step per page free) while halving per-page metadata (ObjectIdMap, PageStatusIndicator) and reducing page-allocation churn during cache fills. 1 MB was over-conservative. Co-authored-by: Copilot <[email protected]>
TedHartMS
approved these changes
May 17, 2026
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.
Updates Garnet's v2 defaults to better reflect typical operational deployments and the vector-database workload.
Rationale
PageSize = 16m(was32m)A modest 2× reduction. Earlier drafts proposed 4 MB; after a critical re-examination (see below), 16 MB is the more defensible compromise.
Real benefits over 32 MB:
FlushedUntilAddress, which gates replication acks and AOF commit cadence.HeadAddressstep →LogSizeTrackerkeeps memory closer to target on tight budgets.Cache behavior is preserved. For a 64 GB typical deployment:
pagePointers16 KB → L1d.pagePointers32 KB → still L1d on every mainstream server CPU.Costs are modest:
PageOffsetCAS /HandlePageOverflowinvocations).PageStatusIndicator,objectPages, etc.).ReadCachePageSize = 4m(was32m)The previous 32 MB default was clearly broken for the read cache; 4 MB is the right operational point.
ReadCacheMemorySizedefault and 32 MB pages, there are only ~32 pages total — too coarse for any useful eviction or second-chance behavior. 4 MB gives 256 pages, which is plenty.LogSizeTracker.DetermineEvictionRangeevicts at record granularity; the page-size choice only governs when the underlying page buffer is freed back to the allocator. At 256 pages on a 1 GB budget, each freed page is a 0.4% memory step — comfortably smooth.ObjectIdMap,PageStatusIndicator) and 4× the page-allocation churn during cache fills. 4 MB captures the win at a fraction of the overhead and matches the 4:1 ratio with the 16 MB main-log page.NullDevice— there is no flush IO whose cost grows with page count, so smaller pages are essentially free on that axis.ValueOverflowThreshold = 16k(was4k)Critical for the vector-database use case.
IStreamBuffer.InitialIOSizeto be hinted up to the record size so the first IO grabs the entire inline record; without that hint the OS-page-sized initial read (4 KB on Linux x64) wastes one IO for inline records > 4 KB. That fix is straightforward and orthogonal.Why not 4 MB? (notes from review)
Earlier iterations proposed
PageSize = 4m. Critical review concluded the operational arguments for going that small don't hold up:FlushedUntilAddresstick frequency, which is a marginal effect.ReadOnlyAddressgranularity" — Actually a negative. Records age into immutable 8× faster, causing more RCU traffic on writes.pagePointersfor 4 MB / 64 GB log is 128 KB, which spills out of L1d on every modern server CPU.16 MB captures the meaningful operational wins without these downsides.
Testing
GarnetServerConfigTests(ImportExportConfigLocal,ImportExportConfigAzure,ValueOverflowThresholdParsing) had hard-coded"32m"and"4k"assertions; updated to the new defaults. Affected tests pass locally.