Initialize SKObject ownedObjects/keepAliveObjects ConcurrentDictionary with concurrency level and capacity#4182
Initialize SKObject ownedObjects/keepAliveObjects ConcurrentDictionary with concurrency level and capacity#4182nietras wants to merge 5 commits into
Conversation
Just a suggested change based on mono#4181 Must be evaluated by whether normal usage does not match this
📦 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 -- 4182PowerShell / Windows: iex "& { $(irm https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.ps1) } 4182"Step 2 — Add the local NuGet source dotnet nuget add source ~/.skiasharp/hives/pr-4182/packages --name skiasharp-pr-4182More options
Or download manually from Azure Pipelines — look for the Remove the source when you're done: dotnet nuget remove source skiasharp-pr-4182 |
📊 Benchmark: allocations for
|
There was a problem hiding this comment.
Pull request overview
This PR tweaks SKObject’s lazy-initialized OwnedObjects and KeepAliveObjects dictionaries to use explicit ConcurrentDictionary constructor parameters, aiming to reduce the allocations caused by the default capacity/concurrency settings (related to #4181’s allocation observations).
Changes:
- Initialize
OwnedObjectswithConcurrentDictionary(concurrencyLevel: 1, capacity: 1)instead of the default constructor. - Initialize
KeepAliveObjectswithConcurrentDictionary(concurrencyLevel: 1, capacity: 1)instead of the default constructor.
| lock (locker) { | ||
| ownedObjects ??= new ConcurrentDictionary<IntPtr, SKObject> (); | ||
| ownedObjects ??= new ConcurrentDictionary<IntPtr, SKObject> ( | ||
| concurrencyLevel: 1, capacity: 1); |
| lock (locker) { | ||
| keepAliveObjects ??= new ConcurrentDictionary<IntPtr, SKObject> (); | ||
| keepAliveObjects ??= new ConcurrentDictionary<IntPtr, SKObject> ( | ||
| concurrencyLevel: 1, capacity: 1); |
There was a problem hiding this comment.
@mattleibow thanks for looking at this, and I have no problem as such committing changes suggested by copilot, but would it perhaps be better to have these parameters forwarded from different children of SKObject so parameters match usage? SKCanvas keeps capacity 1, others can have more?
|
Have you run this in a real app and have data that shows improvement? Also, SkiaSharp can run in a web server in multiple threads. There typically should not be cross-thread usages of these fields. However, do you have a scenario where these changes help besides the allocations? What are the downsides of this PR and what scenarios will it impact? |
|
I've tested before and after this PR with an Uno Platform sample app, once just measuring the startup allocations and once measuring a steady state animation playing on repeat. The difference is very minor and is indistinguishable from noise, so I'm not sure if it's worth it unless we have a concrete scenario where we're seeing these allocations causing significant GC time or something similar. @nietras did you encounter a real scenario where this PR would decently affect the allocations and/or gc time? |
📉 Perf impact note:
|



Just a suggestion on what a change might be for #4181
Must be evaluated by whether normal usage matches this etc.
I am assuming normal/default use case is 1 UI thread, like in Avalonia. This does not prevent usage with more threads. But is having more buckets/lock objects really necessary for that anyway, as simple "pointer" storage.