Replace node_size with memory_limit - #7965
Conversation
| } | ||
|
|
||
| if (!section.exists(Keys::kFilterBits) && (config.nodeSize >= 2)) | ||
| if (!section.exists(Keys::kFilterBits)) |
There was a problem hiding this comment.
Bloom filter unconditionally enabled for minimal-budget configs. Re-gate on budget:
| if (!section.exists(Keys::kFilterBits)) | |
| if (!section.exists(Keys::kFilterBits) && config.cacheMemoryBudget() != 0) |
There was a problem hiding this comment.
✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.
There was a problem hiding this comment.
✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.
| return std::clamp(gb * 2, 4, 128); | ||
| case SizedItem::BurstSize: | ||
| return std::clamp(gb, 4, 48); | ||
| case SizedItem::AccountIdCacheSize: |
There was a problem hiding this comment.
AccountIdCacheSize hardcoded to 300007 ignores new budget. Scale like other caches:
| case SizedItem::AccountIdCacheSize: | |
| return std::clamp(gb * 2344, 20047, 300007); |
There was a problem hiding this comment.
✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.
There was a problem hiding this comment.
✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.
| return std::clamp(gb * 2, 4, 128); | ||
| case SizedItem::BurstSize: | ||
| return std::clamp(gb, 4, 48); | ||
| case SizedItem::AccountIdCacheSize: |
There was a problem hiding this comment.
AccountIdCacheSize hardcoded to 300007 while other db caches scale with budget—document why or scale it:
case SizedItem::AccountIdCacheSize:
// Fixed regardless of budget: account ID cache is not
// memory-significant and uses a documented policy value.
return 300007;
There was a problem hiding this comment.
✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.
There was a problem hiding this comment.
Done, kept it fixed with the comment. It's ~22 MB at most and the value stays prime for hash distribution. resolved: f80e72c6f7
There was a problem hiding this comment.
✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.
| } | ||
|
|
||
| if (!section.exists(Keys::kFilterBits) && (config.nodeSize >= 2)) | ||
| if (!section.exists(Keys::kFilterBits)) |
There was a problem hiding this comment.
Bloom filter now enabled unconditionally for all configs, increasing memory in minimal deployments. Gate on cacheMemoryBudget():
| if (!section.exists(Keys::kFilterBits)) | |
| if (!section.exists(Keys::kFilterBits) && config.cacheMemoryBudget() != 0) |
There was a problem hiding this comment.
Done, gated on a nonzero budget. resolved: f80e72c6f7
There was a problem hiding this comment.
✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.
| : 0)) | ||
| { | ||
| auto const budget = app.config().cacheMemoryBudget(); | ||
| JLOG(j_.warn()) << "TreeNodeCache sizing: target=" |
There was a problem hiding this comment.
Normal startup log at warn level will trigger monitoring alerts. Use j_.info() for routine messages:
| JLOG(j_.warn()) << "TreeNodeCache sizing: target=" | |
| JLOG(j_.info()) << "TreeNodeCache sizing: target=" |
There was a problem hiding this comment.
Done, moved to info. resolved: f80e72c6f7
There was a problem hiding this comment.
✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
| } | ||
|
|
||
| if (!section.exists(Keys::kFilterBits) && (config.nodeSize >= 2)) | ||
| if (!section.exists(Keys::kFilterBits) && config.cacheMemoryBudget() != 0) |
There was a problem hiding this comment.
Bloom filter bits enabled for all budgets (was medium+ tiers only). Likely unintended scope expansion for a memory-reduction PR — gate on threshold:
| if (!section.exists(Keys::kFilterBits) && config.cacheMemoryBudget() != 0) | |
| if (!section.exists(Keys::kFilterBits) && config.cacheMemoryBudget() >= (16ull << 30)) |
High Level Overview of Change
Replaces
[node_size]with[memory_limit]: the memory budget, in gigabytes, that the server sizes its caches within. When it is not set, the budget defaults to detected physical RAM, and on Linux the detection respects the container or service cgroup limit. Setting 0 selects minimal sizes with no enforcement.[node_size]still parses as a deprecated alias (tiny/small/medium/large/huge map to 4/8/32/64/128 GB) and warns at startup, so existing configs keep working.Context of Change
node_size has served for a long time, but its five presets are entry counts, which are hard to reason about in bytes, and the targets are advisory. During capacity testing our 64 GB nodes ran out of memory at about 2.5M accounts on the "huge" preset, while the same nodes on "large" carried the same load at 2.3 GiB RSS. That test is the calibration for this change: a 64 GB budget produces the "large" sizing that held up, and 128 GB produces the old "huge". The model is the one Redis uses with maxmemory, a byte budget the process keeps.
The change:
tree_cache_age,ledger_cache_age,ledger_fetch_size).TaggedCachegains an opt-in hard cap that demotes the approximately oldest entry whenever growth would exceed it, on fresh inserts and weak-to-strong revivals, with bounded work per call. The sweep remains the primary evictor, and saturation is logged.[workers]and[io_workers]still override.[node_db]cache_sizeandcache_agenow default to the documented 16384 records and 5 minutes. They previously inherited the tree cache values, and the age was read in the wrong unit, which gave a "huge" node a second 8.4M-entry cache with 15-hour retention.server_inforeportsmemory_limitin GB instead ofnode_size(API-CHANGELOG updated), and the unusedSizedItem::OpenFinalLimitandRamSizeGbentries are removed.API Impact
libxrplchange: new defaultedTaggedCacheconstructor parameter;Config::getValueForloses its node-size override parameter; two deadSizedItementries removed.server_infodropsnode_size, addsmemory_limit(see API-CHANGELOG.md).Test Plan
Config tests cover parsing (gigabytes, 0, garbage and values over 1024 rejected), the budget-derived values, standalone behavior, and the alias forms (names, the legacy 0-4 index, and
[memory_limit]winning when both are set). GTest covers the hard cap holding through a 1000-insert burst with the sweep unable to fire, and cap 0 disabling it. The full unit test suite passes.Future Tasks
[node_size]alias at the end of the deprecation period.