feat(compiler): memory-aware concurrency limit#3764
Conversation
8fbc440 to
6a3714b
Compare
|
Claude finished @RafaelGranza's task in 3m 47s —— View job PR Review: memory-aware concurrency limit
Reviewed for correctness, the cgroup/container divergence, the fail-fast path, and the flag wiring. This is a clean, well-tested PR — no blockers or correctness bugs. One inline nit and a few notes below. BlockerNone. ImportantNone. Nit
Things I checked that are correct
One heads-up (not a bug)On a many-core but memory-modest host (e.g. 16 CPU / 16 GB, defaults), derived concurrency drops from the old |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3764 +/- ##
==========================================
+ Coverage 75.21% 75.46% +0.25%
==========================================
Files 438 439 +1
Lines 39577 39549 -28
==========================================
+ Hits 29767 29845 +78
+ Misses 7720 7636 -84
+ Partials 2090 2068 -22 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
6a3714b to
d8bdcfb
Compare
|
Claude finished @RafaelGranza's task in 2m 58s —— View job PR Review: memory-aware concurrency limit
This PR is clean — no blockers, no correctness bugs. The change is well-scoped, deterministically tested, and the prior review's log nit has been resolved. BlockerNone. ImportantNone. Things I re-verified on this revision
Nit (non-blocking)
Heads-up for release notes (not a bug)On a many-core / memory-modest host (e.g. 16 CPU / 16 GB, defaults), derived concurrency drops from the old |
d8bdcfb to
ff2d77b
Compare
|
Claude finished @RafaelGranza's task in 57s —— View job PR Review: memory-aware concurrency limit
This revision is clean — no blockers, no correctness bugs. Builds and vets clean locally ( BlockerNone. ImportantNone. Re-verified on this revision
Nit (non-blocking)
Heads-up for release notes (not a bug)On a many-core / memory-modest host (e.g. 16 CPU / 16 GB, defaults), derived concurrency drops from the old |
| hostMemory := memory.TotalMemory() | ||
| cgroupLimit, err := memlimit.FromCgroup() | ||
| if err == nil && cgroupLimit > 0 && cgroupLimit < hostMemory { | ||
| return cgroupLimit / megabyte |
There was a problem hiding this comment.
Won't be covering this line by a test
ff2d77b to
6c2bd83
Compare
|
Claude finished @RafaelGranza's task in 3m 4s —— View job PR Review: memory-aware concurrency limit (re-review after sync)
This revision is clean — no blockers, no correctness bugs. The notable change in this push is that the fail-fast path was replaced with a floor of 1 ( BlockerNone. ImportantNone. Verified on this revision
Nit (non-blocking)
Heads-up for release notes (not a bug)On a many-core / memory-modest host (e.g. 16 CPU / 16 GB, defaults), derived concurrency drops from the old |
6c2bd83 to
22bfa47
Compare
|
Claude finished @RafaelGranza's task in 54s —— View job PR Review: memory-aware concurrency limit (re-review on sync)
This revision is clean — no blockers, no correctness bugs. The change is well-scoped and deterministically tested. BlockerNone. ImportantNone. Verified on this revision
Nit (non-blocking)
Heads-up for release notes (not a bug)On a many-core / memory-modest host (e.g. 16 CPU / 16 GB, defaults), derived concurrency drops from the old |
rodrodros
left a comment
There was a problem hiding this comment.
Rafael the 0 as a way to set is default is not a good solution because 0 can be a desired value in itself. A node set with 0,0 values can me a node that will reject any compilation requests.
Since Cobra allow us to have some expression, can't the defaults be triggered at runtime and CLI print the default values? I don't know if this is a good approach.
The other approach is to allow other values, such as "default" or "dynamic" which are non-numbers and if set, then the calculation is done
|
@rodrodros |
22bfa47 to
c3a7e8e
Compare
|
Claude finished @RafaelGranza's task in 3m 52s —— View job PR Re-review:
|
c3a7e8e to
221e7c6
Compare
|
I'll analyze this and get back to you. |
221e7c6 to
34df267
Compare
|
Claude finished @RafaelGranza's task in 3m 44s —— View job PR Re-review: string-flag approach + CodeQL alertReviewed the latest push, which replaces the Verdict: clean. One low-severity CodeQL finding addressed inline; no blockers, no correctness bugs. Design: string flags resolve @rodrodros's concern correctlyThe empty-string sentinel is a genuinely better fix than
Nit (non-blocking)
Already resolved from earlier review rounds
Verified unchanged-and-correct
Heads-up for release notes (not a bug)Default derived concurrency on a many-core / memory-modest host (e.g. 16 CPU / 16 GB) is |
34df267 to
cfbf38e
Compare
|
Claude finished @RafaelGranza's task in 2m 37s —— View job PR Re-review (sync): CodeQL fix landed
Verdict: clean — no blockers, no correctness bugs. CodeQL #766 — resolved ✅
n, err := strconv.ParseUint(value, 10, strconv.IntSize)
Re-verified on this revision
Nits (non-blocking, carried over)
Heads-up for release notes (not a bug)Default derived concurrency on a many-core / memory-modest host (e.g. 16 CPU / 16 GB) is |
cfbf38e to
077722c
Compare
|
Claude finished @RafaelGranza's task in 2m 15s —— View job PR Re-review (sync): empty-string sentinel + CodeQL fix verified
Verdict: clean — no blockers, no correctness bugs. Resolved review threads
Verified on this revision
Nits (non-blocking, carried over)
Heads-up for release notes (not a bug)Default derived concurrency on a many-core / memory-modest host (e.g. 16 CPU / 16 GB) is |
|
Claude finished @RafaelGranza's task in 2m 45s —— View job PR Review: memory-aware concurrency limit (re-review on sync)
Verdict: clean — no blockers, no correctness bugs. New commit this round —
|
e648ef6 to
4adf717
Compare
|
Claude finished @RafaelGranza's task in 1m 4s —— View job PR Re-review (sync): GOMAXPROCS pin verified
Verdict: clean — no blockers, no correctness bugs. The only change since the last review round is the compile-child New this round —
|
Explaining the chosen variables
Plus: the compile child process is pinned to Real vs virtual memoryEach compilation runs in its own child process, with two very different memory numbers:
They differ this much because Go reserves far more virtual memory than it uses. Compilation memory ceiling (4 GB)
Measured peak virtual memory of the heaviest honest mainnet contract:
4 GB sits conformably above that peak. Pinning the compile child to GOMAXPROCS=2A compilation is one Rust FFI call, so extra Go threads do not make it faster; they only add per-thread stacks that grow the virtual memory peak. Inheriting a large host GOMAXPROCS pushes the memory peak up (1.93 GB at 128 cores, near the limit). Pinning keeps it host-independent (~1.58 GB) at no time cost. Node memory reserve (4 GB)Measured on a real synced mainnet node via
4 GB covers the peak with headroom. ValidationChecked derivation, fallback and virtual peak across Docker (arm64), native macOS (arm64) and a native amd64 host, from 1 to 128 cores. Build, tests and lint green. |

Auto-size Sierra compilation concurrency from available memory
What
Today the number of concurrent Sierra→CASM compilations defaults to
GOMAXPROCS, which ignores memory. A burst of large compilations can exhaust RAM.This PR derives the limit from available memory and CPU count.
Formula (all values in MB):
The limit is always at least 1: a memory-tight node compiles one at a time rather than refusing to start.
Behavior change (heads-up for operators)
The default concurrency is no longer always
GOMAXPROCS. On a many-core but memory-modest host it drops: e.g. 16 CPU / 16 GB with defaults goes from16to(16-4)/4 = 3. Intended, but nodes with heavy P2P orgetCompiledCasmload will see less compile parallelism. Raise--max-concurrent-compilations(used as-is) or--node-memory-reserveto tune.Flags
--max-concurrent-compilationsemptyempty= derive from memory + CPUs. A set value is used as-is.--max-compilation-queueemptyempty= twice the resulting concurrency.--node-memory-reserve4096--max-compilation-memory4096Why these libraries
Two needs: read total RAM, and read the container memory limit. Choice:
pbnjay/memoryautomemlimitGOMEMLIMIT, pure Go, reads cgroup v1/v2.Both were already indirect dependencies; this only promotes them to direct.
automemlimit's only dependency ispbnjay/memory, so the pair adds no new transitive dependency.gopsutilwas considered and dropped: it does not read the cgroup limit (same blind spot aspbnjay), and it would be an extra dependency on top ofpbnjay(whichautomemlimitpulls anyway).Container awareness
A cgroup is the Linux kernel feature that caps how much memory a process group may use; it is how Docker and Kubernetes enforce memory limits. A process can sit on a 64 GiB host but be capped to 4 GiB by its cgroup, and the kernel kills it (OOM) if it goes over.
Reading total system memory (
sysinfo//proc/meminfo) returns the host RAM and ignores cgroups, so inside a memory-limited container it over-reports.AvailableMemoryMB()instead returnsmin(host RAM, cgroup limit), so it respectsdocker run --memory=..and Kubernetes limits.Margin default
--node-memory-reservedefaults to 4 GiB, a conservative reserve for the rest of the node. No deeper analysis; it is configurable. Operators with a large--db-cache-sizeor many VMs can raise it.Testing
Verified by running the node and reading the startup log
(
setting Sierra compilation concurrency).availableMemoryMB--max-compilation-memory 4096--node-memory-reserve 100000--memory=8g--memory=6gThe Docker rows confirm the value follows the cgroup limit, not the 16 GiB host.
When the budget fits no compilation the limit floors to 1, so the node boots and compiles one at a time instead of refusing to start.