Skip to content

Add performance-fixer skill + make the benchmark harness AI-ready#4361

Merged
mattleibow merged 4 commits into
mainfrom
mattleibow-perf-fixer-skill
Jul 8, 2026
Merged

Add performance-fixer skill + make the benchmark harness AI-ready#4361
mattleibow merged 4 commits into
mainfrom
mattleibow-perf-fixer-skill

Conversation

@mattleibow

@mattleibow mattleibow commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

What

Adds a performance-fixer skill — a periodic AI scanner that finds a hot-path performance opportunity in the managed C# binding, proves it, fixes it (managed-only, ABI-safe), and opens a linked issue + draft PR — plus source changes that make the benchmark harness easy for that skill (and humans) to write and run benchmarks against.

Modelled on the existing memory-leak-fixer skill and on a colleague's optimize-aspnetcore-performance skill, and grounded in our real perf PRs:

PR Win Lesson encoded
#4241 SKMatrix native→managed, up to 24× bit-exact port + the x86/x87 native fallback
#4345 SKColor alloc-free parse, 1.4–2.3× span parser + additive ReadOnlySpan overload
#4247 SKSurface.Canvas caching cache a stable native wrapper (4 preconditions)

The defining discipline: two proofs

Unlike a generic perf pass, every fix must be shown faster (a BenchmarkDotNet New vs Old measurement) and behaviour-identical (an equivalence test — bit-exact vs SkiaApi.sk_* for numeric ports). A faster answer that differs from Skia is a rendering regression, not a win. The skill also hard-codes the traps we learned the hard way: the x87 float-determinism fallback, the ARM64 Vector256 5.7–6.5× regression, benchmark statistical rigor, and full behaviour parity (exceptions / ownership / GC.KeepAlive / rendered pixels).

Structure

References are split by area (the aspnetcore pattern): hot-paths/ (SkiaSharp code areas) + bcl-patterns/ (.NET techniques), plus decision-framework.md (impact×complexity rubric), measuring.md, signals.md (detection routing), and repo-helpers.md. A lean router SKILL.md ties them together.

.agents/skills/performance-fixer/
├── SKILL.md
└── references/
    ├── decision-framework.md · measuring.md · signals.md · repo-helpers.md
    ├── hot-paths/     geometry-math · color · handles-and-collections · text-and-fonts · pixels-and-images
    └── bcl-patterns/  strings-and-spans · numerics-and-simd · memory-and-buffers · collections · interop-and-marshalling

A gh-aw workflow (.github/workflows/performance-fixer.md) runs the skill on a schedule (every 12h, dry-run on PR, focus-area knob), compiled to .lock.yml.

Benchmark harness: ready in the source

So a skill run never wastes time repairing infra:

  • Replaced TheBenchmark.csTemplateBenchmark.cs. The old template was a trap: [SimpleJob(Net48/Net70/Net80)] silently skips on macOS/Linux (verified) and its empty bodies taught none of the conventions. The new one is a working New vs Old + [MemoryDiagnoser] + [Params] + return-sink skeleton that runs out of the box (verified: Old 88/1048 B → New 0 B).
  • Added benchmarks/README.md — the on-ramp: how to add/run a benchmark, --list flat for discovery and --job short for fast iteration, the single-TFM warning, the New-vs-Old + sink conventions, and the InternalsVisibleTo native-oracle note.

Labels

Every finding is labelled tenet/performance (the quality-tenet umbrella) plus one agent-chosen perf/* sub-type, decided by the win's dominant measured driver (a removed P/Invoke → perf/interop, removed managed allocations → perf/allocations, etc.). This aligns to the canonical taxonomy in .agents/skills/issue-triage/references/labels.md (#4362) and matches the memory-leak-fixer convention, so agentic perf findings triage the same way as human-filed ones.

Verification

  • Skill passes quick_validate; all cross-reference links resolve; the gh-aw workflow compiles with 0 errors / 0 warnings.
  • Smoke-tested the harness end-to-end: externals-download → built the benchmark project → ran SKColorParseBenchmark's full New-vs-Old comparison and the new TemplateBenchmark (clean, Ratio/Allocated populated).
  • Two GPT-5.5 review passes drove fixes (x87 fallback, benchmark rigor, StringUtilities vs Utils naming, TFM guards).

Validated end to end on real CI

The workflow self-tests on any PR touching the skill via a forced dry-run. Those runs demonstrated the full range of correct behaviour:

  • Correctly rejected a non-findingSKFourByteTag.Parse's char[4] is already stack-allocated by .NET 9/10 escape analysis, so it emitted a noop rather than a bogus "removes per-frame allocation" PR.
  • Found real, fully-proven winsSKShaper.Shape throwaway glyph-array extraction (~30% faster, 55–71% fewer allocations → perf/allocations) and the SKColorSKColorF operator P/Invoke (→ perf/interop), each with a New-vs-Old benchmark run twice and a mutation-checked bit-exact equivalence test.
  • A real dispatch run created correctly-labelled artifacts — issue [performance] Port SKColor -> SKColorF conversion from native P/Invoke to managed C# #4369 and draft PR [performance] Port SKColor -> SKColorF conversion to managed C# #4370 (the SKColorSKColorF port), both carrying tenet/performance + perf/interop, confirming the labels land on the issue and the PR.

Notes

Co-authored-by: Copilot App 223556219+Copilot@users.noreply.github.com

Add a `performance-fixer` skill that periodically scans the managed C#
binding for a hot-path performance opportunity, proves it with a
BenchmarkDotNet New-vs-Old measurement AND a behaviour-parity test, fixes
it (managed-only, ABI-safe), and opens a linked issue + draft PR. Modelled
on `memory-leak-fixer` and grounded in real perf PRs (#4241 SKMatrix
native->managed, #4345 SKColor alloc-free parse, #4247 SKSurface.Canvas
caching).

The skill's defining discipline is two proofs: every fix must be shown
FASTER (benchmark) and behaviour-IDENTICAL (equivalence test, bit-exact vs
SkiaApi.sk_* for numeric ports) - a faster-but-different answer is a
rendering regression, not a win.

References are split by area (mirroring dotnet/aspnetcore's
optimize-perf skill): hot-paths/ (SkiaSharp code areas) + bcl-patterns/
(.NET techniques), plus decision-framework, measuring, signals, and
repo-helpers. A gh-aw workflow runs the skill on a schedule.

Make the benchmark harness AI-ready so a run never has to repair infra:
- Replace the broken TheBenchmark.cs (its [SimpleJob(Net48/70/80)]
  attributes silently skip off-Windows and its empty bodies taught none of
  the conventions) with a working TemplateBenchmark.cs: a New-vs-Old +
  [MemoryDiagnoser] + [Params] + return-sink skeleton that runs out of the
  box.
- Add benchmarks/README.md: the on-ramp (add/run commands, --list flat and
  --job short for fast iteration, single-TFM warning, New-vs-Old + sink
  conventions, InternalsVisibleTo native-oracle note).

Also register /performance-fixer in AGENTS.md's command tables.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

📦 Try the packages from this PR

Warning

Do not run these scripts without first reviewing the code in this PR.

Step 1 — Download the packages

bash / macOS / Linux:

curl -fsSL https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.sh | bash -s -- 4361

PowerShell / Windows:

iex "& { $(irm https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.ps1) } 4361"

Step 2 — Add the local NuGet source

dotnet nuget add source ~/.skiasharp/hives/pr-4361/packages --name skiasharp-pr-4361
More options
Option Description
--successful-only / -SuccessfulOnly Only use successful builds
--force / -Force Overwrite previously downloaded packages
--list / -List List available artifacts without downloading
--build-id ID / -BuildId ID Download from a specific build

Or download manually from Azure Pipelines — look for the nuget artifact on the build for this PR.

Remove the source when you're done:

dotnet nuget remove source skiasharp-pr-4361

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

📖 Documentation Preview

The documentation for this PR has been deployed and is available at:

🔗 View Staging Site
🔗 View Staging Docs
🔗 View Staging Gallery (Blazor)
🔗 View Staging Gallery (Uno Platform)
🔗 View Staging SkiaFiddle

This preview will be updated automatically when you push new commits to this PR.


This comment is automatically updated by the documentation staging workflow.

…gory

Every issue/PR the workflow emits now gets tenet/performance automatically,
plus exactly one agent-chosen perf/* category label (perf/interop,
perf/allocations, perf/rendering, perf/throughput, perf/startup,
perf/memory-leak, perf/size) decided by the dominant measured driver of the
win. Wired via safe-outputs allowed-labels (both create-issue and
create-pull-request) and a Step 2 guardrail decision table; documented in the
skill's Phase 4. Recompiled the workflow lock.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
mattleibow added a commit that referenced this pull request Jul 7, 2026
Align the memory-leak-fixer's terminology with the performance-fixer
skill (PR #4361): the 11 rotating leak categories are now 'focus areas'
rather than 'families', and the workflow's focus_family input is renamed
to focus_area. Reword the single overarching 'leak family' to 'class of
leaks' to drop the term entirely.

- types-of-leaks.md: intro, table header (Family -> Focus area), and
  cross-references (family N -> area N).
- SKILL.md: catalogue framing, Phase 1.1 round-robin, and all references.
- memory-leak-fixer workflow: focus_family -> focus_area input + body.
- Recompile the .lock.yml.

No behavioural change — terminology only.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
mattleibow added a commit that referenced this pull request Jul 7, 2026
… areas (#4362)

[skills] Add perf/* label taxonomy and rework memory-leak-fixer focus areas (#4362)

Two related improvements to the agentic-workflow skills. Memory-leak
findings previously landed with only the `agentic-workflows` label — a
label read by nothing and present on zero issues/PRs — so leaks and
performance reports had no meaningful classification, and the triage
skill had no rule mapping either to a quality tenet.

~~ perf/* label taxonomy ~~

There is no dedicated "memory-leak" label, and grouping every speed,
memory, or size concern under the single `tenet/performance` umbrella
loses the sub-type. Add a `perf/*` axis beneath that umbrella, grounded
in the repo's real corpus (61 tenet/performance + ~25 memory-leak
issues): perf/memory-leak, perf/allocations, perf/interop,
perf/rendering, perf/throughput, perf/startup, perf/size. Any perf/*
implies tenet/performance.

  * memory-leak-fixer workflow: drop agentic-workflows; label the
    finding issue and fix PR with tenet/performance + perf/memory-leak.
  * issue-triage skill: add the perf axis to labels.md, the schema
    (classifiedPerf enum + validated perf[] field), the cheatsheet,
    SKILL.md, and the report template.
  * auto-triage workflow: apply classification.perf[] labels; raise the
    add-labels cap 10 -> 12 to fit the new axis.

Type classification (type/bug vs type/enhancement) is left to triage on
purpose — a leak is usually a bug, but a "make it faster" report may be
an enhancement.

~~ "family" -> "focus area" rework ~~

Align the memory-leak-fixer's vocabulary with the new performance-fixer
skill (#4361), which routes over "focus areas" and a focus_area knob.
The 11 rotating leak categories are now focus areas rather than
families, and the workflow's focus_family dispatch input is renamed to
focus_area. The single overarching "leak family" becomes "class of
leaks" so the term is dropped entirely. Terminology only — no
behavioural change.

Both .lock.yml files were regenerated with `gh aw compile` (0 errors);
the perf/* labels were created in mono/SkiaSharp.

Co-authored-by: Matthew Leibowitz <mattleibow@live.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
mattleibow and others added 2 commits July 7, 2026 23:30
Merge main and match the perf/* label taxonomy the memory-leak-fixer now
uses: apply tenet/performance automatically and let the agent add the
matching perf/* sub-type from .agents/skills/issue-triage/references/labels.md
(chosen by the win's dominant driver — a removed P/Invoke maps to
perf/interop, whose taxonomy example is literally "remove native interop in
SKMatrix"). Drop the agentic-workflows label to match the sibling workflow,
reference the shared taxonomy instead of duplicating a table, and recompile.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@mattleibow
mattleibow marked this pull request as ready for review July 8, 2026 07:47
@mattleibow
mattleibow merged commit bd14b40 into main Jul 8, 2026
94 of 98 checks passed
@mattleibow
mattleibow deleted the mattleibow-perf-fixer-skill branch July 8, 2026 07:49
@mattleibow mattleibow added this to the 4.151.0-preview.2 milestone Jul 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant