perf(cmd): scale db-cache-size to available memory - #3927
perf(cmd): scale db-cache-size to available memory#3927infrmtcs-agent[bot] wants to merge 1 commit into
Conversation
|
Claude finished @infrmtcs-agent[bot]'s task in 6m 22s —— View job Review:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| func dbCacheSizeForMemoryMB(memMB uint64) uint { | ||
| return min(max(uint(memMB/cacheMemoryFraction), defaultCacheSizeMb), maxAutoCacheSizeMb) | ||
| } |
There was a problem hiding this comment.
important — the lower clamp isn't memory-aware, which is the one case where being memory-aware matters most.
For any memMB < 4096 the result is pinned at defaultCacheSizeMb (1024), so:
| container limit | resulting cache | share of memory |
|---|---|---|
| 1024 MB | 1024 MB | 100% |
| 2048 MB | 1024 MB | 50% |
| 4096 MB | 1024 MB | 25% |
A 1–2 GB cgroup (small k8s pods, CI containers) gets a block cache equal to half or all of its limit, and since pebble.NewCache is the ceiling the process grows into it, this ends in an OOM kill. The pre-PR fixed default had the same flaw, but this function is exactly where it should be fixed — otherwise the flag advertises "scales to available memory" while ignoring memory below 4 GB.
Consider bounding the floor too:
func dbCacheSizeForMemoryMB(memMB uint64) uint {
quarter := uint(memMB / cacheMemoryFraction)
return min(max(quarter, min(defaultCacheSizeMb, uint(memMB))), maxAutoCacheSizeMb)
}or, if the 1024 floor is deliberate, say so in the comment and log a warning when the floor exceeds a quarter of detected memory so operators of small containers see it.
Separately: defaultCacheSizeMb is no longer the default, only the floor — minAutoCacheSizeMb would pair with maxAutoCacheSizeMb and keep the doc comment on line 720 self-explanatory.
| defaultRPCMaxRequestQueue := uint(256000) | ||
| defaultRPCMaxBlockScan := uint(math.MaxUint) | ||
| defaultMaxCacheSize := uint(1024) | ||
| defaultMaxCacheSize := min(max(uint(utils.AvailableMemoryMB()/4), 1024), 8192) |
There was a problem hiding this comment.
nit — this re-implements dbCacheSizeForMemoryMB with hardcoded literals (4, 1024, 8192) in an external test package, so the clamp constants now live in three places (juno.go, here, generate-config.js). Changing cacheMemoryFraction breaks this test for the wrong reason. defaultMaxHandles on the next line has the same issue and is pre-existing, so if you'd rather not export a helper just for tests it's fine to leave — but a short comment pointing at dbCacheSizeForMemoryMB as the source of truth would help the next person.
| wantConc: 1, | ||
| }, | ||
| { | ||
| name: "explicit setting bypasses derivation", | ||
| cfg: Config{ | ||
| MaxConcurrentCompilations: 5, MaxConcurrentCompilationsExplicit: true, | ||
| MaxCompilationMemory: 4096, NodeMemoryReserve: 4096, DBCacheSize: 8192, | ||
| }, | ||
| cores: 64, | ||
| memMB: 65536, | ||
| wantConc: 5, | ||
| }, |
There was a problem hiding this comment.
nit — good table, two gaps worth one row each:
MaxCompilationQueueExplicit: true— the queue branch is never exercised with an explicit value, soqueueSize = cfg.MaxCompilationQueueis untested here.MaxCompilationMemory: 0— the "no memory limit" path returnsmax(1, cores)and the DB-cache reserve becomes irrelevant; a row assertingwantConc == coreswould lock in that the cache subtraction doesn't leak into it.
| // AvailableMemoryMB returns the RAM this process can use, in MB. | ||
| // It checks if the memory is limited by the cgroup limit, otherwise it uses the host RAM. | ||
| func AvailableMemoryMB() uint64 { | ||
| hostMemory := memory.TotalMemory() | ||
| cgroupLimit, err := memlimit.FromCgroup() | ||
| if err == nil && cgroupLimit > 0 && cgroupLimit < hostMemory { | ||
| return cgroupLimit / megabyte | ||
| } | ||
| return hostMemory / megabyte |
There was a problem hiding this comment.
nit — the move to utils is clean (and dropping compiler's dependency on it is the right call), but note this returns total RAM (memory.TotalMemory()) or the cgroup limit, not free memory. That was fine when only the compiler budget used it; now it also sizes a real allocation that the process grows into, and the user-facing docs say "a quarter of available memory", which reads as free memory. Worth either a one-line comment here clarifying total-vs-free, or wording the docs as "a quarter of total/cgroup-limited memory".
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3927 +/- ##
==========================================
- Coverage 77.44% 74.63% -2.82%
==========================================
Files 163 462 +299
Lines 19703 40871 +21168
==========================================
+ Hits 15259 30504 +15245
- Misses 3436 8266 +4830
- Partials 1008 2101 +1093
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
0745361 to
0823d18
Compare
|
Claude finished @infrmtcs's task in 5m 21s —— View job Review:
|
| } | ||
|
|
||
| func dbCacheSizeForMemoryMB(memMB uint64) uint { | ||
| return min(max(uint(memMB/cacheMemoryFraction), defaultCacheSizeMb), maxAutoCacheSizeMb) |
There was a problem hiding this comment.
important (re-anchored after the rebase — the original comment was marked outdated) — the lower clamp isn't memory-aware, which is the one range where being memory-aware matters most.
For any memMB < 4096 the result is pinned at defaultCacheSizeMb (1024):
| detected memory | resulting cache | share |
|---|---|---|
| 1024 MB | 1024 MB | 100% |
| 2048 MB | 1024 MB | 50% |
| 4096 MB | 1024 MB | 25% |
pebble.NewCache is a ceiling the process grows into, so a 1–2 GB cgroup (small k8s pods, CI containers) ends up with a block cache at half or all of its limit. This is not a regression — the fixed 1024 default had the same property — but this function is now the thing that claims to scale with memory, so it's the right place to close the gap:
| return min(max(uint(memMB/cacheMemoryFraction), defaultCacheSizeMb), maxAutoCacheSizeMb) | |
| quarter := uint(memMB / cacheMemoryFraction) | |
| return min(max(quarter, min(defaultCacheSizeMb, uint(memMB))), maxAutoCacheSizeMb) |
If the flat 1024 floor is deliberate, saying so in the doc comment above (and logging a warning when the floor exceeds a quarter of detected memory) would at least make it visible to operators of small containers.
Separately: defaultCacheSizeMb is no longer the default, only the floor — minAutoCacheSizeMb would pair with maxAutoCacheSizeMb and keep the doc comment on line 721 self-explanatory.
0823d18 to
386db67
Compare
|
Claude finished @infrmtcs's task in 5m 19s —— View job Review:
|
| func dbCacheSizeForMemoryMB(memMB uint64) uint { | ||
| return min(max(uint(memMB/cacheMemoryFraction), defaultCacheSizeMb), maxAutoCacheSizeMb) | ||
| } |
There was a problem hiding this comment.
important (standing item, re-anchored after this rebase) — and a correction to the fix I suggested earlier: the snippet I proposed in my previous review does not actually fix the case it describes. Please ignore it.
The problem stands: for any memMB < 4096 the result is pinned at defaultCacheSizeMb (1024), so a 1 GB cgroup gets a block cache equal to 100% of its limit and a 2 GB cgroup 50%. pebble.NewCache is a ceiling the process grows into, so small k8s pods / CI containers are the ones most likely to OOM. Not a regression (the fixed 1024 default had the same property), but this function is now what claims to scale with memory.
Why my earlier suggestion was wrong — min(max(quarter, min(defaultCacheSizeMb, uint(memMB))), maxAutoCacheSizeMb):
| memMB | quarter | min(1024, memMB) |
result | share |
|---|---|---|---|---|
| 1024 | 256 | 1024 | 1024 | 100% |
| 2048 | 512 | 1024 | 1024 | 50% |
min(defaultCacheSizeMb, memMB) only bites below 1024 MB of total memory, so both rows in my own table come out unchanged. Bounding the floor by memory isn't enough — it has to be bounded by a fraction of memory, i.e. the floor must not be allowed to exceed what the quarter rule would grant. Concretely, either drop the floor when memory can't afford it:
// dbCacheSizeForMemoryMB gives the cache a quarter of memMB, capped at
// maxAutoCacheSizeMb. The minAutoCacheSizeMb floor only applies when memory is
// large enough for it to stay within the quarter budget, so a small container
// does not get a cache the size of its whole limit.
func dbCacheSizeForMemoryMB(memMB uint64) uint {
quarter := uint(memMB / cacheMemoryFraction)
if quarter < minAutoCacheSizeMb {
return max(quarter, minCacheSizeMb) // e.g. 64; pebble's own floor is 8 MB
}
return min(quarter, maxAutoCacheSizeMb)
}…or keep the flat floor deliberately (defensible — a 1–2 GB host can't sync Juno anyway), and in that case say so in the doc comment on line 724 and log a warning when the floor exceeds a quarter of detected memory, so operators of memory-capped containers can see why the process is being killed. Either way, cmd/juno/juno_internal_test.go should get rows for memMB = 512 and memMB = 2048 to pin the chosen behaviour down.
Separately (unchanged): defaultCacheSizeMb is no longer the default, only the floor — minAutoCacheSizeMb would pair with maxAutoCacheSizeMb and keep the doc comment above self-explanatory.
386db67 to
213ab9b
Compare
|
Claude finished @infrmtcs's task in 3m 36s —— View job Review:
|
1 participant
Summary
utils/memory.go) with teststuning.md,_config-options.md) and config generation to reflect the new default