Avoid syscall overhead in polling rss usage#95
Conversation
edwintorok
left a comment
There was a problem hiding this comment.
proc_pid_status(5) and proc_pid_statm(5) say that VmRSS is inaccurate.
Accurate values can be read from /proc/pid/smaps or /proc/pid/smaps_rollup, although these are slower.
Since you have now reduced the frequency of these reads, should we (eventually) add an option to use the more accurate statistics instead? (which should probably read both, so we can see whether the inaccuracy matters for us or not)
edwintorok
left a comment
There was a problem hiding this comment.
Looks good, some minor observations.
This is a good point! I'll make an issue of this. |
| } | ||
| in | ||
| let child = launch_process runtime_config exec_args in | ||
| config.on_launch child; |
There was a problem hiding this comment.
The Fun.protect ~finally:child.close should live inside starts on the next line. If Rss_poller.start raises (say Domain.spawn failing) the cursor is never freed and the child keeps running.
tmcgilchrist
left a comment
There was a problem hiding this comment.
On Linux, the polling is unnecessary, the kernel already tracks the peak.
This is worth deciding on rather than inheriting. Reading max_rss_stubs.c, the three platforms return fundamentally different things:
| Platform | Field | Semantics |
|---|---|---|
| Linux (:59) | VmHWM | peak RSS, kernel-maintained, monotone |
| macOS (:79) | pti_resident_size | current RSS |
| FreeBSD (:96) | ki_rssize | current RSS |
On Linux, VmHWM never decreases, so a single sample taken any time after the peak yields the exact answer, and the max rss peak fold is redundant. The "up to 50% of time within the callback" cost the PR is fixing buys literally nothing there — the fix is fewer samples, not a dedicated domain. Only macOS and FreeBSD genuinely need to catch the peak by sampling, and there the interval is an accuracy/overhead tradeoff. I measured that side on macOS with a child having a well-defined ~800 MB peak:
--rss-freq=0.001 : 784192 kB 784192 kB
--rss-freq=0.1 : 784128 kB 784128 kB (default — accurate)
--rss-freq=1.0 : 702416 kB 784128 kB (~10% low on one run)
So the default is well chosen, but the flag's documentation should say what it actually trades: on Linux, only trailing latency; on macOS/FreeBSD, the accuracy of the peak. As written it reads as if turning it down is free and turning it up is free, and neither is true.
| Arg.( | ||
| value | ||
| & opt float 0.1 (* Sample at 10Hz by default. *) | ||
| & info [ "rss-freq" ] ~docv:"rss-freq" ~doc) |
There was a problem hiding this comment.
Add validation on the interval. --rss-freq=0 and --rss-freq=-1 are both accepted and both busy-spin a domain at 100% CPU.
Reject interval <= 0 here, either an Arg.conv that returns `Error below zero, or aTerm.ret check or clamp to a sane floor.
|
|
||
| let stop t = | ||
| Atomic.set t.stop_flag true; | ||
| Domain.join t.domain |
There was a problem hiding this comment.
stop blocks for up to a full interval, making Ctrl-C look like a hang. stop sets stop_flag and immediately Domain.joins, but the poller is almost always parked in Unix.sleepf interval on line 14 and only re-checks the flag after waking. So this join waits out whatever is left of the current sleep.
There was a problem hiding this comment.
At the default frequency I see this as acceptable. If not, then a select on a pipe with timeout can be used plus a write from the stop function, but I'm not sure whether this works on Windows.
| else begin | ||
| (* sample before waiting so that we get a reading at launch *) | ||
| let rss = get_rss_kb pid in | ||
| (try Unix.sleepf interval |
There was a problem hiding this comment.
Unguarded sleepf busy-spins on interval <= 0. collect_events guards its sleep with if poll_sleep > 0.0 then ...; this one doesn't.\nUnix.sleepf never raises for the degenerate inputs, it just returns immediately.
Currently, RSS is polled once every time the events callback fires. When the event rate is very high this leads to a high percentage of CPU usage in getting the RSS stat (up to 50% of time within the callback).
Using a separate domain that periodically polls RSS can be more accurate (since we are not waiting for an event to sample) plus the frequency can be configurable with a low enough default (10Hz) so that it does not impact event collection.
NB stacked on top of #93