Add retained-scene refresh profiling and guides#486
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b813d6e82
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50f4624a4d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82a857839e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8d8a599b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| hash.Add(erodeInputRevision); | ||
| break; | ||
| case ImageImageFilter imageImageFilter: | ||
| hash.Add(imageImageFilter.Image?.Version ?? 0); |
There was a problem hiding this comment.
The ImageImageFilter cache key only hashes image.Version, so in-place edits to SKImage.Data (same array instance) do not change the revision and GetRenderImageFilter(...) can keep returning a stale native filter. This means filter outputs using ImageImageFilter can ignore legitimate runtime image-buffer mutations until callers replace the whole Data array object.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a6a5708eb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| hash.Add(erodeInputRevision); | ||
| break; | ||
| case ImageImageFilter imageImageFilter: | ||
| hash.Add(imageImageFilter.Image?.Version ?? 0); |
There was a problem hiding this comment.
GetRenderImageFilter(...) reuses cached native filters based on TryGetImageFilterRevision(...), but the ImageImageFilter branch only hashes image.Version. SKImage.Version changes on property assignment, not when callers mutate the existing byte[] in place, so filter caches can serve stale pixels after legitimate image.Data buffer edits. This makes filter output incorrect until the image object or data reference is replaced.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 909f953c1e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| hash.Add(erodeInputRevision); | ||
| break; | ||
| case ImageImageFilter imageImageFilter: | ||
| hash.Add(imageImageFilter.Image?.Version ?? 0); |
There was a problem hiding this comment.
TryGetImageFilterRevision hashes ImageImageFilter using only image.Version, but SKImage.Version does not change when callers mutate the existing Data buffer in place. In that case GetRenderImageFilter will reuse a stale native image filter and filtered output will not reflect valid runtime image-byte updates until a new Data array instance is assigned.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce8b3df7ac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| get => _color; | ||
| set | ||
| { | ||
| if (_color.Equals(value)) |
There was a problem hiding this comment.
The Color setter calls _color.Equals(value), but _color is nullable and can legitimately become null after paint.Color = null. Any later assignment (including assigning null again) then throws a NullReferenceException, which turns a normal paint update into a runtime crash path for callers that clear and reapply colors.
Useful? React with 👍 / 👎.
1 participant
Summary
This PR adds retained-scene mutation refresh APIs to
SKSvg, a dedicated SVG pipeline profiler plus BenchmarkDotNet coverage for the hot phases, a larger retained-scene/how-to documentation set, and a follow-up optimization pass that reduces parser, retained-scene, text-layout, native picture, and bitmap-render overhead without dropping SVG feature support.Changes
Runtime and optimization work
TryApplyRetainedSceneMutationAndRender(...)overloads for direct element, address-key, and id-based retained-scene refresh flows inSKSvgPicturedirectly from the retained scene when an incremental mutation succeeds instead of forcing a fullFromSvgDocument(...)rebuildSvgLoadPipelineProfilerand wired it intotests/Svg.Skia.Benchmarksvia--profile-svgtextPathplacementSvgDocumentCompatibilityLoaderin a single pass to cut parser-side allocations and repeated enumerationSvgInlineStyleAttributeParsersoSvgElementFactorystays focused on XML attribute dispatchIndexOf(...)scansSvgElementAddress.Keywithout LINQ so key materialization is not repeated on every accesstextPathplacement traversalSKShader,SKColorFilter,SKPathEffect, andSKImageFilterstate for complex paints while keeping mutable picture-backed paths on the uncached routeCorrectness and regression coverage
textPathcontenttextPathBaseUrihandling in the profiler, inline-style CSS property casing, and unstable temporary<use>addresses during retained-scene key cachingDocumentation
SvgDocument, reload, andVectorDrawableentry pointsSvgDocumenthandling and mutation, retained scene graph usage, and performance/retained-scene refreshHitTestElements(...),HitTestSceneNodes(...), and topmost-target APIsSourceDocument-> retained scene -> shim model -> native picture flowSvg.Skia,Svg.Model, andSvg.Editor.Skiapackage pages to point at the new guides and current terminologyPerformance notes
Profiled with:
dotnet run --project tests/Svg.Skia.Benchmarks/Svg.Skia.Benchmarks.csproj -c Release --no-build -- --profile-svg '/Users/wieslawsoltes/Downloads/solar battery.svg' 30Current
osx-arm64Releaseprofiler values for/Users/wieslawsoltes/Downloads/solar battery.svg:SvgDocumentfrom string1.72 ms4.58 ms0.03 msSKPicture0.60 ms3.69 ms14.86 msSKSvg.FromSvg(...)11.66 ms13.43 msFromSvgDocument(...)rebuild19.54 ms12.41 msAgainst the original five-stage baseline on this asset, the current branch moved from
15.34 msto10.62 msfor parse + retained compile + shim model + native picture + bitmap render, which is about30.8%faster overall.Representative targeted BenchmarkDotNet deltas on the same asset:
CompileNodeTreeOnly:2.061 ms -> 1.782 ms(-13.5%)RegisterDependenciesOnly:369.7 us -> 328.5 us(-11.1%)ResolveRuntimePayloadsOnly:149.1 us -> 137.6 us(-7.7%)DrawNativePicture1x:3.803 ms -> 3.397 ms(-10.7%)RenderTransparentBitmap1x:3.672 ms -> 3.498 ms(-4.7%)RenderOpaqueBitmap1x:3.773 ms -> 3.525 ms(-6.6%)Main takeaway: retained-scene compile remains the largest in-pipeline
SKSvgload cost for this asset, but the branch removes repeated work across parsing, retained-scene indexing/dependency rebuild, text placement, native picture construction, and bitmap rendering while keeping incremental retained-scene refresh below a fullFromSvgDocument(...)rebuild after DOM edits.Validation
dotnet tool run lunet --stacktrace builddotnet build tests/Svg.Skia.Benchmarks/Svg.Skia.Benchmarks.csproj -c Release --no-restoredotnet test tests/Svg.Skia.UnitTests/Svg.Skia.UnitTests.csproj -f net10.0 -c Release --no-restore --filter "FullyQualifiedName~CreateRetainedSceneGraphPicture_MatchesCurrentPicture_ForPrimitiveShapesDocument|FullyQualifiedName~CreateRetainedSceneGraphPicture_MatchesCurrentPicture_ForRootOpacityDocument|FullyQualifiedName~CreateRetainedSceneGraphPicture_MatchesCurrentPicture_ForFilteredGroupDocument|FullyQualifiedName~CreateRetainedSceneGraphPicture_MatchesCurrentPicture_ForLongTextPathDocument|FullyQualifiedName~RetainedSceneGraph_TextPathPercentStartOffset_FollowsArcGeometry|FullyQualifiedName~resvgTests.e_textPath|FullyQualifiedName~resvgTests.a_letter_spacing|FullyQualifiedName~resvgTests.a_word_spacing|FullyQualifiedName~SvgAnimationControllerTests"Notes