Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Skia SKPaint pooling #2376

Closed
wants to merge 3 commits into from

Conversation

@MarchingCube
Copy link
Member

MarchingCube commented Mar 17, 2019

What does the pull request do?

Leverages my recent changes to SkiaSharp (mono/SkiaSharp#807) to speed up rendering by avoiding constant allocation of SKPaint objects. I benchmarked this by rendering a Rectangle 1000 times using immediate renderer.

SkiaSharp did not push any new package yet, so this will have to wait until they release a new version on nuget.

What is the current behavior?

No pooling

Method Mean Error StdDev Gen 0/1k Op Gen 1/1k Op Gen 2/1k Op Allocated Memory/Op
RenderRectangle 11.34 ms 0.0726 ms 0.0606 ms 2718.7500 - - 2.05 MB

Additionally ThreadSafeObjectPool has a locking issue - Get and Return obtain 2 different locks which can lead to some strange race conditions.

What is the updated/expected behavior with this PR?

Pooling

Method Mean Error StdDev Gen 0/1k Op Gen 1/1k Op Gen 2/1k Op Allocated Memory/Op
RenderRectangle 10.08 ms 0.0190 ms 0.0168 ms 2437.5000 - - 1.84 MB

How was the solution implemented (if it's not obvious)?

DrawingContextImpl.CreatePaint will utilize one of three available SKPaint pools. If we are dealing with most common case (solid color brushes) it will use either a pool for normal brushes or use a pen pool (that enables stroke). In case of other brush type the General pool will be used that supports any kind of paint. Pools for brushes and pens do not reset, so it is important to ensure all paint properties will be assigned during construction (Color for brushes, all stroke properties for pens).

Checklist

Breaking changes

Fixed issues

cc: @Gillibald @kekekeks

MarchingCube and others added 3 commits Mar 17, 2019
@grokys

This comment has been minimized.

Copy link
Member

grokys commented May 16, 2019

@Gillibald @kekekeks any thoughts on this? LGTM from a quick review but I'm not so familiar with this code. Any idea why it's still WIP?

@Gillibald

This comment has been minimized.

Copy link
Contributor

Gillibald commented May 25, 2019

I think this is currently blocked by the SkiaSharp dependency. It uses a not yet released API. I like these changes a lot.

@Gillibald

This comment has been minimized.

Copy link
Contributor

Gillibald commented Jun 7, 2019

I thought about this a bit more. Maybe we should cache shaders instead of paint and mutate a single instance of paint instead. We could cache resources like shaders per DrawingContext. There are some situations where we need a new instance of paint but the common scenario just needs one per DrawingContext.

This approach would make it easier to control aliasing and interpolation. A draw call would not need an extra parameter for filter, interpretation, aliasing and instead we configure this on the DrawingContext's paint.

@MarchingCube

This comment has been minimized.

Copy link
Member Author

MarchingCube commented Dec 2, 2019

I think this need a bit more thinking about. I think we still have cases where we multiple SKPaint instances. Newer Skia also made creating new objects a bit cheaper, so new profiling run is needed. I will close this for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.