Refactor SKManagedStream: replace parent/child chain with SKData snapshot for duplicate/fork#3588
Refactor SKManagedStream: replace parent/child chain with SKData snapshot for duplicate/fork#3588Copilot wants to merge 4 commits into
Conversation
…shot for duplicate/fork Agent-Logs-Url: https://github.com/mono/SkiaSharp/sessions/923fa277-732c-47b8-9b41-f421fb165b9d Co-authored-by: mattleibow <1096616+mattleibow@users.noreply.github.com>
Agent-Logs-Url: https://github.com/mono/SkiaSharp/sessions/923fa277-732c-47b8-9b41-f421fb165b9d Co-authored-by: mattleibow <1096616+mattleibow@users.noreply.github.com>
…onstructor Agent-Logs-Url: https://github.com/mono/SkiaSharp/sessions/6018f013-525a-4a42-bec0-37873a909c77 Co-authored-by: mattleibow <1096616+mattleibow@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refactors SKManagedStream’s duplicate/fork behavior to produce independent streams by snapshotting the underlying seekable .NET stream into a shared SKData, aligning the managed implementation more closely with “independent stream” semantics.
Changes:
- Updated
SKManagedStreamto lazily create and reuse aSKDatasnapshot forDuplicate/Fork, removing the previous parent/child WeakReference chain behavior. - Updated managed-stream duplication/forking tests to validate that original and duplicated/forked streams remain readable and independent.
- Adjusted disposal-related test coverage to reflect the new independent-stream lifecycle behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
binding/SkiaSharp/SKManagedStream.cs |
Implements Duplicate/Fork by creating a shared SKData snapshot and returning native memory streams backed by that snapshot. |
tests/Tests/SkiaSharp/SKManagedStreamTest.cs |
Updates tests to validate the new semantics: duplicates/forks are readable independently and multiple duplicates are supported. |
| private SKData GetOrCreateSnapshot () | ||
| { | ||
| VerifyOriginal (); | ||
| if (snapshotData != null) | ||
| return snapshotData; | ||
|
|
||
| if (!stream.CanSeek) | ||
| return IntPtr.Zero; | ||
|
|
||
| var newStream = new SKManagedStream (stream, disposeStream); | ||
| newStream.parent = new WeakReference (this); | ||
|
|
||
| wasCopied = true; | ||
| disposeStream = false; | ||
| child = new WeakReference (newStream); | ||
|
|
||
| stream.Position = 0; | ||
| return null; | ||
|
|
||
| var pos = stream.Position; | ||
| try { | ||
| stream.Position = 0; | ||
| snapshotData = SKData.Create (stream, stream.Length); | ||
| } finally { | ||
| stream.Position = pos; | ||
| } | ||
|
|
||
| return newStream.Handle; | ||
| return snapshotData; | ||
| } |
There was a problem hiding this comment.
GetOrCreateSnapshot() reads the entire managed stream into an SKData snapshot (native memory) on first Duplicate/Fork and keeps it alive for the lifetime of the SKManagedStream. This changes Duplicate/Fork from O(1) to O(stream.Length) memory/time and can retain very large buffers even if only a single duplicate was needed. Consider limiting snapshot creation (e.g., only when Skia requests duplicate, and optionally releasing the snapshot after the last duplicate/fork is created or providing a size/feature guard), or documenting this clearly as a potentially expensive operation.
| // via SKData (ref-counted), so multiple duplicates share one copy with | ||
| // zero additional managed allocations per duplicate. |
There was a problem hiding this comment.
The field comment says duplicates share one copy with “zero additional managed allocations per duplicate”, but each duplicate still creates a managed SKStream wrapper object. If the intent is to say there are no additional byte-buffer allocations in managed memory after the initial snapshot, consider rewording to avoid overpromising.
| // via SKData (ref-counted), so multiple duplicates share one copy with | |
| // zero additional managed allocations per duplicate. | |
| // via SKData (ref-counted), so multiple duplicates share one native | |
| // byte buffer with no additional managed byte-buffer allocations per duplicate. |
|
Closing in favor of PR #3589, which has the same SKData snapshot implementation but with better test coverage:
|
SKAbstractManagedStream: addprivate protected (bool owns, bool weak)constructorSKManagedStreamduplicate/fork semantics with SKData snapshotSKManagedStreamTest.csfor new independent stream behaviorprivate protected SKAbstractManagedStream(bool owns, bool weak)constructor — dead code sinceOnDuplicate/OnForknow return native handles directlyprivate SKManagedStream(Stream, bool, bool weak)constructor — never calledSKAbstractManagedStream(bool owns)to call base directly⌨️ Start Copilot coding agent tasks without leaving your editor — available in VS Code, Visual Studio, JetBrains IDEs and Eclipse.