[performance] Remove per-call char[4] allocation in SKFourByteTag.Parse#4396
Conversation
Rewrite SKFourByteTag.Parse to read the tag characters directly with space padding instead of allocating a char[4] scratch buffer on every call, and add an ABI-safe Parse(ReadOnlySpan<char>) overload that the string overload delegates to. The result is bit-for-bit identical to the original algorithm (proven by a new equivalence test suite) and ~30% faster, with the char[4] allocation removed entirely (32 B/call on TFMs without escape analysis). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
📖 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. |
…ourByteTag tests
The equivalence theory previously computed expected values at runtime via a
copy of the original char[4] algorithm (ReferenceParse), and one input used the
range operator ("..."[..4]). On the .NET Framework test target the range
operator pulls in System.Index/System.Range, which are declared in multiple
referenced assemblies (HarfBuzzSharp + mscorlib), producing CS8356 and failing
compilation of the whole Tests (.NET Framework) job.
Bake the expected packed-uint values in as [InlineData] constants (input +
expected) so the oracle cannot drift at runtime, drop ReferenceParse and the
MemberData source, and remove the range operator. The string and span overloads
are both asserted against the constant, which is strictly stronger than the old
span-vs-string comparison.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
📦 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 -- 4396PowerShell / Windows: iex "& { $(irm https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.ps1) } 4396"Step 2 — Add the local NuGet source dotnet nuget add source ~/.skiasharp/hives/pr-4396/packages --name skiasharp-pr-4396More options
Or download manually from Azure Pipelines — look for the Remove the source when you're done: dotnet nuget remove source skiasharp-pr-4396 |
There was a problem hiding this comment.
Pull request overview
This PR optimizes SKFourByteTag.Parse by removing the per-call char[4] scratch allocation and introducing an ABI-safe Parse(ReadOnlySpan<char>) overload that the string? overload delegates to, keeping the packed-tag semantics unchanged.
Changes:
- Rewrote
SKFourByteTag.Parse(string?)to delegate to a newParse(ReadOnlySpan<char>)overload, avoiding scratch-buffer allocation. - Added a comprehensive test suite covering null/empty, padding/truncation, low-byte truncation, and surrogate code-unit behavior.
- Added a BenchmarkDotNet New-vs-Old benchmark for
SKFourByteTag.Parse.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| binding/SkiaSharp/SKFourByteTag.cs | Adds Parse(ReadOnlySpan<char>) and rewrites Parse(string?) to delegate, eliminating scratch allocation. |
| tests/Tests/SkiaSharp/SKFourByteTagTest.cs | Adds theory/fact coverage to validate exact packed results and span/string overload parity. |
| benchmarks/SkiaSharp.Benchmarks/Benchmarks/SKFourByteTagParseBenchmark.cs | Adds a baseline-vs-new benchmark comparing the original algorithm to the updated implementation. |
After the empty-span guard the length is always >= 1, so the 'tag.Length > 0' check on the first character was dead. Read tag[0] directly to drop the redundant branch; behaviour is unchanged (all parity tests still pass). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
….Parse (#4408) [performance] Remove per-call char[4] allocation in HarfBuzzSharp.Tag.Parse (#4408) Fixes: #4407 HarfBuzzSharp.Tag.Parse(string) packed a tag by copying up to four chars into a freshly allocated char[4] scratch buffer, space-padding the tail, then folding the low byte of each slot into the uint. That scratch array is allocated on every call, and tag parsing is hot during OpenType hydration — feature tags ("liga", "kern"), variation-axis tags ("wght", "wdth", "slnt"), and table tags are often parsed in batches when configuring fonts and shaping features. Rewrite Parse to read the first four chars directly and pad missing trailing slots with spaces inline, with no scratch buffer. Packing semantics are unchanged: null/empty -> Tag.None, short input space-padded, over-long input truncated to four, and only the low byte of each UTF-16 code unit is kept. Add an ABI-safe Parse(ReadOnlySpan<char>) overload and have the string overload delegate to it via tag.AsSpan(), so callers can parse a slice of a larger string without a substring allocation. The existing string signature is untouched. This mirrors the already-merged SkiaSharp.SKFourByteTag.Parse optimization (#4396); Tag is the HarfBuzzSharp sibling that still carried the old char[4] path. Change is body + additive overload only, so it is ABI-safe. Measured (BenchmarkDotNet, MemoryDiagnoser, net10.0) on a 22-tag batch: faster and allocation-free, repeatable across runs. On .NET 10 both rows report zero managed allocation because the JIT stack-allocates the non-escaping char[4], so the win there is codegen (dropping the loops, bounds checks, and array indexing); on net462 / netstandard2.0, which lack object stack allocation, the old path additionally heap-allocated the char[4] per call, so those TFMs also lose that allocation. No allocation regression on any TFM. Correctness is pinned by precomputed [InlineData] constants (input + expected packed uint) asserted against both the string and span overloads, plus a verbatim copy of the original char[4] algorithm as a cross-check oracle, a sliced-span case, null/empty -> None, a ToString round trip, and a teeth guard proving the equivalence assertions discriminate a wrong value. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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>
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 ·
≈ 6.1× faster (206.8 ns → 34.1 ns) for the 22-tag batch. Note on allocation: on .NET 10 the JIT stack-allocates the non-escaping |
What this does
SKFourByteTag.Parse(string?)allocated achar[4]scratch buffer on every call to pad/truncate an OpenType tag before packing it into auint. This PR rewrites the body to read the (up to) four characters directly with space padding — no scratch array — and adds an ABI-safe additiveParse(ReadOnlySpan<char>)overload that thestringoverload delegates to.Invariant that keeps it correct: the original packed
(byte)cof each of the first four chars, padding missing trailing slots with' 'and truncating extra chars;IsNullOrEmptyreturned tag0. The new code preserves all of that exactly —null/empty → empty span →0, the same byte-truncation happens in the existingSKFourByteTag(char,char,char,char)constructor, and space padding is identical.No public signature changed — one method body changed plus one additive overload. ABI-safe.
Proof it is faster
Standalone New-vs-Old harness (the two exact algorithm bodies;
Parseis pure managed, no native call). net10.0, AMD EPYC 7763, .NET 10.0.9 X64 RyuJIT AVX2, 110M calls over a representative tag batch, 3 runs:~30% faster, repeatable. *On modern .NET the JIT escape-analyses the old
char[4]and stack-allocates it in the tight steady-state loop (0 B in runs 2-3), but the allocation is real on cold/tier-0 code (run 1 = 32 B/call) and on TFMs without escape analysis (.NET Framework, netstandard2.0 consumers), where the old path allocates 32 B on every call. The new path never allocates on any TFM.The committed benchmark is
benchmarks/SkiaSharp.Benchmarks/Benchmarks/SKFourByteTagParseBenchmark.cs(standard[MemoryDiagnoser]New-vs-Old form). BenchmarkDotNet's default child-process toolchain cannot build the multi-TFM binding in this CI-less environment (same limitation noted in #4369), so the numbers above were taken with an equivalent in-process harness; run it in CI with:Proof it is identical
tests/Tests/SkiaSharp/SKFourByteTagTest.csgains a bit-exact equivalence suite: a[Theory]comparing the newParseagainst a verbatim copy of the original char[4] algorithm acrossnull/ empty / 1–6 chars / all-spaces / NUL / chars above0xFF(low-byte truncation) / surrogate code units, plus tests that the newReadOnlySpan<char>overload agrees with thestringoverload (including a span sliced out of a larger string) and that empty/defaultspans return0.SKFourByteTagTesttests pass (net10.0).'_'turns the suite red (2 failures), confirming it is a real guardrail.Label
perf/allocations— the dominant, measured driver is eliminating the per-callchar[4]scratch allocation (there is no P/Invoke on this path).Fixes #4395
Produced automatically by the SkiaSharp agentic performance-fixer workflow (via the
performance-fixerskill). Scope: managed C# only. Numbers are empirically measured on the named hardware/TFM (not statically reasoned); absolute ns are environment-dependent. ABI impact: none.