[SVG] Shared <use> path geometry

[SVG] Shared <use> path geometry

Based on Fredrik Söderquist <fs@opera.com>'s idea.

For <use>-instanced SVGPathElements, reuse the referenced Path object (now
cached in SVGPath).  This ensures that all <path> instances paint using the
same SkPath, which in turn reduces the pressure on internal Skia path caches.

BUG=skia:4527, 549242
R=fs@opera.com

Committed: https://crrev.com/bc2589c36b60164e0191247e891ee71cbcd47090
Cr-Commit-Position: refs/heads/master@{#357138}

[f(malita), 2015-10-29 16:04:25]

Description was changed from

==========
[SVG] Shared <use> path geometry

WIP, not ready for review/landing.

BUG=skia:4527
R=fs@opera.com
==========

to

==========
[SVG] Shared <use> path geometry

WIP, not ready for review/landing.

BUG=skia:4527
R=fs@opera.com
==========

[f(malita), 2015-10-29 16:06:35]

This is not ready for review/landing, but sharing for early feedback.  Fredrik,
is this what you had in mind?

I think what's missing is some cleanup in LayoutSVGShape because now we're
caching the path in two places, which doesn't makes sense.

[fs, 2015-10-29 16:22:08]

On 2015/10/29 at 16:06:35, fmalita wrote:
> This is not ready for review/landing, but sharing for early feedback. 
Fredrik, is this what you had in mind?
> 
> I think what's missing is some cleanup in LayoutSVGShape because now we're
caching the path in two places, which doesn't makes sense.

Well I actual thought the thing you put in SVGPathElement should go in SVGPath
(the successor for SVGPathSegList) together with the actual
buildPathFromByteStream(...) call. I don't think clean up in LayoutSVGShape
would be needed - it should just be holding onto the same SkPath(Ref) anyway -
right? Could probably ditch the OwnPtr there though (even without something like
this.)

That being said though, this does look like it should be functionally equivalent
and good enough to gauge the effect.

[f(malita), 2015-10-29 16:49:25]

On 2015/10/29 16:22:08, fs wrote:
> On 2015/10/29 at 16:06:35, fmalita wrote:
> > This is not ready for review/landing, but sharing for early feedback. 
> Fredrik, is this what you had in mind?
> > 
> > I think what's missing is some cleanup in LayoutSVGShape because now we're
> caching the path in two places, which doesn't makes sense.
> 
> Well I actual thought the thing you put in SVGPathElement should go in SVGPath
> (the successor for SVGPathSegList) together with the actual
> buildPathFromByteStream(...) call.

Ah, I see you've been doing some major refactoring in this area!  I think I
understand what you're suggesting, but one thing that looks trickier for SVGPath
is the invalidation logic: there's no single mutation entry point (or notifier)
AFAICT, so I presume we'll have to audit all methods and inval whenever the
bytestream gets changed?


> I don't think clean up in LayoutSVGShape
> would be needed - it should just be holding onto the same SkPath(Ref) anyway -
> right? Could probably ditch the OwnPtr there though (even without something
like
> this.)

Right, the actual SkPathRef should be shared, but there's a lot of state
management around that cache (and allocation overhead) which would be nice to
get rid of.  Maybe as a followup.


> That being said though, this does look like it should be functionally
equivalent
> and good enough to gauge the effect.

Yup, it certainly seems to work/fix GPU perf.

Since you're a lot more familiar with this area than I am plus this is
essentially your idea, let me know if you want to take over.  I think I'm mostly
getting in your way at this point :)

[fs, 2015-10-29 17:10:31]

On 2015/10/29 at 16:49:25, fmalita wrote:
> On 2015/10/29 16:22:08, fs wrote:
> > On 2015/10/29 at 16:06:35, fmalita wrote:
> > > This is not ready for review/landing, but sharing for early feedback. 
> > Fredrik, is this what you had in mind?
> > > 
> > > I think what's missing is some cleanup in LayoutSVGShape because now we're
> > caching the path in two places, which doesn't makes sense.
> > 
> > Well I actual thought the thing you put in SVGPathElement should go in
SVGPath
> > (the successor for SVGPathSegList) together with the actual
> > buildPathFromByteStream(...) call.
> 
> Ah, I see you've been doing some major refactoring in this area!  I think I
understand what you're suggesting, but one thing that looks trickier for SVGPath
is the invalidation logic: there's no single mutation entry point (or notifier)
AFAICT, so I presume we'll have to audit all methods and inval whenever the
bytestream gets changed?

With the list removed we should be down to essentially two mutation points:
setAttribute (setValueAsString) and the animation code path. Ought to be
possible to make a notifier if it turns out to be needed. (I've toyed a bit with
making this class parse lazily, and it wasn't too big a hassle - and it should
be roughly the same usage pattern I think.)

> > I don't think clean up in LayoutSVGShape
> > would be needed - it should just be holding onto the same SkPath(Ref) anyway
-
> > right? Could probably ditch the OwnPtr there though (even without something
like
> > this.)
> 
> Right, the actual SkPathRef should be shared, but there's a lot of state
management around that cache (and allocation overhead) which would be nice to
get rid of.  Maybe as a followup.
> 
> 
> > That being said though, this does look like it should be functionally
equivalent
> > and good enough to gauge the effect.
> 
> Yup, it certainly seems to work/fix GPU perf.
> 
> Since you're a lot more familiar with this area than I am plus this is
essentially your idea, let me know if you want to take over.  I think I'm mostly
getting in your way at this point :)

Well, I can jot it down on my mental TODO. Should we perhaps open a new bug for
this then? (The one this stemmed from was closed, and I like focused bugs.)

[f(malita), 2015-10-29 18:47:05]

On 2015/10/29 17:10:31, fs wrote:
> On 2015/10/29 at 16:49:25, fmalita wrote:
> > On 2015/10/29 16:22:08, fs wrote:
> > > On 2015/10/29 at 16:06:35, fmalita wrote:
> > > > This is not ready for review/landing, but sharing for early feedback. 
> > > Fredrik, is this what you had in mind?
> > > > 
> > > > I think what's missing is some cleanup in LayoutSVGShape because now
we're
> > > caching the path in two places, which doesn't makes sense.
> > > 
> > > Well I actual thought the thing you put in SVGPathElement should go in
> SVGPath
> > > (the successor for SVGPathSegList) together with the actual
> > > buildPathFromByteStream(...) call.
> > 
> > Ah, I see you've been doing some major refactoring in this area!  I think I
> understand what you're suggesting, but one thing that looks trickier for
SVGPath
> is the invalidation logic: there's no single mutation entry point (or
notifier)
> AFAICT, so I presume we'll have to audit all methods and inval whenever the
> bytestream gets changed?
> 
> With the list removed we should be down to essentially two mutation points:
> setAttribute (setValueAsString) and the animation code path. Ought to be
> possible to make a notifier if it turns out to be needed. (I've toyed a bit
with
> making this class parse lazily, and it wasn't too big a hassle - and it should
> be roughly the same usage pattern I think.)
> 
> > > I don't think clean up in LayoutSVGShape
> > > would be needed - it should just be holding onto the same SkPath(Ref)
anyway
> -
> > > right? Could probably ditch the OwnPtr there though (even without
something
> like
> > > this.)
> > 
> > Right, the actual SkPathRef should be shared, but there's a lot of state
> management around that cache (and allocation overhead) which would be nice to
> get rid of.  Maybe as a followup.
> > 
> > 
> > > That being said though, this does look like it should be functionally
> equivalent
> > > and good enough to gauge the effect.
> > 
> > Yup, it certainly seems to work/fix GPU perf.
> > 
> > Since you're a lot more familiar with this area than I am plus this is
> essentially your idea, let me know if you want to take over.  I think I'm
mostly
> getting in your way at this point :)
> 
> Well, I can jot it down on my mental TODO. Should we perhaps open a new bug
for
> this then? (The one this stemmed from was closed, and I like focused bugs.)

opened https://code.google.com/p/chromium/issues/detail?id=549242

I'll continue fiddling with this then for the next couple of days - let's see if
I can get it in a landable state.  Feel free to jump in/take over whenever it
pops to the top of your list :)

[f(malita), 2015-10-29 18:47:44]

Description was changed from

==========
[SVG] Shared <use> path geometry

WIP, not ready for review/landing.

BUG=skia:4527
R=fs@opera.com
==========

to

==========
[SVG] Shared <use> path geometry

WIP, not ready for review/landing.

BUG=skia:4527,549242
R=fs@opera.com
==========

[fs, 2015-10-29 18:51:19]

On 2015/10/29 at 18:47:05, fmalita wrote:
> On 2015/10/29 17:10:31, fs wrote:
> > On 2015/10/29 at 16:49:25, fmalita wrote:
> > > On 2015/10/29 16:22:08, fs wrote:
> > > > On 2015/10/29 at 16:06:35, fmalita wrote:
> > > > > This is not ready for review/landing, but sharing for early feedback. 
> > > > Fredrik, is this what you had in mind?
> > > > > 
> > > > > I think what's missing is some cleanup in LayoutSVGShape because now
we're
> > > > caching the path in two places, which doesn't makes sense.
> > > > 
> > > > Well I actual thought the thing you put in SVGPathElement should go in
> > SVGPath
> > > > (the successor for SVGPathSegList) together with the actual
> > > > buildPathFromByteStream(...) call.
> > > 
> > > Ah, I see you've been doing some major refactoring in this area!  I think
I
> > understand what you're suggesting, but one thing that looks trickier for
SVGPath
> > is the invalidation logic: there's no single mutation entry point (or
notifier)
> > AFAICT, so I presume we'll have to audit all methods and inval whenever the
> > bytestream gets changed?
> > 
> > With the list removed we should be down to essentially two mutation points:
> > setAttribute (setValueAsString) and the animation code path. Ought to be
> > possible to make a notifier if it turns out to be needed. (I've toyed a bit
with
> > making this class parse lazily, and it wasn't too big a hassle - and it
should
> > be roughly the same usage pattern I think.)
> > 
> > > > I don't think clean up in LayoutSVGShape
> > > > would be needed - it should just be holding onto the same SkPath(Ref)
anyway
> > -
> > > > right? Could probably ditch the OwnPtr there though (even without
something
> > like
> > > > this.)
> > > 
> > > Right, the actual SkPathRef should be shared, but there's a lot of state
> > management around that cache (and allocation overhead) which would be nice
to
> > get rid of.  Maybe as a followup.
> > > 
> > > 
> > > > That being said though, this does look like it should be functionally
> > equivalent
> > > > and good enough to gauge the effect.
> > > 
> > > Yup, it certainly seems to work/fix GPU perf.
> > > 
> > > Since you're a lot more familiar with this area than I am plus this is
> > essentially your idea, let me know if you want to take over.  I think I'm
mostly
> > getting in your way at this point :)
> > 
> > Well, I can jot it down on my mental TODO. Should we perhaps open a new bug
for
> > this then? (The one this stemmed from was closed, and I like focused bugs.)
> 
> opened https://code.google.com/p/chromium/issues/detail?id=549242

Thanks.
 
> I'll continue fiddling with this then for the next couple of days - let's see
if I can get it in a landable state.

Ok.

> Feel free to jump in/take over whenever it pops to the top of your list :)

*Looks at (virtual) list*

Yees,,,

[f(malita), 2015-10-29 20:39:38]

Description was changed from

==========
[SVG] Shared <use> path geometry

WIP, not ready for review/landing.

BUG=skia:4527,549242
R=fs@opera.com
==========

to

==========
[SVG] Shared <use> path geometry
wwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwww

Based on Fredrik Söderquist <fs@opera.com>'s idea.

For <use>-instanced SVGPathElements, reuse the referenced Path object (now
cached in SVGPath).  This ensures that all <path> instances paint using the
same SkPath, which in turn reduces the pressure on internal Skia path caches.

BUG=skia:4527,549242
R=fs@opera.com
==========

[f(malita), 2015-10-29 20:39:48]

Description was changed from

==========
[SVG] Shared <use> path geometry
wwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwww

Based on Fredrik Söderquist <fs@opera.com>'s idea.

For <use>-instanced SVGPathElements, reuse the referenced Path object (now
cached in SVGPath).  This ensures that all <path> instances paint using the
same SkPath, which in turn reduces the pressure on internal Skia path caches.

BUG=skia:4527,549242
R=fs@opera.com
==========

to

==========
[SVG] Shared <use> path geometry

Based on Fredrik Söderquist <fs@opera.com>'s idea.

For <use>-instanced SVGPathElements, reuse the referenced Path object (now
cached in SVGPath).  This ensures that all <path> instances paint using the
same SkPath, which in turn reduces the pressure on internal Skia path caches.

BUG=skia:4527,549242
R=fs@opera.com
==========

[f(malita), 2015-10-30 00:27:01]

Updated, PTAL.

[fs, 2015-10-30 09:48:33]

fs@opera.com changed reviewers:
+ pdr@chromium.org

[fs, 2015-10-30 09:48:34]

On 2015/10/30 at 00:27:01, fmalita wrote:
> Updated, PTAL.

Thanks, this looks pretty much exactly like what I had in mind. LGTM w/ nit.
Let's try to prevent any "siloing" by having an "external party" vet it.

+pdr

https://codereview.chromium.org/1425913004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.h (right):

https://codereview.chromium.org/1425913004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.h:133: //
TODO(fmalita): the Path is now cached in SVGPath; while this additional cache is
just a
When (if) we manage to turn 'd' into a property well essentially be carry a Path
in ComputedStyle, so hopefully that would fix some part of this. There's still
the cases of all the non-<path> geometries that we turn into Paths for various
reasons, so I guess we need to think some more about that...

https://codereview.chromium.org/1425913004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGPathElement.cpp (right):

https://codereview.chromium.org/1425913004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGPathElement.cpp:76: if (const auto*
element = correspondingElement())
Nit: I'd prefer s/auto/SVGElement/ here - there's no complications worth
"inferring".

[f(malita), 2015-10-30 12:42:56]

https://codereview.chromium.org/1425913004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGPathElement.cpp (right):

https://codereview.chromium.org/1425913004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGPathElement.cpp:76: if (const auto*
element = correspondingElement())
On 2015/10/30 09:48:34, fs wrote:
> Nit: I'd prefer s/auto/SVGElement/ here - there's no complications worth
> "inferring".

Done.

[pdr., 2015-10-30 16:46:01]

LGTM

https://codereview.chromium.org/1425913004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGPath.h (right):

https://codereview.chromium.org/1425913004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGPath.h:77: OwnPtr<SVGPathByteStream>
m_byteStream;
I hope we can combine the byte stream and skpath at some point.

[f(malita), 2015-10-30 17:35:52]

The CQ bit was checked by fmalita@chromium.org

[f(malita), 2015-10-30 17:35:52]

The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com
Link to the patchset: https://codereview.chromium.org/1425913004/#ps60001
(title: "review")

[commit-bot: I haz the power, 2015-10-30 17:36:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1425913004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1425913004/60001

[commit-bot: I haz the power, 2015-10-30 18:31:51]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-30 18:32:40]

Patchset 4 (id:??) landed as
https://crrev.com/bc2589c36b60164e0191247e891ee71cbcd47090
Cr-Commit-Position: refs/heads/master@{#357138}

[Stephen White, 2015-10-30 19:52:59]

\o/