Skip to content

perf(skia): Reuse shared empty clip SKPath#23592

Open
MartinZikmund wants to merge 1 commit into
masterfrom
dev/mazi/render-skia-emptypath
Open

perf(skia): Reuse shared empty clip SKPath#23592
MartinZikmund wants to merge 1 commit into
masterfrom
dev/mazi/render-skia-emptypath

Conversation

@MartinZikmund

Copy link
Copy Markdown
Member

Reuse a single shared empty SKPath for the native-element clip path when no frame has been recorded yet, instead of allocating a new SKPath() on every such Draw call.

The returned clip path is borrowed read-only by hosts (the non-empty branches already return shared frame-owned / cached paths, and consumers such as the Win32 RenderingNegativePathReevaluated handler only read it to compute a clip region — never mutate or dispose it), so a single shared instance is safe.

Validation

  • Compile: Uno.UI.Skia.csproj (net10.0) builds clean — 0 warnings, 0 errors.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KBAigLEAf3csGwnWE22Yr5

Avoid allocating a new SKPath each frame when no frame has been recorded yet; the native clip path is borrowed read-only by hosts, so a single shared instance is safe.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KBAigLEAf3csGwnWE22Yr5
@github-actions github-actions Bot added the area/skia ✏️ Categorizes an issue or PR as relevant to Skia label Jun 29, 2026
@github-actions

github-actions Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Claude finished @MartinZikmund's task in 2m 35s —— View job


Review

  • Gather context
  • Read changed file (CompositionTarget.Rendering.skia.cs)
  • Trace all callers of OnNativePlatformFrameRequested / Draw to verify safety claim
  • Verify ApplyClipPath (Win32 consumer) does not mutate the incoming path

Verdict: LGTM — no issues found.

The optimization is correct on all dimensions I checked:

Mutation safety confirmed. Every consumer treats the returned SKPath as read-only:

  • macOS (MacOSWindowHost.cs:116): nativeElementClipPath.IsEmpty ? null : nativeElementClipPath.ToSvgPathData() — pure read.
  • Win32 (Win32NativeElementHostingExtension.cs:211): path.Op(_tempPath, SKPathOp.Intersect, _tempPath) — reads path, writes the result into _tempPath. No Dispose/Reset/Rewind on path.
  • X11, browser, Linux FB, Android, Apple UIKit renderers: all use the path only for IsEmpty checks or to call ToSvgPathData().

Thread safety confirmed. _emptyPath is static readonly and never written after type-initialization; sharing it across concurrent render threads is safe.

Latent memory-leak also fixed. The previous return new SKPath() (no-frame branch) was never disposed by any caller — an allocation that leaked on every paint tick before the first frame arrived. The shared static eliminates both the per-call allocation and the leak.

Comment is appropriate. Two-line comment explaining the non-obvious thread/mutation invariant is exactly what the code style asks for.

One observation (pre-existing, not introduced here): _lastNativeClipPath and _lastScaledNativeClipPath are also static (line 34/36), meaning the scale cache is shared across all CompositionTarget instances in a multi-window scenario. Not a problem in practice today, but worth noting if multi-window render pipelines diverge in the future.

@unodevops

Copy link
Copy Markdown
Contributor

🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23592/wasm-skia-net9/index.html

@nventive-devops

Copy link
Copy Markdown
Contributor

The build 220109 found UI Test snapshots differences: skia-linux-screenshots: 57, skia-windows-screenshots: 2348, wasm: 11

Details
  • skia-linux-screenshots: 57 changed over 2348

    🚨🚨 Comparison Details (first 20) 🚨🚨
    • Gamepad_CurrentReading.png-dark
    • Gamepad_CurrentReading.png
    • Gamepad_Enumeration.png-dark
    • Gamepad_Enumeration.png
    • ContextRequested.png-dark
    • ContextRequested.png
    • DisplayInformation.png-dark
    • ElementLevelTheme.png-dark
    • ElementLevelTheme.png
    • Focus_FocusVisual_Properties.png-dark
    • ImageIconPage.png-dark
    • Examples.png-dark
    • Examples.png
    • ExpanderColorValidationPage.png-dark
    • ExpanderColorValidationPage.png
    • Buttons.png-dark
    • Buttons.png
    • DropDownButtonPage.png-dark
    • DropDownButtonPage.png
    • ButtonClippingTestsControl.png-dark
  • skia-windows-screenshots: 2348 changed over 2366

    🚨🚨 Comparison Details (first 20) 🚨🚨
    • ApplicationViewMode.png
    • ApplicationViewSizing.png
    • AppWindowPositionAndSize.png
    • AppXamlDefinedResources.png-dark
    • AutoBorderStretchwithleftmargin.png-dark
    • AutoSuggestBoxChosenSuggestion.png
    • BadgeNotificationTests.png
    • Basics.png
    • BindableDrawerLayout_ChangePane.png
    • BitmapIcon_Foreground.png-dark
    • Border_Clipped_Change_Property.png
    • Border_CornerRadius.png-dark
    • Border_LinearGradientBrush.png-dark
    • Border_LinearGradientBrush.png
    • ButtonClippingTestsControl.png-dark
    • ButtonClippingTestsControl.png
    • CalendarView_Theming.png-dark
    • CalendarView_Theming.png
    • Circle.png-dark
    • Circle.png
  • wasm: 11 changed over 1076

    🚨🚨 Comparison Details (first 20) 🚨🚨
    • SamplesApp.Windows_UI_Xaml_Controls.ListView.ListViewSelectedItems
    • UITests.Shared.Windows_UI_Xaml_Controls.MediaPlayerElement.MediaPlayerElement_Minimal
    • UITests.Windows_UI_Xaml_Media_Animation.ColorAnimation_Background
    • SamplesApp.Wasm.Windows_UI_Xaml_Controls.ListView.ListView_IsSelected
    • UITests.Shared.Windows_UI_Xaml_Controls.MediaPlayerElement.MediaPlayerElement_Ogg_Extension
    • UITests.Windows_UI_Xaml_Controls.CalendarView.CalendarView_Theming
    • UITests.Microsoft_UI_Xaml_Controls.WebView2Tests.WebView2_NavigationProperties
    • UITests.Shared.Microsoft_UI_Xaml_Controls.ExpanderTests.WinUIExpanderPage
    • Uno.UI.Samples.Content.UITests.WebView.WebView_AnchorNavigation
    • SamplesApp.Microsoft_UI_Xaml_Controls.WebView2Tests.WebView2_EnableDevTools
    • UITests.Uno_Web.Http.CookieManagerTests

@unodevops

Copy link
Copy Markdown
Contributor

⚠️⚠️ The build 220109 has failed on Uno.UI - CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/skia ✏️ Categorizes an issue or PR as relevant to Skia

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants