[performance] Read SKShaper glyph results via zero-copy spans (remove per-shape array copies)#4388
Merged
Merged
Conversation
SKShaper.Shape read the HarfBuzz results through buffer.GlyphInfos / buffer.GlyphPositions. Each getter allocates a full GlyphInfo[] / GlyphPosition[] copy of the native glyph buffer (via GetGlyphInfoSpan().ToArray()) but the extraction loop reads each exactly once, so both copies are pure per-shape waste on the SKCanvas.DrawShapedText hot path. Switch to the existing zero-copy buffer.GetGlyphInfoSpan() / GetGlyphPositionSpan(); the loop body is unchanged (indexing is identical on ReadOnlySpan<T>) and the buffer outlives the loop, so the spans over native memory stay valid. Result is bit-identical to the array path. Measured (BenchmarkDotNet New-vs-Old, Linux x64, net10.0, InProcessEmitToolchain): 3/24/96 glyphs -> 0.65x/0.56x/0.56x time and 0.42x/0.31x/0.29x allocations (304->128 B, 1872->576 B, 7248->2112 B); no allocation regression. Adds SKShaperGlyphExtractionBenchmark (proof of the win) and SKShaperGlyphExtractionTests (oracle-vs-subject bit-exact parity across Arabic + Latin, span/array parity, empty boundary; verified to fail on a deliberately perturbed Shape). Private method body only; public ABI unchanged. Produced by the performance-fixer agentic workflow + performance-fixer skill. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
perf/allocations
Contributor
There was a problem hiding this comment.
Pull request overview
This PR optimizes the managed HarfBuzz shaping hot path in SKShaper.Shape(Buffer, ...) by avoiding two per-shape managed array allocations (GlyphInfo[] / GlyphPosition[]) and reading shaping results through the existing zero-copy span accessors instead.
Changes:
- Update
SKShaper.Shape(Buffer, float, float, SKFont)to usebuffer.GetGlyphInfoSpan()/GetGlyphPositionSpan()rather than the allocatingGlyphInfos/GlyphPositionsproperties. - Add regression tests that assert span-vs-array parity and bit-identical
SKShaper.Resultoutput against an array-getter oracle. - Add a BenchmarkDotNet benchmark and embed a real font resource for stable benchmark execution regardless of working directory.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| source/SkiaSharp.HarfBuzz/SkiaSharp.HarfBuzz/SKShaper.cs | Switch glyph extraction to zero-copy spans to remove per-shape array copies. |
| tests/Tests/SkiaSharp/SKShaperGlyphExtractionTests.cs | Add tests that lock in span-vs-array equivalence and empty-input behavior. |
| benchmarks/SkiaSharp.Benchmarks/SkiaSharp.Benchmarks.csproj | Embed content-font.ttf for the new benchmark to load reliably. |
| benchmarks/SkiaSharp.Benchmarks/Benchmarks/SKShaperGlyphExtractionBenchmark.cs | Add benchmark comparing old (array getters) vs new (span getters) extraction. |
Comment on lines
+166
to
+175
| var bytes = new byte[stream.Length]; | ||
| var read = 0; | ||
| while (read < bytes.Length) | ||
| { | ||
| var n = stream.Read(bytes, read, bytes.Length - read); | ||
| if (n == 0) | ||
| break; | ||
| read += n; | ||
| } | ||
| return bytes; |
Comment on lines
+1
to
+3
| using System; | ||
| using System.Reflection; | ||
| using System.Text; |
mattleibow
marked this pull request as ready for review
July 11, 2026 07:28
Contributor
Author
📦 Try the packages from this PRWarning 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 -- 4388PowerShell / Windows: iex "& { $(irm https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.ps1) } 4388"Step 2 — Add the local NuGet source dotnet nuget add source ~/.skiasharp/hives/pr-4388/packages --name skiasharp-pr-4388More options
Or download manually from Azure Pipelines — look for the Remove the source when you're done: dotnet nuget remove source skiasharp-pr-4388 |
Contributor
Author
|
📖 Documentation Preview The documentation for this PR has been deployed and is available at: 🔗 View Staging Site This preview will be updated automatically when you push new commits to this PR. This comment is automatically updated by the documentation staging workflow. |
mattleibow
added
the
partner/agentic-workflows
mattleibow
added a commit
that referenced
this pull request
Jul 13, 2026
The Polish agent decides whether a page needs new prose. When a page's data.json changed, it read the existing prose.json, judged it 'compatible', and skipped it — silently dropping brand-new product PRs. Concretely, a scheduled run (PR #4413) added three performance PRs (#4408, #4388, #4396) to 4.151.0-unreleased.data.json but left the page's prose untouched, so the rendered page never mentioned them and the PR shipped data.json with a stale .md. (An earlier run on the same facts did update the prose — the behaviour is non-deterministic.) Remove the soft 'is the old prose still matching?' judgment entirely: - release-notes-data.py: when a page's data.json genuinely changes, DELETE its _sources/<v>.prose.json so the agent must re-author the page from scratch against the fresh facts. Scoped to a real change (not a bare --force re-render). The human-owned <v>.notes.md sidecar is preserved; the .md is not deleted because render overwrites it wholesale from the new prose. - release-notes-render.py: a data.json with no matching prose.json is now a HARD ERROR in --all (was: warn and keep the stale committed page). After Prepare, unchanged pages keep their prose and changed pages have theirs deleted, so a missing prose.json can only mean the agent failed to author a changed/new page — the run fails instead of shipping stale. Verified: all 71 committed pages have prose (hard-fail is safe); render --all passes with prose present and fails (exit 1, names the page) when a page's prose is missing. SKILL.md and the spec (§4.6) updated to match. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Collaborator
Local benchmark — macOS (real hardware)Ran the committed Environment: Apple M3 Pro · macOS 26.5 · .NET 10.0.9 (Arm64 RyuJIT) · BenchmarkDotNet 0.13.5 ·
≈ 1.7–1.9× faster and ~58–71% fewer bytes allocated per shape (e.g. 96 glyphs: 7,248 B → 2,112 B). The allocation reduction holds on modern .NET (these arrays genuinely escape), not just legacy TFMs. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
SKShaper.Shape(Buffer, float, float, SKFont)read the HarfBuzz shaping results throughbuffer.GlyphInfosandbuffer.GlyphPositions. Each getter allocates a fullGlyphInfo[]/GlyphPosition[]copy of the native glyph buffer (Buffer.GlyphInfosisGetGlyphInfoSpan().ToArray();Buffer.GlyphPositionsisGetGlyphPositionSpan().ToArray()), yetthe extraction loop reads each exactly once. Those two copies are pure per-shape waste on the
SKCanvas.DrawShapedText→SKShaper.Shapehot path.This PR reads the results through the existing zero-copy
buffer.GetGlyphInfoSpan()/GetGlyphPositionSpan()instead:The loop body is unchanged — indexing is identical on
ReadOnlySpan<T>. Thebufferoutlives theloop and both accessors call
GC.KeepAlive(this), so the spans over native memory stay valid. Thethree real
Resultarrays (points / clusters / codepoints) are still built.GlyphInfo/GlyphPositionare blittable, so the result is bit-identical to the array path.Proof 1 — benchmark (New vs Old)
benchmarks/SkiaSharp.Benchmarks/Benchmarks/SKShaperGlyphExtractionBenchmark.cs. BenchmarkDotNet[MemoryDiagnoser], Linux x64, .NET 10 (net10.0),InProcessEmitToolchain, 2 independent runs.Run 1 agreed (time ratios 0.54–0.67; identical allocations 304→128 / 1872→576 / 7248→2112 B).
~33–44% faster, 58–71% fewer bytes allocated per shape, no allocation regression. Absolute ns are
hardware/TFM-specific; the ratios are the portable result.
Proof 2 — equivalence test
tests/Tests/SkiaSharp/SKShaperGlyphExtractionTests.cs(8 cases): span-vs-array accessor parity;ShapeResultMatchesArrayGetterOracle[Theory](Arabiccontent-font.ttf+ Latinsegoeui.ttf,various offsets) asserting the shipped
ShapeResultis bit-identical to an oracle recomputed fromthe array getters; and an empty-input boundary. 15/15 SKShaper tests pass before and after. A
deliberately perturbed
Shape(Codepoint + 1) turns the 6 oracle cases red while the empty +raw-parity cases stay green — the test is non-vacuous.
Risk / ABI
Private method body only; public
SKShaperAPI unchanged (ABI-safe). Managed C# only — no native /externals/skiachanges. Rendering output is identical before and after.Testing
dotnet build tests/SkiaSharp.Tests.Console/SkiaSharp.Tests.Console.csproj -c Release -f net10.0--filter-class "*SKShaper*"→ 15 passed / 0 failed (pre- and post-fix).dotnet run -c Release --project benchmarks/SkiaSharp.Benchmarks -- --filter "*SKShaperGlyphExtraction*".Produced by the performance-fixer agentic workflow using the
performance-fixerskill. Numbersabove were empirically measured on the stated hardware/TFM (Linux x64, net10.0,
InProcessEmitToolchain); ratios are the portable takeaway.
Fixes #4387