runtime: support native CPU profiling - #2339
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: CPU profiling for native LLGo executables
This is a well-engineered change. The separation of the async-signal-unsafe sampling path (C, profile.c) from the Go drain path is sound, the ring buffer uses correct atomic/lock discipline (handler uses non-blocking try_lock only, so no self-deadlock; double-checks active after acquiring the lock to race correctly against stop()), errno is saved/restored on every entry point per the existing fault.c convention, and the pprof record stream format matches runtime/pprof.readProfile (header {3,0,hz}, sample {3+n,0,1,pcs...}, lost {4,0,0,lost} — all verified against upstream addCPUData). The frame-pointer walk is defensively bounded (alignment, llgo_mem_readable probe, monotonic FP stride, ret < 4096 guard).
Findings are inline. Highest priority: the msync-per-frame probe inside the SIGPROF handler, and confirming the case 0 retry contract.
Other notes (no reliable inline location)
- Test platform gating (
test/std/runtime/pprof/pprof_test.go): the new tests assert the profile containscpuProfileHotLoop. On platforms whereSetCPUProfileRateis the no-op stub (wasm/baremetal/unsupported arch),readProfilereturnseof=truewith only the period record, sorequireCPUProfileContainswill fail. If CI only runs these on darwin/linux amd64/arm64 this is fine; otherwise consider a matching//go:buildconstraint ort.Skip. The tests are also timing-dependent (500ms/300ms hot loops) — a documented flakiness expectation ortesting.Short()guard may help on loaded CI. - Magic numbers (
cpuprof_sigprof_llgo.go): the drain cap256(appears twice) and preallocation sizes1024/16are unexplained. Consider named constants tied toLLGO_PROF_SAMPLESwith a one-line rationale.
| while (fp != 0 && sample->n < LLGO_PROF_STACK) { | ||
| uintptr_t prev, ret; | ||
| if ((fp & (word - 1)) != 0 || | ||
| !llgo_mem_readable((void *)fp) || |
There was a problem hiding this comment.
[P1] msync() per frame in SIGPROF handler: signal-unsafe + costly
The SIGPROF handler's frame walk calls llgo_mem_readable() up to twice per frame, and each call issues an msync(page, 1, MS_ASYNC) syscall (see fault.c:112).
Signal-safety: msync is not on the POSIX signal-safety(7) async-signal-safe list. SIGPROF interrupts arbitrary code, so this is technically undefined behavior. In practice msync is a thin syscall wrapper and the kernel VMA lock is not held by the interrupted user thread, so real-world risk is low — but the file header comment (lines 3-5) claims the handler does not call into Go while being silent on this syscall dependency. Worth documenting as a deliberate trade-off.
Performance: with LLGO_PROF_STACK == 64, a single sample can execute up to ~128 syscalls, all inside the signal handler. At the default 100 Hz that is up to ~12,800 syscalls/sec purely for readability probes, scaling with stack depth and with hz (the API allows up to 1,000,000). The profiler thus perturbs exactly the deep-stack CPU-bound workloads it is meant to measure. Unlike fault.c, which probes once on a dying path, this runs on every steady-state tick. Consider caching the last known-good readable page range across frames within a walk, or a lower sampling frame cap.
Also: there is a TOCTOU gap between the llgo_mem_readable check and the *(uintptr_t*)fp dereference — a concurrent munmap/mremap could still fault. The window is tiny and inherent to unsynchronized frame walking, but a fault here (delivered while SIGPROF processing is active) risks recursing rather than recovering.
| cpuProfilePeriodRecord[2] = uint64(hz) | ||
| latomic.StoreUint32(&cpuProfilePeriodPending, 1) | ||
| latomic.StoreInt32(&cpuProfileRate, int32(hz)) | ||
| case 0: |
There was a problem hiding this comment.
[P2] case 0 clears cpuProfileOpen while old samples may remain in ring
The comment says "Samples from a stopped profile have not been drained yet," yet this path stores cpuProfileOpen = 0, re-permitting a fresh StartCPUProfile immediately. On the C side llgo_cpu_profile_start returns 0 precisely when read_index != write_index (ring not drained) and does not reset the ring indices. If a caller retries StartCPUProfile, the next successful start would begin with leftover samples from the previous profile still in the ring, and those stale PCs would be attributed to the new profile.
Please confirm the intended contract: either keep cpuProfileOpen held until the drain completes, or have the retry path discard the residual ring. At minimum, expand the comment to explain why clearing cpuProfileOpen here is safe despite undrained samples.
| @@ -0,0 +1,15 @@ | |||
| //go:build (darwin || linux) && (baremetal || (!amd64 && !arm64)) | |||
There was a problem hiding this comment.
[P2] readProfile linkname not defined for wasm builds
The //go:linkname runtime_pprof_readProfile runtime/pprof.readProfile symbol is provided in exactly two files:
- impl (
cpuprof_sigprof_llgo.go):!baremetal && !wasm && (darwin || linux) && (amd64 || arm64) - this stub:
(darwin || linux) && (baremetal || (!amd64 && !arm64))
The union is (darwin || linux) && ... — wasm satisfies neither. Meanwhile the sibling stubs SetCPUProfileRate (cpuprof_stub_llgo.go, tag includes wasm) and pprof_runtime_stub_llgo.go (darwin || linux || wasm) do cover wasm. So a wasm build importing runtime/pprof would resolve every other pprof linkname but leave runtime/pprof.readProfile undefined. This looks like an unintended exclusion. Consider widening this stub to also cover wasm, e.g. wasm || ((darwin || linux) && (baremetal || (!amd64 && !arm64))).
| data = append(data, uint64(pcs[i])) | ||
| } | ||
| tags = append(tags, nil) | ||
| if records == 255 { |
There was a problem hiding this comment.
[P3] if records == 255 break is load-bearing but non-obvious
The loop header for records := 0; n != 0 && records < 256; records++ already bounds the loop, so this inner break looks redundant. It is not: without it, the 256th iteration (records==255) would still run the trailing c_cpuProfileRead below, consuming a sample the loop then discards — silently dropping one sample per drain chunk. A future reader may "simplify" it away. Add a short comment (e.g. "don't consume a sample we won't emit this chunk"), or restructure so the read happens at the top of the loop guarded by capacity, removing the special case.
| // Linux's profile writer expects readProfile to block. Keep the wait | ||
| // out of the signal path and poll at the 100 Hz profiler's period; | ||
| // Darwin already sleeps in pprof. | ||
| c.Usleep(10000) |
There was a problem hiding this comment.
[P3] Poll comment implies interval tracks configured hz
10000 usec = 10 ms = 100 Hz period, so the arithmetic is internally consistent. But the profiler rate is user-configurable (1..1,000,000 Hz); 100 Hz is only the default. The 10 ms poll is a fixed constant regardless of the actual hz, so "poll at the 100 Hz profiler's period" reads as if the interval adapts to the configured rate. Suggest wording like "poll at 10 ms (the default 100 Hz period)".
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Compared with |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
6e4d796 to
086d454
Compare
a48cd70 to
0649012
Compare
1 participant
Summary
ITIMER_PROF/SIGPROFon Darwin and Linux amd64/arm64runtime/pprofos/signalwatcher across Notify, Stop, Ignore, and Resetsignal.Notify(SIGPROF)registration is restored after profiling stopsValidation
go test -vet=off ./test/std/runtime/pprof ./test/std/os/signalruntime/pprofandos/signalcompatibility packages; lifecycle tests repeated five timesgo tool pprof -topparsed LLGo profiles on Darwin and Linux; the synthetic hot loop accounted for 93.75% and 95% flat CPU respectivelyScope
This PR covers native executables on Darwin/Linux amd64/arm64. Profile labels, inline-frame expansion, richer C-stack unwinding, and c-archive/c-shared integration remain follow-up work.