Skip to content

feat(compiler): memory-aware concurrency limit#3764

Open
RafaelGranza wants to merge 5 commits into
mainfrom
granza/compiler-memory-limit
Open

feat(compiler): memory-aware concurrency limit#3764
RafaelGranza wants to merge 5 commits into
mainfrom
granza/compiler-memory-limit

Conversation

@RafaelGranza

@RafaelGranza RafaelGranza commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

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):

limit = min( (availableMemory - nodeMemoryReserve) / maxMemoryPerCompilation , CPUs )

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 from 16 to (16-4)/4 = 3. Intended, but nodes with heavy P2P or getCompiledCasm load will see less compile parallelism. Raise --max-concurrent-compilations (used as-is) or --node-memory-reserve to tune.

Flags

Flag Default Meaning
--max-concurrent-compilations empty empty = derive from memory + CPUs. A set value is used as-is.
--max-compilation-queue empty empty = twice the resulting concurrency.
--node-memory-reserve 4096 RAM (MB) kept for the rest of the node, out of the compilation budget. New.
--max-compilation-memory 4096 RAM (MB) one compilation may use. Existing; it is the divisor above.

Why these libraries

Two needs: read total RAM, and read the container memory limit. Choice:

Need Library Why
Host/VM RAM pbnjay/memory One function, pure Go, no cgo, all OSes.
Container limit automemlimit Purpose-built for GOMEMLIMIT, pure Go, reads cgroup v1/v2.

Both were already indirect dependencies; this only promotes them to direct.
automemlimit's only dependency is pbnjay/memory, so the pair adds no new transitive dependency.

gopsutil was considered and dropped: it does not read the cgroup limit (same blind spot as pbnjay), and it would be an extra dependency on top of pbnjay (which automemlimit pulls 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 returns min(host RAM, cgroup limit), so it respects docker run --memory=.. and Kubernetes limits.

Environment host-only reading this PR
Bare metal / no limit host RAM host RAM
Container with memory limit host RAM (wrong) cgroup limit
macOS / Windows (no cgroup) host RAM host RAM

Margin default

--node-memory-reserve defaults to 4 GiB, a conservative reserve for the rest of the node. No deeper analysis; it is configurable. Operators with a large --db-cache-size or many VMs can raise it.

Testing

Verified by running the node and reading the startup log
(setting Sierra compilation concurrency).

Environment Config availableMemoryMB concurrent compilations Result
macOS native default 16384 (host) 10 (CPUs) boots
macOS native --max-compilation-memory 4096 16384 3 boots
macOS native --node-memory-reserve 100000 16384 1 (floored) boots
Docker --memory=8g default 8192 (cgroup) 1 boots
Docker --memory=6g default 6144 (cgroup) 1 (floored) boots

The 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.

@RafaelGranza RafaelGranza force-pushed the granza/compiler-memory-limit branch from 8fbc440 to 6a3714b Compare June 25, 2026 01:47
Comment thread node/node.go Outdated
@RafaelGranza RafaelGranza self-assigned this Jun 25, 2026
@RafaelGranza RafaelGranza marked this pull request as ready for review June 25, 2026 01:51
@github-actions

github-actions Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Claude finished @RafaelGranza's task in 3m 47s —— View job


PR Review: memory-aware concurrency limit

  • Read changed files
  • Review starknet/compiler/concurrency.go (core logic)
  • Review node/node.go wiring + fail-fast
  • Review cmd/juno/juno.go flags
  • Review tests
  • Post review

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.

Blocker

None.

Important

None.

Nit

  • node/node.go:319 — misleading log fields when concurrency is overridden (inline). Also answers your question on the log: keep it, just consider gating the memory fields (or adding a source field) so they don't imply they drove a value that was actually set by flag.

Things I checked that are correct

  • compilerInUse set is complete. cfg.HTTP || cfg.Websocket || cfg.P2P || cfg.Sequencer covers every code path that actually invokes throttledCompiler — the RPC handler (getCompiledCasm), the P2P service (p2p.New(..., throttledCompiler)), and the sequencer. Plain feeder sync downloads pre-compiled CASM and never compiles, so a default sync-only node correctly skips the fail-fast and falls through to max(..., 1). Good.
  • No underflow. ConcurrencyLimit guards availableMemory <= nodeMemoryReserve before the uint64 subtraction, so the unsigned subtract can't wrap. The maxMemoryPerCompilation == 0 short-circuit also avoids a divide-by-zero.
  • cgroup logic is sound. cgroupLimit > 0 && cgroupLimit < hostMemory correctly treats the "unlimited" cgroup (max value ≥ host) as "use host RAM", and FromCgroup() returns an error on non-Linux so it falls back to host memory. Units are consistent (bytes compared in bytes, then /megabyte).
  • Tests are deterministic, not time-based: NodeMemoryReserve: AvailableMemoryMB() + 4096 guarantees nothing fits regardless of host RAM, and the two new node tests cover both the reject-on-zero-budget and ignore-when-no-compiler branches. The table tests hit the boundary cases (==reserve, <reserve+one, cap-by-parallel, unbounded).
  • Dependency claim holdsautomemlimit's only dep is pbnjay/memory; both move from indirect → direct with no new transitive deps.

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 GOMAXPROCS (16) to (16-4)/4 = 3. That's the intended behavior, but it's a meaningful default change for existing deployments — worth a line in the release notes / changelog so operators on P2P or heavy getCompiledCasm workloads aren't surprised by reduced compile parallelism.

Comment thread node/node.go Outdated
@codecov

codecov Bot commented Jun 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 86.88525% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.46%. Comparing base (c35a08d) to head (4adf717).

Files with missing lines Patch % Lines
node/node.go 83.78% 3 Missing and 3 partials ⚠️
starknet/compiler/concurrency.go 84.61% 1 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@RafaelGranza RafaelGranza force-pushed the granza/compiler-memory-limit branch from 6a3714b to d8bdcfb Compare June 25, 2026 02:14
@github-actions

github-actions Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Claude finished @RafaelGranza's task in 2m 58s —— View job


PR Review: memory-aware concurrency limit

  • Read changed files
  • Review starknet/compiler/concurrency.go (core logic)
  • Review node/node.go wiring + fail-fast + log
  • Review cmd/juno/juno.go flags + tests
  • Build + vet affected packages
  • Post review

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.

Blocker

None.

Important

None.

Things I re-verified on this revision

  • Log nit resolved. The latest commit (d8bdcfbff) splits the log into two branches: the auto-derive path logs the memory fields (availableMemoryMB, nodeMemoryReserveMB, maxMemoryPerCompilationMB), and the explicit-flag path logs only limit. This exactly addresses the earlier "misleading memory fields when overridden" feedback — the memory fields no longer imply they drove a value set by flag. 👍
  • compilerInUse set is still complete. Traced every throttledCompiler consumer: the RPC handler (getCompiledCasm) is only served via makeRPCOverHTTP/makeRPCOverWebsocket, gated on cfg.HTTP/cfg.Websocket (node.go:566, 587); P2P passes it to p2p.New (node.go:464); the sequencer wires it (node.go:368). makeGRPC (node.go:629) takes only database, version — it does not compile — so gRPC is correctly excluded. cfg.HTTP || cfg.Websocket || cfg.P2P || cfg.Sequencer covers exactly the compiling paths.
  • No underflow / divide-by-zero. maxMemoryPerCompilation == 0 short-circuits to maxParallel, and availableMemory <= nodeMemoryReserve is guarded before the unsigned subtraction (concurrency.go:30–37).
  • cgroup logic sound. cgroupLimit > 0 && cgroupLimit < hostMemory treats an "unlimited" cgroup (limit ≥ host) as host RAM, and FromCgroup() erroring on non-Linux falls back to host memory. Byte/MB units are consistent.
  • Fail-fast + fallback correct. Returns a clear actionable error when budget is 0 and a compiling service is enabled; otherwise max(..., 1) keeps one slot for sync-only nodes. The two new node tests cover both branches deterministically (AvailableMemoryMB() + 4096 guarantees a zero budget regardless of host RAM).
  • Builds & vets clean locally (starknet/compiler, node); table tests cover the boundary cases (==reserve, <reserve+one, cap-by-parallel, unbounded).

Nit (non-blocking)

  • Codecov flags 2 uncovered lines in concurrency.go (the in-container return cgroupLimit / megabyte branch of AvailableMemoryMB). That path only runs under a real cgroup limit and is genuinely awkward to unit-test — fine to leave, but if you want the coverage, AvailableMemoryMB could be refactored to take the host/cgroup values as params so a pure-logic helper is testable. Optional.

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 GOMAXPROCS (16) to (16-4)/4 = 3. Intended, but it's a meaningful default change for existing P2P / heavy getCompiledCasm deployments — worth a changelog line so operators aren't surprised by reduced compile parallelism.

@RafaelGranza RafaelGranza force-pushed the granza/compiler-memory-limit branch from d8bdcfb to ff2d77b Compare June 25, 2026 02:39
@github-actions

github-actions Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Claude finished @RafaelGranza's task in 57s —— View job


PR Review: memory-aware concurrency limit

  • Read changed files
  • Review starknet/compiler/concurrency.go (core logic)
  • Review node/node.go wiring + fail-fast + log
  • Review cmd/juno/juno.go flags + tests
  • Build + vet affected packages
  • Post review

This revision is clean — no blockers, no correctness bugs. Builds and vets clean locally (starknet/compiler, node, cmd). The change is well-scoped and deterministically tested.

Blocker

None.

Important

None.

Re-verified on this revision

  • Log split is correct. node/node.go now logs "deriving Sierra compilation concurrency" with the memory fields (availableMemoryMB, nodeMemoryReserveMB, maxMemoryPerCompilationMB) only on the auto-derive path, and "using configured Sierra compilation concurrency" with just limit on the explicit-flag path. This resolves the earlier "misleading memory fields when overridden" feedback — the memory fields no longer imply they drove a value set by flag. 👍 (Answering your inline question: keep the log — it fires once at startup and is the line operators will grep when compilation is slower than expected; Info is the right altitude.)
  • No underflow / divide-by-zero. maxMemoryPerCompilation == 0 short-circuits to maxParallel, and availableMemory <= nodeMemoryReserve is guarded before the unsigned subtraction (concurrency.go:30–37).
  • cgroup logic sound. cgroupLimit > 0 && cgroupLimit < hostMemory treats an "unlimited" cgroup (limit ≥ host) as host RAM; FromCgroup() erroring on non-Linux falls back to host memory. Byte/MB units are consistent.
  • Fail-fast + fallback correct. Returns an actionable error when budget is 0 and a compiling service is enabled (cfg.HTTP || cfg.Websocket || cfg.P2P || cfg.Sequencer); otherwise max(..., 1) keeps one slot for sync-only nodes. The three node tests cover reject-on-zero-budget, ignore-when-no-compiler, and explicit-override — all deterministic (AvailableMemoryMB() + 4096 guarantees a zero budget regardless of host RAM).
  • Queue derivation correct. MaxCompilationQueue == 02 * concurrentCompilations, applied after concurrency is resolved, so it also tracks an explicitly-set concurrency. Both flag defaults moved to 0 consistently in cmd/juno and the precedence test.

Nit (non-blocking)

  • Codecov flags the in-container return cgroupLimit / megabyte branch of AvailableMemoryMB as uncovered. It only runs under a real cgroup limit and is awkward to unit-test — fine to leave. If you want the coverage, AvailableMemoryMB could take host/cgroup values as params so a pure-logic helper is testable. Optional.

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 GOMAXPROCS (16) to (16-4)/4 = 3. Intended, but it's a meaningful default change for existing P2P / heavy getCompiledCasm deployments — worth a changelog line so operators aren't surprised by reduced compile parallelism.

hostMemory := memory.TotalMemory()
cgroupLimit, err := memlimit.FromCgroup()
if err == nil && cgroupLimit > 0 && cgroupLimit < hostMemory {
return cgroupLimit / megabyte

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't be covering this line by a test

@RafaelGranza RafaelGranza force-pushed the granza/compiler-memory-limit branch from ff2d77b to 6c2bd83 Compare June 25, 2026 20:19
@github-actions

github-actions Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Claude finished @RafaelGranza's task in 3m 4s —— View job


PR Review: memory-aware concurrency limit (re-review after sync)

  • Read changed files at latest commit
  • Review starknet/compiler/concurrency.go (core logic)
  • Review node/node.go wiring + log split
  • Review cmd/juno/juno.go flags + tests
  • Build + vet affected Go packages
  • Post review

This revision is clean — no blockers, no correctness bugs. starknet/compiler and node build and go vet clean locally (the cmd/juno link failure here is only missing prebuilt Rust artifacts, unrelated to this change).

The notable change in this push is that the fail-fast path was replaced with a floor of 1 (max(ConcurrencyLimit(...), 1)), and the docs now state "The limit is always at least 1: on a memory-tight node compilations run one at a time rather than the node refusing to start." This is a sound simplification — a tight-memory node degrades to serial compilation instead of refusing to boot, and there's no longer any need to enumerate which services compile. The node_test.go cases were updated to match (TestNewNodeRunsOneAtATimeOnLowMemory now expects NoError).

Blocker

None.

Important

None.

Verified on this revision

  • No underflow / divide-by-zero. maxMemoryPerCompilation == 0 short-circuits to maxParallel, and availableMemory <= nodeMemoryReserve is guarded before the unsigned subtraction (concurrency.go:30–37). The floor of 1 also guarantees a usable compiler regardless of budget.
  • cgroup logic sound. cgroupLimit > 0 && cgroupLimit < hostMemory treats an "unlimited" cgroup (limit ≥ host) as host RAM; FromCgroup() erroring on non-Linux falls back to host. Byte/MB units are consistent (memory.TotalMemory() and FromCgroup() are both bytes, divided by megabyte).
  • Log split is correct. Auto-derive path logs the memory fields under "deriving Sierra compilation concurrency"; the explicit-flag path logs only limit under "using configured Sierra compilation concurrency" — the memory fields no longer imply they drove a flag-set value.
  • Queue derivation correct. MaxCompilationQueue == 02 * concurrentCompilations, computed after concurrency resolves, so it tracks both derived and explicit concurrency.
  • Tests deterministic. NodeMemoryReserve: AvailableMemoryMB() + 4096 forces a zero budget regardless of host RAM/OS, exercising the floor; TestNewNodeSkipsDerivedConcurrency covers the explicit-override branch. Flag defaults moved to 0 consistently across cmd/juno and the precedence test.
  • Dependency claim holdsautomemlimitpbnjay/memory only; both move indirect → direct with no new transitive deps.

Nit (non-blocking)

  • Stale PR description. The PR body still documents the fail-fast behavior ("the node fails fast at startup", the two startup error test rows). The code now floors at 1 and never refuses to start on a zero budget. Worth updating the description so it matches the shipped behavior. The docs (tuning.md) are already correct.
  • Codecov: in-container return cgroupLimit / megabyte branch (concurrency.go:16) is uncovered — you noted this in an inline comment. It only runs under a real cgroup limit and is awkward to unit-test; fine to leave. If you want it, extracting a pure helper that takes (hostMemory, cgroupLimit, cgroupErr) would make the selection logic testable.

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 GOMAXPROCS (16) to (16-4)/4 = 3. Intended, but a meaningful default change for existing P2P / heavy getCompiledCasm deployments — worth a changelog line so operators aren't surprised by reduced compile parallelism.
· branch granza/compiler-memory-limit

@RafaelGranza RafaelGranza force-pushed the granza/compiler-memory-limit branch from 6c2bd83 to 22bfa47 Compare June 26, 2026 11:10
@github-actions

github-actions Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Claude finished @RafaelGranza's task in 54s —— View job


PR Review: memory-aware concurrency limit (re-review on sync)

  • Read changed files at latest commit
  • Review starknet/compiler/concurrency.go (core logic)
  • Review node/node.go wiring + log split
  • Review cmd/juno/juno.go flags + tests
  • Post review

This revision is clean — no blockers, no correctness bugs. The change is well-scoped and deterministically tested.

Blocker

None.

Important

None.

Verified on this revision

  • No underflow / divide-by-zero. maxMemoryPerCompilation == 0 short-circuits to maxParallel, and availableMemory <= nodeMemoryReserve is guarded before the unsigned subtraction (concurrency.go:30–37). max(ConcurrencyLimit(...), 1) in node.go:296 guarantees a usable compiler even on a zero budget, so a memory-tight node degrades to serial compilation rather than refusing to boot.
  • cgroup logic sound. cgroupLimit > 0 && cgroupLimit < hostMemory treats an "unlimited" cgroup (limit ≥ host) as host RAM; FromCgroup() erroring on non-Linux falls back to host. Units consistent — both memory.TotalMemory() and FromCgroup() are bytes, divided by megabyte.
  • Log split correct. Auto-derive path logs the memory fields under "deriving Sierra compilation concurrency"; the explicit-flag path logs only limit under "using configured Sierra compilation concurrency" — memory fields no longer imply they drove a flag-set value. This resolves your earlier inline question.
  • Queue derivation correct. MaxCompilationQueue == 02 * concurrentCompilations, computed after concurrency resolves, so it tracks both derived and explicit concurrency.
  • Tests deterministic. NodeMemoryReserve: AvailableMemoryMB() + 4096 forces a zero budget regardless of host RAM/OS, exercising the floor (TestNewNodeRunsOneAtATimeOnLowMemory); TestNewNodeSkipsDerivedConcurrency covers the explicit-override branch. Flag defaults moved to 0 consistently across cmd/juno and the precedence test. Table tests cover the boundary cases (==reserve, <reserve+one, cap-by-parallel, unbounded).
  • Dependency claim holdsautomemlimit's only dep is pbnjay/memory; both move indirect → direct with no new transitive deps (go.mod).

Nit (non-blocking)

  • Codecov: in-container return cgroupLimit / megabyte branch (concurrency.go:16) is uncovered — you flagged this inline yourself. It only runs under a real cgroup limit and is awkward to unit-test; fine to leave. If you want it, extract a pure helper taking (hostMemory, cgroupLimit, cgroupErr) so the selection logic is testable in isolation.

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 GOMAXPROCS (16) to (16-4)/4 = 3. Intended, but a meaningful default change for existing P2P / heavy getCompiledCasm deployments — worth a changelog line so operators aren't surprised by reduced compile parallelism.
· branch granza/compiler-memory-limit

Comment thread cmd/juno/juno.go Outdated
Comment thread cmd/juno/juno.go Outdated

@rodrodros rodrodros left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@RafaelGranza

RafaelGranza commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

@rodrodros
I Switched the default to "" (empty string), so 0 stays valid as "disable". So the flags are now Int. It is the simplest option I found, but open to alternatives.

@RafaelGranza RafaelGranza force-pushed the granza/compiler-memory-limit branch from 22bfa47 to c3a7e8e Compare June 30, 2026 04:26
@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Claude finished @RafaelGranza's task in 3m 52s —— View job


PR Re-review: -1 sentinel for derived concurrency

Reviewed the switch from uint+0-sentinel to int+-1-sentinel (commit c3a7e8ee3). Built starknet/compiler and node clean.

Verdict: the -1 approach is sound and correctly resolves @rodrodros's concern. No blockers, no correctness bugs.

Answering the design question

Switched the default to -1, so 0 stays valid as "disable". Open to alternatives.

This is the right call, and I'd keep it over a string "default"/"dynamic" enum — int keeps the flag type simple, avoids custom parsing, and -1 reads naturally as "unset/auto". I verified the 0 semantics @rodrodros wanted actually hold end-to-end:

  • With concurrency=0, queue=0, the throttler computes maxRequests = 0 + 0 = 0, and Throttler.Do returns ErrResourceBusy for every request (activeReqs (1) > maxRequests (0)). So --max-concurrent-compilations 0 is a genuine "reject all compilations" node, exactly as intended. ✅
  • The floor of 1 (max(ConcurrencyLimit(...), 1)) only applies on the derive path, so explicit 0 is not silently bumped to 1. Correct — the floor is a memory-budget guard, not an override of an explicit operator choice. ✅
  • Non-Linux: PreRunE zeroes MaxCompilationMemory when unset → derive path hits the maxMemoryPerCompilation == 0 short-circuit → returns GOMAXPROCS. Matches the old default and the tuning.md wording. ✅
  • MaxCompilationQueue == -12 * concurrentCompilations, tracking both derived and explicit concurrency. ✅
  • Field type change uint → int: no other consumers — only node.go and cmd/juno reference these fields, and both convert explicitly. ✅

Nits (non-blocking)

  • Magic -1 / < 0 checks. node/node.go:299 and :320 test < 0, and the sentinel -1 is repeated as a literal in cmd/juno/juno.go:490-491. This also ties into @rodrodros's "maybe they can be constants?" comment — a single shared const deriveFromMemory = -1 (or similar) used in both the flag default and the node.go branch would make intent self-documenting and keep the two sites from drifting. Optional but cheap.
  • All negatives derive, not just -1. < 0 means -5 also triggers derive, while the help text documents only -1. Harmless, but if you want to be strict you could treat only -1 as the sentinel and reject other negatives in PreRunE. Low priority.
  • Stale PR description. The body still describes the old fail-fast behavior and the 0-sentinel flag table (--max-concurrent-compilations default 0). The shipped behavior is -1 default + floor-to-1. Worth syncing the description; docs/tuning.md is already correct.
  • (Pre-existing) Codecov still flags the in-container return cgroupLimit / megabyte branch (concurrency.go:16) — awkward to unit-test, fine to leave.

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 (16-4)/4 = 3, down from the old GOMAXPROCS (16). Intended, but a meaningful default change for heavy P2P / getCompiledCasm deployments — worth a changelog line.
· branch granza/compiler-memory-limit

@RafaelGranza RafaelGranza force-pushed the granza/compiler-memory-limit branch from c3a7e8e to 221e7c6 Compare June 30, 2026 19:45
@github-actions

Copy link
Copy Markdown
Contributor

Claude Code is working…

I'll analyze this and get back to you.

View job run

Comment thread node/node.go Fixed
@RafaelGranza RafaelGranza force-pushed the granza/compiler-memory-limit branch from 221e7c6 to 34df267 Compare June 30, 2026 19:58
@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Claude finished @RafaelGranza's task in 3m 44s —— View job


PR Re-review: string-flag approach + CodeQL alert

Reviewed the latest push, which replaces the -1/int sentinel with string flags where empty = derive (MaxConcurrentCompilations/MaxCompilationQueue are now string, parsed by parseCompilationLimit). Built starknet/compiler and node clean.

Verdict: clean. One low-severity CodeQL finding addressed inline; no blockers, no correctness bugs.

Design: string flags resolve @rodrodros's concern correctly

The empty-string sentinel is a genuinely better fix than 0 or -1:

  • "" → derive from memory + CPUs; 0 → a real "reject all compilations" value (throttler gets maxRequests = 0+0 and Do returns ErrResourceBusy for every request). This is exactly the 0,0 semantics @rodrodros asked for, and the value is now distinct from "unset". ✅
  • The floor of 1 (max(ConcurrencyLimit(...), 1)) only runs on the derive path, so an explicit 0 is not silently bumped to 1. Correct. ✅
  • MaxCompilationQueue == ""2 * concurrentCompilations, tracking both derived and explicit concurrency. ✅
  • Config-file numeric values parse correctly (new numeric compilation limits in config file precedence test). ✅

Nit (non-blocking)

  • node/node.go:165 — uint64→uint conversion (CodeQL Fix shadowed declaration #766). ParseUint(value, 10, 64) then uint(n) truncates on 32-bit builds. Posted an inline suggestion to parse at strconv.IntSize so the conversion is lossless and the scanner clears. Not a real risk on 64-bit deployments. Fix this →

Already resolved from earlier review rounds

  • @rodrodros's "unnecessary whitespace line" — the cmd/juno/juno.go default block (lines 490–492) is clean now.
  • The "0 is a desired value" objection — resolved by the empty-string sentinel above.
  • Log split (deriving vs using configured) — still correct; memory fields only on the derive path.

Verified unchanged-and-correct

  • No underflow / divide-by-zero in ConcurrencyLimit (maxMemoryPerCompilation == 0 short-circuits; availableMemory <= nodeMemoryReserve guarded before the unsigned subtract).
  • cgroup logic sound (cgroupLimit > 0 && cgroupLimit < hostMemory; FromCgroup() errors fall back to host RAM; consistent byte→MB units).
  • Tests deterministicNodeMemoryReserve: AvailableMemoryMB() + 4096 forces a zero budget regardless of host; TestNewNodeRunsOneAtATimeOnLowMemory exercises the floor, TestNewNodeSkipsDerivedConcurrency the explicit path.

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 (16-4)/4 = 3, down from the old GOMAXPROCS (16). Intended, but a meaningful default change for heavy P2P / getCompiledCasm deployments — worth a changelog line. The PR description still documents the older fail-fast/-1 behavior; worth syncing it to the shipped empty-string design (docs/tuning.md is already correct).
· branch granza/compiler-memory-limit

Comment thread node/node.go Outdated
@RafaelGranza RafaelGranza force-pushed the granza/compiler-memory-limit branch from 34df267 to cfbf38e Compare June 30, 2026 20:14
@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Claude finished @RafaelGranza's task in 2m 37s —— View job


PR Re-review (sync): CodeQL fix landed

  • Read changed files at latest commit (cfbf38ede)
  • Verify CodeQL uint64→uint fix applied
  • Re-check string-flag parsing + 0 semantics
  • Build + go vet affected packages

Verdict: clean — no blockers, no correctness bugs. starknet/compiler and node build and go vet clean locally.

CodeQL #766 — resolved ✅

node/node.go:161 now parses at the platform width:

n, err := strconv.ParseUint(value, 10, strconv.IntSize)

ParseUint rejects out-of-range input up front, so the uint(n) on line 165 is lossless and the alert clears. Negative input also errors cleanly (%q is not a non-negative integer), which is a nicer property than the earlier -1 sentinel.

Re-verified on this revision

  • Empty-string sentinel semantics hold end-to-end. "" → derive; explicit 0 is a real "reject all compilations" value (throttler maxRequests = 0, every request gets ErrResourceBusy). The floor max(ConcurrencyLimit(...), 1) runs only on the derive path (node.go:319), so an explicit 0 is not silently bumped to 1. ✅
  • Queue derivation tracks concurrency. MaxCompilationQueue == ""2 * concurrentCompilations, applied after concurrency resolves (node.go:341-343). ✅
  • Config-file numeric values decode into the string fields — new numeric compilation limits in config file precedence test covers 8/32. ✅
  • No underflow / divide-by-zero in ConcurrencyLimitmaxMemoryPerCompilation == 0 short-circuits to maxParallel; availableMemory <= nodeMemoryReserve is guarded before the unsigned subtraction (concurrency.go:30-37).
  • cgroup logic soundcgroupLimit > 0 && cgroupLimit < hostMemory treats an "unlimited" cgroup as host RAM; FromCgroup() errors fall back to host; byte→MB units consistent.
  • Tests deterministicNodeMemoryReserve: AvailableMemoryMB() + 4096 forces a zero budget regardless of host (TestNewNodeRunsOneAtATimeOnLowMemory exercises the floor; TestNewNodeSkipsDerivedConcurrency the explicit path).

Nits (non-blocking, carried over)

  • Stale PR description. The body still documents the old fail-fast behavior and the 0-default flag table. Shipped behavior is empty-string default + floor-to-1, with 0 meaning "disable". Worth syncing the description (docs/tuning.md is already correct).
  • Codecov: the in-container return cgroupLimit / megabyte branch (concurrency.go:16) stays uncovered — you flagged this inline; awkward to unit-test, fine to leave.

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 (16-4)/4 = 3, down from the old GOMAXPROCS (16). Intended, but a meaningful default change for heavy P2P / getCompiledCasm deployments — worth a changelog line.
· branch granza/compiler-memory-limit

@RafaelGranza RafaelGranza force-pushed the granza/compiler-memory-limit branch from cfbf38e to 077722c Compare June 30, 2026 23:21
@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Claude finished @RafaelGranza's task in 2m 15s —— View job


PR Re-review (sync): empty-string sentinel + CodeQL fix verified

  • Read changed files at latest commit
  • Verify CodeQL uint64→uint fix
  • Re-check string-flag parsing + 0/"" semantics
  • Build affected packages

Verdict: clean — no blockers, no correctness bugs. starknet/compiler and node build clean locally.

Resolved review threads

  • CodeQL Fix shadowed declaration #766 (node/node.go:161) — now strconv.ParseUint(value, 10, strconv.IntSize). ParseUint rejects out-of-range input up front, so the uint(n) on line 165 is lossless on every platform width and the alert clears. Negative input errors cleanly (%q is not a non-negative integer). ✅
  • @rodrodros's "0 is a desired value" — resolved by the empty-string sentinel. "" → derive; explicit 0 → a genuine "reject all compilations" node (maxRequests = 0, every request gets ErrResourceBusy). The two are now distinct. ✅
  • @rodrodros's whitespace nit (cmd/juno/juno.go:490-492) — the default block is clean. ✅

Verified on this revision

  • Floor only on derive path. max(ConcurrencyLimit(...), 1) runs only when concurrentAuto is true (node.go:319), so an explicit 0 is not silently bumped to 1 — the floor is a memory-budget guard, not an override of operator intent. ✅
  • No underflow / divide-by-zero. maxMemoryPerCompilation == 0 short-circuits to maxParallel; availableMemory <= nodeMemoryReserve is guarded before the unsigned subtraction (concurrency.go:30-37). ✅
  • cgroup logic sound. cgroupLimit > 0 && cgroupLimit < hostMemory treats an "unlimited" cgroup as host RAM; FromCgroup() errors fall back to host; byte→MB units consistent. ✅
  • Queue derivation tracks concurrency. MaxCompilationQueue == ""2 * concurrentCompilations, computed after concurrency resolves (node.go:341-343). ✅
  • Log split correct. Memory fields appear only under "deriving Sierra compilation concurrency"; the explicit-flag path logs only limit. ✅
  • Tests deterministic. NodeMemoryReserve: AvailableMemoryMB() + 4096 forces a zero budget regardless of host (TestNewNodeRunsOneAtATimeOnLowMemory exercises the floor; TestNewNodeSkipsDerivedConcurrency the explicit path); config-file numeric values decode into the string fields.

Nits (non-blocking, carried over)

  • Stale PR description. The body still documents the old fail-fast behavior and the 0-default flag table. Shipped behavior is empty-string default + floor-to-1, with 0 meaning "disable". Worth syncing (docs/tuning.md is already correct).
  • Codecov. The in-container return cgroupLimit / megabyte branch (concurrency.go:16) stays uncovered — you flagged this inline; awkward to unit-test, fine to leave. If you want it, extract a pure helper taking (hostMemory, cgroupLimit, cgroupErr) so the selection logic is testable in isolation.

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 (16-4)/4 = 3, down from the old GOMAXPROCS (16). Intended, but a meaningful default change for heavy P2P / getCompiledCasm deployments — worth a changelog line.
· branch granza/compiler-memory-limit

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Claude finished @RafaelGranza's task in 2m 45s —— View job


PR Review: memory-aware concurrency limit (re-review on sync)

  • Read changed files at latest commit
  • Review starknet/compiler/concurrency.go + compiler.go
  • Review node/node.go wiring, parsing, log
  • Review cmd/juno/juno.go flags + tests
  • Post review

Verdict: clean — no blockers, no correctness bugs. starknet/compiler builds clean locally. The only new change since the last review round is the GOMAXPROCS pin (e648ef60d), which I looked at closely; it's correct.

New commit this round — fix(compiler): pin compile child GOMAXPROCS

compiler.go:112 now sets cmd.Env = append(os.Environ(), "GOMAXPROCS="+compileChildGOMAXPROCS) on the compile child.

  • The append is safe even if the operator exports GOMAXPROCS. os/exec dedups the env before execve (dedupEnvCase), keeping the last occurrence of each key. Since the pinned GOMAXPROCS=2 is appended after os.Environ(), it wins over any inherited value — the child is reliably pinned to 2 threads. No silent override hole.
  • Rationale is sound and consistent with the rest of the PR. Fewer scheduler threads → fewer heap arenas → smaller virtual address space, which keeps each child comfortably under the RLIMIT_AS set from --max-compilation-memory. That's the same MB figure used as the divisor in ConcurrencyLimit, so the address-space bound and the memory-budget accounting now line up.

Re-verified unchanged-and-correct

  • No underflow / divide-by-zero in ConcurrencyLimitmaxMemoryPerCompilation == 0 short-circuits to maxParallel; availableMemory <= nodeMemoryReserve guarded before the unsigned subtraction (concurrency.go:30-37).
  • Empty-string sentinel end-to-end. "" → derive; explicit 0 → genuine "reject all compilations" (throttler maxRequests = 0). The floor max(ConcurrencyLimit(...), 1) runs only on the derive path (node.go:317-324), so an explicit 0 is not bumped to 1.
  • CodeQL Fix shadowed declaration #766 resolvedparseCompilationLimit parses at strconv.IntSize (node.go:161), so uint(n) is lossless and negative input errors cleanly.
  • cgroup logic sound; log split keeps memory fields on the derive path only; queue derivation (MaxCompilationQueue == ""2 * concurrentCompilations) tracks both derived and explicit concurrency.
  • Tests deterministicNodeMemoryReserve: AvailableMemoryMB() + 4096 forces a zero budget regardless of host; new numeric compilation limits in config file precedence case covers 8/32 decoding into the string fields.

Nits (non-blocking, carried over)

  • Stale PR description. The body still documents the old fail-fast behavior and the 0-default flag table. Shipped behavior is empty-string default + floor-to-1, with 0 meaning "disable". Worth syncing (docs/tuning.md is already correct).
  • Codecov: the in-container return cgroupLimit / megabyte branch (concurrency.go:16) stays uncovered — you flagged it inline; awkward to unit-test, fine to leave.

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 (16-4)/4 = 3, down from the old GOMAXPROCS. Intended, but a meaningful default change for heavy P2P / getCompiledCasm deployments — worth a changelog line.
· branch granza/compiler-memory-limit

@RafaelGranza RafaelGranza force-pushed the granza/compiler-memory-limit branch from e648ef6 to 4adf717 Compare July 6, 2026 03:01
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Claude finished @RafaelGranza's task in 1m 4s —— View job


PR Re-review (sync): GOMAXPROCS pin verified

  • Read changed files at latest commit (4adf7175e)
  • Review starknet/compiler/concurrency.go + compiler.go
  • Review node/node.go wiring, parsing, log
  • Review cmd/juno/juno.go flags + tests
  • Post review

Verdict: clean — no blockers, no correctness bugs. The only change since the last review round is the compile-child GOMAXPROCS pin (4adf7175e); I looked at it closely and it's correct.

New this round — pin compile child GOMAXPROCS

compiler.go:112 sets cmd.Env = append(os.Environ(), "GOMAXPROCS="+compileChildGOMAXPROCS) (compileChildGOMAXPROCS = "2").

  • The append wins even if the operator exports GOMAXPROCS. os/exec dedups the env before execve (dedupEnvCase) keeping the last occurrence of each key, and the pinned value is appended after os.Environ() — so the child is reliably pinned to 2, no silent-override hole.
  • Rationale is sound and consistent with the PR. Fewer scheduler threads → fewer heap arenas → smaller virtual address space, keeping each child comfortably under the RLIMIT_AS derived from --max-compilation-memory — the same MB figure used as the divisor in ConcurrencyLimit, so the address-space bound and the memory-budget accounting line up.

Re-verified unchanged-and-correct

  • No underflow / divide-by-zero in ConcurrencyLimitmaxMemoryPerCompilation == 0 short-circuits to maxParallel; availableMemory <= nodeMemoryReserve is guarded before the unsigned subtraction (concurrency.go:30-37).
  • Empty-string sentinel end-to-end. "" → derive; explicit 0 → a genuine "reject all compilations" value (throttler maxRequests = 0). The floor max(ConcurrencyLimit(...), 1) runs only on the derive path (node.go:317-324), so an explicit 0 is not bumped to 1.
  • CodeQL Fix shadowed declaration #766 resolvedparseCompilationLimit parses at strconv.IntSize (node.go:161), so uint(n) is lossless and negative input errors cleanly (%q is not a non-negative integer).
  • cgroup logic soundcgroupLimit > 0 && cgroupLimit < hostMemory treats an "unlimited" cgroup as host RAM; FromCgroup() errors fall back to host; byte→MB units consistent.
  • Log split correct — memory fields appear only under "deriving Sierra compilation concurrency"; the explicit-flag path logs only limit.
  • Queue derivationMaxCompilationQueue == ""2 * concurrentCompilations, computed after concurrency resolves, tracking both derived and explicit values.
  • Tests deterministicNodeMemoryReserve: AvailableMemoryMB() + 4096 forces a zero budget regardless of host; the numeric compilation limits in config file case covers decoding into the string fields.

Nits (non-blocking, carried over)

  • Stale PR description. The body still documents the old fail-fast behavior and the 0-default flag table. Shipped behavior is empty-string default + floor-to-1, with 0 meaning "disable". Worth syncing (docs/tuning.md is already correct).
  • Codecov: the in-container return cgroupLimit / megabyte branch (concurrency.go:16) stays uncovered — you flagged it inline; awkward to unit-test, fine to leave.

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 (16-4)/4 = 3, down from the old GOMAXPROCS. Intended, but a meaningful default change for heavy P2P / getCompiledCasm deployments — worth a changelog line.
· branch granza/compiler-memory-limit

@RafaelGranza

Copy link
Copy Markdown
Contributor Author

Explaining the chosen variables

flag value role
--max-compilation-memory 4 GB per-compile address-space ceiling; also sets how many run at once
--node-memory-reserve 4 GB memory held back for the rest of the node

Plus: the compile child process is pinned to GOMAXPROCS=2.

Real vs virtual memory

Each compilation runs in its own child process, with two very different memory numbers:

a compilation what it means
Real memory (RSS) ~150-200 MB what it actually occupies in RAM
Virtual memory (address space) ~1.5-1.8 GB what it maps, used or not

They differ this much because Go reserves far more virtual memory than it uses.

Compilation memory ceiling (4 GB)

--max-compilation-memory is an RLIMIT_AS limit, so it caps virtual memory. A compile maps ~1.6 GB of it, so the limit has to be around that, not the ~200 MB of real usage. Capping real memory is not reliable, only trhorugh cgroup, which needs special permision in containers.

Measured peak virtual memory of the heaviest honest mainnet contract:

arch / cores VmPeak
arm64 ~1.6 GB
amd64, 16 cores 1.77 GB
amd64, 128 cores (unpinned) 1.93 GB
any, pinned to GOMAXPROCS=2 ~1.58 GB

4 GB sits conformably above that peak.

Pinning the compile child to GOMAXPROCS=2

A 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 process_resident_memory_bytes, which counts the node's whole footprint together: Go heap, pebble (DB) cache, state, contract execution and RPC.

RSS
steady state ~1.5 GB
peak over 24h ~2.25 GB
worst over 7d ~2.86 GB

4 GB covers the peak with headroom.

Validation

Checked 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants