perf(benchmark): gate on Bazel analysis phase and report per-function Starlark cost#1316
perf(benchmark): gate on Bazel analysis phase and report per-function Starlark cost#1316xangcastle wants to merge 12 commits into
Conversation
✨ Aspect Workflows Tasks📅 Fri Jul 17 18:25:30 UTC 2026 ✅ 40 successful tasks
⏱ Last updated Fri Jul 17 18:29:41 UTC 2026 · 📊 GitHub API quota 1,245/15,000 (8% used, resets in 11m) |
271921a to
6db41ca
Compare
Bazel analysis benchmark
|
6db41ca to
38e531d
Compare
38e531d to
94513f0
Compare
tamird
left a comment
tamird
left a comment
There was a problem hiding this comment.
The analysis-phase gate is useful; I found one issue that makes the per-function comparison report false new regressions.
… Starlark cost Rewrite the benchmark to gate on Bazel's analysis phase (runAnalysisPhase extracted from --profile) instead of process wall time, excluding JVM/IO noise. The two separate workspaces are merged into a single parameterized one where --packages drive both the analysis load and a fan-in py_binary whose venv scales with it, and the PR comment now surfaces a per-function Starlark CPU breakdown (via --starlark_cpu_profile) so that when something regresses it tells you which functions got slower not just that it did.
94513f0 to
1fcb6bc
Compare
tamird
left a comment
tamird
left a comment
There was a problem hiding this comment.
The refreshed aggregation fixes the false-new report, but the new significance filter can now hide real per-function regressions and still has no cutoff regression test. I also found unrelated root lockfile churn introduced by the latest commit.
0808d64 to
750747b
Compare
tamird
left a comment
tamird
left a comment
There was a problem hiding this comment.
The latest profile-path change reintroduces sampling bias for intermittently observed functions. The earlier significance and remaining root-lockfile findings still apply, and the requested aggregation/comparator regressions are still absent.
tamird
left a comment
tamird
left a comment
There was a problem hiding this comment.
One additional significance issue: main and PR profiles can contain different numbers of usable Starlark runs, so their standard errors cannot share one sample count.
se>0 treating deterministic regressions as no-signal Conditional mean / dropped runs Single n for both sides
tamird
left a comment
tamird
left a comment
There was a problem hiding this comment.
The new revision fixes zero-filled sparse functions, per-function significance, deterministic movers, and unequal sample counts. Three attribution/aggregation gaps remain, and none of the requested statistical regressions were added. Please add focused cases for sparse and fully empty runs, cancelling speedups, zero variance, unequal counts, duplicate function names across files, and builtin caller attribution so later changes cannot silently reintroduce these diagnostics errors.
| values = _parse_packed_or_singles(chunk, 2) | ||
| if not loc_ids or val_idx >= len(values): | ||
| continue | ||
| leaf = loc_ids[0] |
There was a problem hiding this comment.
This attributes every sample to the leaf, including builtins, while the rendered report says builtins are attributed to the caller. In real profiles alias/config_setting/glob therefore dominate the movers table instead of identifying actionable Starlark code. Walk outward through loc_ids to the first non-builtin frame, and cover a builtin-leaf/caller sample.
| if fid is None: | ||
| continue | ||
| name = s(fn_name.get(fid)) | ||
| totals[name] += values[val_idx] / divisor |
There was a problem hiding this comment.
Keying totals/files only by the short function name merges unrelated functions such as _impl from different .bzl files and keeps whichever filename was seen first. That can create false movers and point reviewers at the wrong source. Preserve normalized (file, function) identity through aggregation/comparison and add a same-name/different-file regression.
| star_path if star_enabled else None, cwd=run_cwd) | ||
| analysis_us.append(a) | ||
| wall_ms.append(w) | ||
| if star: |
There was a problem hiding this comment.
A valid decoded profile with zero samples is {}, so this drops the entire measured run. The sparse-function zero-fill above still computes a conditional mean, understates variance, and reports the wrong run count whenever one run is fully empty. Append the empty observation whenever Starlark profiling is enabled (and distinguish an actual decode failure), with a fully empty-run regression. The nearby star_runs/run_once annotations also still say dict[str, float], but the decoder returns dict[str, tuple[float, str]].
| pr_total_ms = pr_total.get("mean_ms", 0.0) | ||
| delta_total = pr_total_ms - main_total_ms | ||
| pct_total = pct(main_total_ms, pr_total_ms) if main_total_ms else 0.0 | ||
| total_se = _combined_se(main_total.get("stddev_ms", 0.0), main_n, |
There was a problem hiding this comment.
total_se is now calculated but never used after removing the total gate. Please delete the dead calculation; the per-function standard errors below are the values that matter.
…filing # Conflicts: # .github/workflows/performance-benchmark.yml
2 participants
Rewrite the benchmark to gate on Bazel's analysis phase (runAnalysisPhase extracted from --profile) instead of process wall time, excluding JVM/IO noise. The two separate workspaces are merged into a single parameterized one where --packages drive both the analysis load and a fan-in py_binary whose venv scales with it, and the PR comment now surfaces a per-function Starlark CPU breakdown (via --starlark_cpu_profile) so that when something regresses it tells you which functions got slower not just that it did.
Changes are visible to end-users: no
Test plan