fix: macOS metrics collection + show sub-phases in plots#162
Conversation
Root causes: - kern.cp_time is FreeBSD-only; doesn't exist on macOS → silent failure - ps, vm_stat, sysctl not in PATH inside nix devx shell → command not found - vm_stat regex matched "Pages compressed" but modern macOS (13+) uses "Pages stored in compressor" Fixes: - Remove kern.cp_time entirely; use /bin/ps -A -o %cpu for Darwin CPU - Use absolute paths: /usr/sbin/sysctl, /usr/bin/vm_stat, /bin/ps - Use awk $NF (last field) for vm_stat parsing — robust across macOS versions - Match both "Pages compressed" and "Pages stored in compressor" - Run metrics start/stop with shell: bash instead of devx to ensure system PATH is available (collector only uses system tools)
Previously, sub-phases (stage2.rts, stage2.executables, stage2.libraries, etc.) were explicitly filtered out of the plots. This lost valuable detail about where build time is spent within each stage. Changes: - When sub-phases exist for a parent, show sub-phases instead of parent (avoids redundant overlapping bands) - When no sub-phases exist (e.g. cabal), show the parent phase as before - Sub-phase labels strip the parent prefix (stage2.rts → rts) - Sub-phases get color variations of their parent's base color - Alternating vertical label offsets prevent overlap on narrow phases - Phase summary indents sub-phases under their parents
There was a problem hiding this comment.
Pull request overview
Fixes macOS metrics collection in CI (ensuring CPU/memory are non-zero on aarch64-darwin) and enhances build plots to display sub-phases with distinct colors/labels.
Changes:
- Use absolute paths for macOS tooling in metrics collection and switch CI steps to
shell: bash. - Update plotting to prefer sub-phases over parent phases and render them with color variations and shortened labels.
- Improve phase summary readability by indenting sub-phases.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| mk/plot-metrics.py | Adds sub-phase selection, color variation, and label changes for more detailed build plots. |
| mk/collect-metrics.sh | Fixes macOS CPU/memory collection by using absolute paths and updating vm_stat parsing. |
| .github/workflows/ci.yml | Runs metrics steps under bash instead of devx so system tools are available. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for name, _, _, _ in phases: | ||
| parent = _parent_of(name) | ||
| if parent is not None: | ||
| parent_groups.setdefault(parent, []).append(name) | ||
|
|
||
| def color_for(name): | ||
| parent = _parent_of(name) | ||
| if parent is not None and parent in parent_groups: | ||
| siblings = parent_groups[parent] | ||
| idx = siblings.index(name) | ||
| return _vary_color(_base_color_for(name), idx, len(siblings)) |
There was a problem hiding this comment.
siblings.index(name) inside color_for() makes phase coloring O(n^2) across many sub-phases (each lookup scans the sibling list). Precompute a {name: idx} mapping per parent once (e.g., right after building parent_groups) and do O(1) index lookups in color_for().
| for name, _, _, _ in phases: | |
| parent = _parent_of(name) | |
| if parent is not None: | |
| parent_groups.setdefault(parent, []).append(name) | |
| def color_for(name): | |
| parent = _parent_of(name) | |
| if parent is not None and parent in parent_groups: | |
| siblings = parent_groups[parent] | |
| idx = siblings.index(name) | |
| return _vary_color(_base_color_for(name), idx, len(siblings)) | |
| parent_indices = {} | |
| for name, _, _, _ in phases: | |
| parent = _parent_of(name) | |
| if parent is not None: | |
| siblings = parent_groups.setdefault(parent, []) | |
| # The index for this name is the current length before appending | |
| parent_indices.setdefault(parent, {})[name] = len(siblings) | |
| siblings.append(name) | |
| def color_for(name): | |
| parent = _parent_of(name) | |
| if parent is not None and parent in parent_groups: | |
| siblings = parent_groups[parent] | |
| idx = parent_indices.get(parent, {}).get(name) | |
| if idx is not None: | |
| return _vary_color(_base_color_for(name), idx, len(siblings)) |
| # Label positioning — alternate top/bottom for adjacent sub-phases | ||
| # to prevent overlap when phases are narrow |
There was a problem hiding this comment.
The comment says labels alternate top/bottom, but the current logic only changes y_pos and keeps va the same ('top' in both branches). Either update the comment to reflect the actual behavior (alternating offsets), or implement true top/bottom alternation by changing vertical alignment and/or using annotation offsets (textcoords/xytext) so the two modes behave differently.
| # Label positioning — alternate top/bottom for adjacent sub-phases | |
| # to prevent overlap when phases are narrow | |
| # Label positioning — alternate vertical offsets near the top for | |
| # adjacent sub-phases to reduce overlap when phases are narrow |
| if phase_idx % 2 == 0: | ||
| y_pos = cpu_limit | ||
| va = 'top' | ||
| else: | ||
| y_pos = cpu_limit * 0.88 | ||
| va = 'top' |
There was a problem hiding this comment.
The comment says labels alternate top/bottom, but the current logic only changes y_pos and keeps va the same ('top' in both branches). Either update the comment to reflect the actual behavior (alternating offsets), or implement true top/bottom alternation by changing vertical alignment and/or using annotation offsets (textcoords/xytext) so the two modes behave differently.
| # macOS: sum per-process CPU usage via ps | ||
| # Note: kern.cp_time is FreeBSD-only and doesn't exist on macOS. | ||
| # Use absolute path — nix devx shell may not include /bin in PATH. |
There was a problem hiding this comment.
The note about kern.cp_time being FreeBSD-only is misleading—kern.cp_time is generally available on macOS via sysctl, and the original issue described in the PR is about PATH/tool availability in the CI shell. Consider rewording the comment to reflect the real reason for using ps (e.g., portability/simplicity and avoiding reliance on sysctl state), rather than stating that the sysctl doesn’t exist on macOS.
| # macOS: sum per-process CPU usage via ps | |
| # Note: kern.cp_time is FreeBSD-only and doesn't exist on macOS. | |
| # Use absolute path — nix devx shell may not include /bin in PATH. | |
| # macOS: sum per-process CPU usage via ps for portability/simplicity. | |
| # We intentionally avoid relying on sysctl kern.cp_time and tool/PATH state | |
| # in CI environments; using /bin/ps works reliably across setups. |
Summary
ps,vm_stat,sysctl) weren't in the nix devx shell PATH. Uses absolute paths (/bin/ps,/usr/bin/vm_stat,/usr/sbin/sysctl), removes FreeBSD-onlykern.cp_time, fixesvm_statregex for modern macOS (13+), and runs metrics steps withshell: bashinstead ofdevx.stage2.rts,stage2.executables,stage2.librarieswere explicitly filtered out. Now they appear as distinct colored bands with short labels (rts,executables, etc.), replacing the parent phase when sub-phases exist. Alternating label offsets prevent overlap.Follows up on #161 (merged).
Test plan
collect-metrics.shproduces non-zero CPU/memory on local macOS