Bug 102272 – [Skia] Deferred SkCanvas save()

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Florin Malita]

Certain patterns in the Skia command stream (observable in captured SKPs for instance) contain redundant save/restore pairs.

This bug is for investigating a lazy PlatformContextSkia save() strategy, where the actually canvas operation is deferred until a clip/matrix change is observed (as opposed to the current approach where the save is performed upfront, on GraphicsContext::save()). Experiments show that this would yield a 1-20% save/restore reduction for regular/html pages, with much higher gains possible for SVG content.

[Florin Malita]

Created attachment 174245 [details]
Patch

[Florin Malita]

First pass.

While there are ASSERTs to verify that external canvas manipulation doesn't interfere, the underlying canvas is still accessible. Maybe we should try to push this all the way and see if it's possible to make PlatformContextSkia::canvas() private?

Also, it feels like the PlatformContextSkia encapsulation work should be a separate patch.

[Philip Rogers]

Comment on attachment 174245 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174245&action=review

It feels like this patch works around the real issue: unnecessary saves and restores. Could this patch be modified to assert that we don't unnecessarily save, or are these saves unavoidable?

> Source/WebCore/ChangeLog:14
> +        gains possible for deeply nested SVG content.

I think we need more data before adding a new check in so many hotpaths.

Does this performance win show up on any of our performance tests such as the page cycler?

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:60
> +// DeferredSaveState -------------------------------------------------------------------

This makes comment panda sad :(

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:63
> +struct PlatformContextSkia::DeferredSaveState {

Can this functionality be moved into PlatformContextSkia::State? m_saveStateStack and m_stateStack have the same lifetime.

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:104
> +    void clipPath(const SkPath&, SkRegion::Op = SkRegion::kIntersect_Op, bool doAntiAlias = false);

Suggestion: This could be defined like:
void clipPath(const SkPath&, bool doAntiAlias = false, SkRegion::Op = SkRegion::kIntersect_Op);
This is reversed from the Skia definition but it would make the callsites easier to read because the op as almost always SkRegion::kIntersect_Op.

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:105
> +    void clipRect(const SkRect&, SkRegion::Op = SkRegion::kIntersect_Op, bool doAntiAlias = false);

The doAntiAlias parameter isn't needed for clipRect because it's always false.

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:252
> +    unsigned m_deferredSave;

m_deferredSaveFlags would be a better name for this.

[Florin Malita]

Created attachment 174422 [details]
SVG tiger 30 frames repaint perftest: first 10 runs are ToT, last 10 are with the lazy save patch.

[Florin Malita]

Thanks for looking at this Philip.

(In reply to comment #3)
> It feels like this patch works around the real issue: unnecessary saves and restores. Could this patch be modified to assert that we don't unnecessarily save, or are these saves unavoidable?

Actually, I think this is a good place to address the issue:

* one contained code change instead of more than a hundred potential sites to be considered
* allows platform specific optimizations (separate paint/matrix/clip saves)
* GC save/restore provides a good, natural abstraction; optimizing the call sites effectively breaks this and exposes gory details unnecessarily.

Note that CanvasRenderingContext2D employs the same deferred save strategy (actually takes it even further and applies it to paint attributes also).

The two approaches are not exclusive though, and cleaning up unnecessary GC save/restores at higher levels remains an option (with the caveat mentioned above). You can think of this as a low level Skia command stream optimization.


> > Source/WebCore/ChangeLog:14
> > +        gains possible for deeply nested SVG content.
> 
> I think we need more data before adding a new check in so many hotpaths.

I would be really surprised if the bitmask check impacted performance, even on hotpaths. Also keep in mind that as Chromium switches to a record once/replay multiple times approach, having a streamlined picture becomes more important and recording costs are possibly amortized.


> Does this performance win show up on any of our performance tests such as the page cycler?

Gains are highly dependent on document structure - whether or not it triggers lots of redundant context saves. I've put together an SVG Tiger 30-frame repaint perftest, attached screenshot with 10 regular vs. 10 lazy save runs: in this (admittedly extreme) case, the time went down from ~200ms to ~190ms - about 5% improvement.

The Acid3 page cycler numbers are too close to make a call on my workstation - which is actually good news: no penalty for content that doesn't benefit from deferred saves.

Some page cycler numbers (lazy/regular), but keep in mind they're not stable over multiple runs:

milliseconds	12142/12269
mean per set	1214.20/1226.90
mean per page	1214.20/1226.90
timer lag	304.00/295.00
timer lag per page	30.40/29.50

Site	Min		Max		Mean		Std.d		Err %
acid3	1179.00/1181.00	1361.00/1412.00	1197.89/1206.33	9.87/13.11	0.39/0.51


> > Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:60
> > +// DeferredSaveState -------------------------------------------------------------------
> 
> This makes comment panda sad :(

When in Rome... do as the comment panda says?

OK :)


> > Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:63
> > +struct PlatformContextSkia::DeferredSaveState {
> 
> Can this functionality be moved into PlatformContextSkia::State? m_saveStateStack and m_stateStack have the same lifetime.

Yes, at the moment. I'm intending to look into tagged/granular saves (matrix vs. clip vs. paint) though, which would require separate stacks.


> > Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:104
> > +    void clipPath(const SkPath&, SkRegion::Op = SkRegion::kIntersect_Op, bool doAntiAlias = false);
> 
> Suggestion: This could be defined like:
> void clipPath(const SkPath&, bool doAntiAlias = false, SkRegion::Op = SkRegion::kIntersect_Op);
> This is reversed from the Skia definition but it would make the callsites easier to read because the op as almost always SkRegion::kIntersect_Op.
> 
> > Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:105
> > +    void clipRect(const SkRect&, SkRegion::Op = SkRegion::kIntersect_Op, bool doAntiAlias = false);
> 
> The doAntiAlias parameter isn't needed for clipRect because it's always false.

Agreed on the above, but since these are just Skia wrappers I would prefer to keep the interface consistent (POLS and all) unless there's a serious downside.


> > Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:252
> > +    unsigned m_deferredSave;
> 
> m_deferredSaveFlags would be a better name for this.

OK.

[Philip Rogers]

(In reply to comment #5)
> Yes, at the moment. I'm intending to look into tagged/granular saves (matrix vs. clip vs. paint) though, which would require separate stacks.
> 

I was worried that we were sweeping glaring save/restore mistakes under the rug for Skia platforms only but this makes me feel better, as it will force us to look at the hot callsites and fix any glaring issues. The combination of lazy saves and granular saves seems like a clean solution to me.

[Stephen White]

Comment on attachment 174245 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174245&action=review

>> Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:104
>> +    void clipPath(const SkPath&, SkRegion::Op = SkRegion::kIntersect_Op, bool doAntiAlias = false);
> 
> Suggestion: This could be defined like:
> void clipPath(const SkPath&, bool doAntiAlias = false, SkRegion::Op = SkRegion::kIntersect_Op);
> This is reversed from the Skia definition but it would make the callsites easier to read because the op as almost always SkRegion::kIntersect_Op.

If you're going to change the signature, I suggest adding an enum to PlatformContextSkia for the antialias flag, so call sites are more self-documenting.

[Mike Reed]

+1 to that.

either keep the same signature as SkCanvas, or make it *really* clear w/ an enum.

[Florin Malita]

After chatting with Mike, it sounds like the best approach would be to take the canvas encapsulation one step further:

1) add more SkCanvas wrapper methods - to alleviate the need for most PlatformContextSkia::canvas() calls.
2) flush pending saves on canvas() access - for the remaining direct canvas users that cannot be easily refactored.

This should eliminate the risk of SkCanvas changes bypassing the deferred save logic.

Since the PlatformContextSkia refactoring is going to be significant and orthogonal to the deferred save change, it makes sense to split it into a separate patch.

[Florin Malita]

Created attachment 176002 [details]
Patch

Updated per comments, rebased after http://trac.webkit.org/changeset/135390.

[Philip Rogers]

Comment on attachment 176002 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176002&action=review

Nice work Florin; I like how this patch looks now.

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:61
> +// Holds deferred save info.

Some of these comments (e.g., "Flush pending saves." below) don't add value. I don't think they hurt but the comment pandas may ask for these to be removed.

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:152
> +    SkCanvas* canvas();

Can you add a comment that using canvas() directly should be discouraged?

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:283
> +    //        come in handy if/when adding granular save() support (clip vs. matrix vs. paint).

if/when -> when

[Florin Malita]

Created attachment 176577 [details]
Patch

Thanks Philip, updated.

[Florin Malita]

Bump.

[Stephen Chenney]

Comment on attachment 176577 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176577&action=review

I was worried about how much effect it would really have, but as it stands the overhead is not too large so even small performance effect is OK.

Stephen White should review, I think.

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:62
> +    DeferredSaveState(unsigned mask, int count) : m_flags(mask), m_restoreCount(count) { }

Why is this not added into State? You always push one when you push a State object (except the constructor), and always pop one when you pop a State object. At least, it seems that way to me and you could avoid the overhead of managing a second Vector.

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:280
> +    Vector<DeferredSaveState> m_saveStateStack;

Why not WTF::Vector? What namespaces are we using?

[Florin Malita]

(In reply to comment #14)
> I was worried about how much effect it would really have, but as it stands the overhead is not too large so even small performance effect is OK.

The extra inline bitmask test is basically not measurable in any of the perftests I looked at.


> > Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:62
> > +    DeferredSaveState(unsigned mask, int count) : m_flags(mask), m_restoreCount(count) { }
> 
> Why is this not added into State? You always push one when you push a State object (except the constructor), and always pop one when you pop a State object. At least, it seems that way to me and you could avoid the overhead of managing a second Vector.

Right, at this point the two states are managed in sync. But the plan is to follow up with another patch which implements targeted save operations (matrix vs. clip vs. paint) and in that scenario the two stacks need to be distinct. We could defer having separate stacks until that point though, if you think it's needed.


> > Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:280
> > +    Vector<DeferredSaveState> m_saveStateStack;
> 
> Why not WTF::Vector? What namespaces are we using?

wtf/Vector.h already includes "using WTF::Vector", so most WebCore code just uses the unprefixed form.

[Stephen Chenney]

Comment on attachment 176577 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176577&action=review

>> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:62
>> +    DeferredSaveState(unsigned mask, int count) : m_flags(mask), m_restoreCount(count) { }
> 
> Why is this not added into State? You always push one when you push a State object (except the constructor), and always pop one when you pop a State object. At least, it seems that way to me and you could avoid the overhead of managing a second Vector.

If there's a follow on patch I don't see any reason to merge.

>> Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:280
>> +    Vector<DeferredSaveState> m_saveStateStack;
> 
> Why not WTF::Vector? What namespaces are we using?

I would add the WTF to increase clarity. This is Skia maintained code, essentially, and not everyone is familiar with the source of these containers.

[Florin Malita]

Comment on attachment 176577 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176577&action=review

> I would add the WTF to increase clarity. This is Skia maintained code, essentially, and not everyone is familiar with the source of these containers.

Makes sense, will do.

[Stephen White]

Comment on attachment 176577 [details]
Patch

OK.  r=me

[Florin Malita]

Created attachment 177302 [details]
Patch for landing

[Florin Malita]

Thanks Stephen & Stephen :)

[WebKit Review Bot]

Comment on attachment 177302 [details]
Patch for landing

Rejecting attachment 177302 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ripts/update-webkit line 152.

Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2
Updating OpenSource
From http://git.chromium.org/external/Webkit
   6e7bd80..609a784  HEAD       -> origin/HEAD
error: Ref refs/remotes/origin/master is at 609a784ca2e853e52c3cddbbebe1adac105b3a77 but expected 6e7bd80c78700a6a0ae7563e3412a21f935603e8
 ! 6e7bd80..609a784  master     -> origin/master  (unable to update local ref)
Died at Tools/Scripts/update-webkit line 152.

Full output: http://queues.webkit.org/results/15105624

[WebKit Review Bot]

Comment on attachment 177302 [details]
Patch for landing

Rejecting attachment 177302 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ripts/update-webkit line 152.

Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2
Updating OpenSource
From http://git.chromium.org/external/Webkit
   4d1f4e9..6d1c7b0  HEAD       -> origin/HEAD
error: Ref refs/remotes/origin/master is at 6d1c7b03d56865028e46f67fa7f11f1f52a33c79 but expected 4d1f4e901fee3b3faed178cb2e7b3d1184197f51
 ! 4d1f4e9..6d1c7b0  master     -> origin/master  (unable to update local ref)
Died at Tools/Scripts/update-webkit line 152.

Full output: http://queues.webkit.org/results/15132061

[WebKit Review Bot]

Comment on attachment 177302 [details]
Patch for landing

Clearing flags on attachment: 177302

Committed r136435: <http://trac.webkit.org/changeset/136435>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.