Conversation
LoadFontBytesAsync copied each font into a growing MemoryStream and then ToArray()'d it, allocating about 3.4x the file size per face. When the stream is seekable, read it straight into an array of its length. Fixes unoplatform#24861
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The localized optimization preserves stream-position handling and fallback behavior, with no identified regressions.
Review effort: Balanced
Findings: None
What changed in this PR
Reduces Skia font-loading allocations to address #24861 without changing the font cache’s behavior.
Changes:
- Reads seekable streams directly into a buffer sized to the remaining bytes.
- Preserves the existing fallback for non-seekable streams.
| File | Description |
|---|---|
| src/Uno.UI/UI/Xaml/Documents/TextFormatting/FontDetailsCache.cs | Avoids buffer growth and an extra copy when loading seekable font streams. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
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.
GitHub Issue: closes #24861
PR Type:
🐞 Bugfix
What changed? 🚀
FontDetailsCache.LoadFontBytesAsynccopied each font into a growingMemoryStreamand then calledToArray(), so every face paid for the buffer doubling plus a second full copy. DuringPreloadFontsthis is the ~14 MB of transientbyte[]reported in the issue.When the stream is seekable, the font is now read with
ReadExactlyAsyncstraight into an array of the stream length. That is the case for every app-package font:ms-appxresolves to a localFileStreamon Skia desktop, WebAssembly (after the asset download) and Android, and the HTTP path returns a buffered stream. Non-seekable streams keep the previous behaviour.I first tried the existing
StreamExtensions.ReadBytesAsync()helper, which also pre-sizes the buffer. It awaits withoutConfigureAwait(false)though, so each read hops back to the UI thread, and it made the font preload about 240 ms slower in the measurements below.ReadExactlyAsyncresumes on the UI thread only once per face.Measured with SamplesApp, which preloads the same
Uno.Fonts.OpenSansmanifest (37 faces, 5.35 MB): Skia desktop, Win32, Release,net10.0-desktop, 9 interleaved launches each, medians, using the marks from the issue:OnLaunchedstartPreloadFontscompletesThe instrumentation was local only and is not part of this PR.
Tests run locally on Skia desktop (Win32):
Given_TextBlock*,Given_GlyphRunRenderer,Given_PersonPicture,Given_SkiaTextFormatter,Given_BlockLayoutEngineandGiven_TextBox. 390 passed, 10 skipped, 1 failed:Given_TextBlock.When_Inlines_Transitively_Change, which fails the same way on master on my machine (the screenshot comes back empty).PR Checklist ✅
No new test: the change is limited to how the bytes are read, and the existing font loading tests cover that path. An allocation assertion in a runtime test would be too noisy to be reliable.
Not needed: internal change, no public API or behaviour change.
Screenshots Compare Test Runresults.