[Perf] Add first warmup config to perf dispatch

[hughperkins]

Issue: #

Brief Summary

• add first warmup, defaulting to 1

• also changes default warmup from 3 to 0

  Summary

  Splits the perf-dispatcher warmup into two distinct phases:

  • first_warmup — warmup iterations for the initial evaluation cycle of a given geometry, when kernels still need JIT compilation. Defaults to 1.
  • warmup — warmup iterations for subsequent re-evaluation cycles, when kernels are already compiled. Defaults to 0.
    Previously there was a single NUM_WARMUP = 3 used for every evaluation cycle. The new defaults significantly reduce the per-call overhead of re-evaluation (warmup drops from
    3 → 0) while still giving the first cycle a JIT-warmup buffer.

  Changes

  python/quadrants/lang/_perf_dispatch.py

  • New module constant NUM_FIRST_WARMUP = 1; NUM_WARMUP changed from 3 → 0.
  • New first_warmup parameter on the perf_dispatch decorator and PerformanceDispatcher.__init__.
  • New _first_eval_completed: set[int] to track which geometries have finished their initial evaluation.
  • New _get_effective_warmup(geometry_hash) — returns num_first_warmup before the first evaluation completes, num_warmup afterward.
  • _compute_are_trials_finished now takes a num_compatible argument and requires that every compatible implementation has at least one entry in trial_counts before
    declaring trials finished. This fixes a bug where, with warmup=0, the check could fire after only one impl had run.
  • Updated docstring for perf_dispatch to document both parameters.

  tests/python/test_perf_dispatch.py

  • NUM_WARMUP import replaced with NUM_FIRST_WARMUP.
  • Existing tests now pin first_warmup=WARMUP, warmup=WARMUP (local WARMUP=3) so they continue to exercise the same behaviour as before.
  • Two new tests:
    • test_perf_dispatch_first_warmup_vs_warmup — verifies that the first evaluation uses first_warmup and subsequent re-evaluations use warmup.
    • test_perf_dispatch_default_warmup_values — verifies the new defaults (1 / 0) produce the expected call counts.

  Good points

  • Reduces steady-state overhead. With the old defaults, every re-evaluation triggered 3 warmup calls per implementation; the new default of 0 removes that cost entirely
    once kernels are compiled.
  • Keeps JIT warmup where it matters. The first evaluation cycle still gets a warmup pass, so the initial timing measurement is not polluted by compilation time.
  • Fixes a real bug. The num_compatible check in _compute_are_trials_finished closes a hole where, with warmup=0, evaluation could be declared finished before all
    compatible implementations had been measured — meaning the "fastest" choice could be made from incomplete data.
  • Backwards-compatible API surface. first_warmup is a new keyword-only argument with a sensible default; existing callers don't need to change unless they want to.
  • Test coverage for the new behaviour. Both the parameter split and the new defaults are covered explicitly.

  Bad / risky points

  • Default behaviour change is silent. Existing callers that did not pass warmup= were getting 3 warmup iterations per cycle and now get 0. Any caller that was implicitly
    relying on warmup runs to settle caches / hide first-call jitter will now see noisier measurements. There is no deprecation path.
  • first_warmup=1 may be too low for some kernels. A single warmup is enough to pay JIT cost, but not enough to amortise things like first-touch allocation or autotuning
    inside the kernel. Workloads that previously got 3 warmups for the first cycle may now make a fastest-impl decision based on a less-stable measurement.
  • Semantics of "trials finished" subtly changed even when warmup > 0. The new len(trial_counts) >= num_compatible requirement is correct, but it's a behavioural change
    that affects every caller, not just the warmup=0 case. Worth flagging in case any downstream code depends on the old timing.
  • Naming. first_warmup vs warmup reads a bit oddly — warmup no longer means "warmup" in the obvious sense, it means "warmup on re-evaluation". Names like
    initial_warmup / recheck_warmup (or warmup / recheck_warmup) might be clearer. Worth a quick bikeshed before merge.
  • Per-geometry state grows. _first_eval_completed is another set keyed by geometry hash, alongside the existing per-geometry dicts. Minor, but worth noting for
    long-running processes that see many distinct geometries.
  • CI noise in history. The branch contains two empty "retrigger flaky CI" commits and two merge commits from main. Consider squashing on merge to keep main's history
    clean.

copilot:summary

Walkthrough

copilot:walkthrough

[hughperkins]

this local var should be in lowercase

[hughperkins]

lets test a non-zero warmup

[hughperkins]

should have atest with non-zero

[hughperkins]

also, should check something like active + warmup == num calls to non-chosen kernel

[hughperkins]

what is the purpose of such large, and identcal, warmups

[hughperkins]

(note to self: self-review completed; need to address the comments)

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e2edd39598

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Keep warmup affecting initial evaluation by default

Setting first_warmup to a fixed default of 1 while keeping warmup separate changes existing @perf_dispatch(..., warmup=N) behavior: unless callers also pass first_warmup, the initial benchmark cycle now uses only one warmup iteration. In configurations that do not re-evaluate (for example repeat_after_seconds=0 or short-lived runs), the caller’s warmup value is effectively ignored, which can pick a fastest implementation from noisier timings and regress performance. Consider making first_warmup inherit warmup when it is not explicitly provided to preserve prior semantics.

Useful? React with 👍 / 👎.

[claude]

🟡 The local variable WARMUP = 3 inside test_perf_dispatch_kernels, test_perf_dispatch_python, and test_perf_dispatch_kernel_py_mix uses UPPER_CASE, which PEP 8 reserves for module-level constants. It should be renamed to warmup = 3 in all three functions.

Extended reasoning...

What the bug is: PEP 8 specifies that UPPER_CASE names are reserved for module-level constants (e.g. NUM_WARMUP, NUM_FIRST_WARMUP). A variable named WARMUP = 3 declared at function scope inside a test function does not meet this criterion — it is a local variable that should use lowercase (warmup = 3).

Where it appears: The same pattern occurs in three test functions introduced by this PR:

• test_perf_dispatch_kernels (line 35): WARMUP = 3
• test_perf_dispatch_python (~line 121): WARMUP = 3
• test_perf_dispatch_kernel_py_mix (~line 207): WARMUP = 3

In each case the variable is used purely locally within that function to pass first_warmup=WARMUP, warmup=WARMUP and to drive loop bounds and assertions.

Why existing code doesn't prevent it: Python itself does not enforce naming conventions; the violation is purely stylistic. Linters such as pylint (C0103) or ruff (N806) would flag it, but if those checks are not run in CI or are not configured to catch this pattern the code silently ships with the inconsistency.

Impact: No functional impact — the tests behave correctly regardless of the variable's name. The concern is readability and consistency: a reader skimming the file sees WARMUP and may assume it is an imported or module-level constant (like NUM_FIRST_WARMUP used elsewhere in the same file), potentially causing confusion.

How to fix: Rename the local variable to warmup (lowercase) in all three functions and update the six references within each function (first_warmup=warmup, warmup=warmup, loop bounds range((warmup + 5)), comparisons if it <= warmup, assertions assert trials == warmup + 1).

Proof: The PR author themselves flagged this exact issue at line 35 in their inline self-review comment: "this local var should be in lowercase". The same pattern is replicated identically in the two subsequent test functions, so the fix needs to be applied three times.

Addressing the refutation: One verifier argued this is too minor to flag and that UPPER_CASE for a local constant-like variable is a common pattern. While the functional impact is zero, the PR author's own self-review comment demonstrates awareness that this is an error to be corrected — not an intentional stylistic choice. Reporting it as a nit is appropriate precisely because it is minor; it does not need to block the merge but should be addressed before landing.

[claude]

🟣 _get_min_trials_finished (lines 221–222) is dead code that is never called anywhere in the codebase; this is a pre-existing issue unrelated to this PR's logic. If it were ever invoked on a geometry with no trial data it would raise ValueError: min() arg is an empty sequence. The PR's refactor of _compute_are_trials_finished was a natural opportunity to remove it.

Extended reasoning...

What the bug is and how it manifests

The method _get_min_trials_finished(self, geometry_hash) at lines 221–222 of _perf_dispatch.py simply wraps a bare min() over the values of the per-geometry trial-count dict. Because min() on an empty sequence raises ValueError, any caller that passes a geometry hash whose dict has not yet been populated would crash. More importantly, no caller exists at all — the method is unreachable dead code.

The specific code path that triggers it

A grep across the entire repository finds _get_min_trials_finished mentioned only at its definition site. Neither _compute_are_trials_finished nor any other method or test invokes it. The method is therefore completely unreachable in the current codebase.

Why existing code does not prevent it

_compute_are_trials_finished has always inlined its own min() call — even before this PR. The PR reinforced that pattern by adding the num_compatible guard and the _get_effective_warmup() indirection, again inlining min() directly. At no point in the history of this file was _get_min_trials_finished wired up as the shared helper it appears to have been intended as.

What the impact would be

Because the method is never called, the ValueError risk is theoretical rather than practical. The real impact is code-quality: a reader seeing _get_min_trials_finished alongside _get_effective_warmup and _compute_are_trials_finished might assume it is called somewhere or plan to call it, potentially without noticing the missing empty-sequence guard. This creates confusion about the correct computation pattern.

How to fix it

Simply delete the _get_min_trials_finished method. All call sites that would logically need it already inline min() with a proper empty-sequence guard (via the len(trial_counts) < num_compatible early-return in _compute_are_trials_finished).

Step-by-step proof that the method is dead

1. Open _perf_dispatch.py. The only occurrence of the string _get_min_trials_finished is the definition at line 221.
2. Search the entire repository for _get_min_trials_finished — zero results outside the definition.
3. Trace _compute_are_trials_finished: it accesses trial_counts = self._trial_count_by_dispatch_impl_by_geometry_hash[geometry_hash], guards with len(trial_counts) < num_compatible, then calls min(trial_counts.values()) directly. _get_min_trials_finished is bypassed.
4. No test, no other module, no internal call site references _get_min_trials_finished. The method is provably unreachable.

[erizmr]

Approving on the basis that I have reviewed the design and the public facing API, tests, and they look reasonable to me.

[hughperkins]

Thanks!