Introduce SVGPaintServer

Introduce SVGPaintServer

Fetch the actual fill from the various paint-server RenderSVGResources
into a new "data blob" SVGPaintServer. This makes said resources
independent of the GC, and means that GC-state can be managed more
directly by the caller.
Since this makes postApplyResource useless, it's removed, and
applyResource is renamed to preparePaintServer.
Some functions residing on RenderSVGResource is moved to SVGPaintServer.
Paint-server fetch+application is moved to updateGraphicsContext, which
is also changed to return a bool indicating if a paint-server is
available. Handling of clip-path-as-mask is also moved here.

BUG=420022
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183619

[fs, 2014-10-08 14:23:13]

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

[pdr., 2014-10-09 00:58:04]

pdr@chromium.org changed reviewers:
+ chrishtr@chromium.org

[pdr., 2014-10-09 01:00:50]

+cc chrishtr, because this is our first non-*Painter classes in painting.


This is a fantastic cleanup, comments below.

https://codereview.chromium.org/638933002/diff/20001/Source/core/paint/SVGPai...
File Source/core/paint/SVGPaintServer.cpp (right):

https://codereview.chromium.org/638933002/diff/20001/Source/core/paint/SVGPai...
Source/core/paint/SVGPaintServer.cpp:66: bool
SVGPaintServer::requestForRenderer(RenderObject& renderer, RenderStyle* style,
RenderSVGResourceModeFlags resourceModeFlags, SVGPaintServer& paintServer)
Can we create and return the paintserver from this function instead of passing
it in? Using an OwnPtr<SVGPaintServer>, returning 0 can signify the failure
mode.

An alternative is to create the paint server in preparePaintServer which would
let you set the gradient/color/pattern in the constructor so only a
setFallbackColor function would be needed on SVGPaintServer.

https://codereview.chromium.org/638933002/diff/20001/Source/core/paint/SVGPai...
File Source/core/paint/SVGPaintServer.h (right):

https://codereview.chromium.org/638933002/diff/20001/Source/core/paint/SVGPai...
Source/core/paint/SVGPaintServer.h:19: SVGPaintServer();
This can be private.

https://codereview.chromium.org/638933002/diff/20001/Source/core/paint/SVGPai...
Source/core/paint/SVGPaintServer.h:26: static bool
updateGraphicsContext(GraphicsContext*, RenderStyle*, RenderObject&,
RenderSVGResourceModeFlags, GraphicsContextStateSaver&);
Passing in both a context and a statesaver that holds the context is messy. Can
we just expose the context on GraphicsContextStateSaver and just pass that in?

https://codereview.chromium.org/638933002/diff/20001/Source/core/paint/SVGPai...
Source/core/paint/SVGPaintServer.h:39: Color m_color;
This makes me a little sad because a paint server for the color black is 12
bytes. Because this won't simultaneously contain a gradient and a pattern, can
we do something smarter?

[fs, 2014-10-09 07:23:08]

On 2014/10/09 01:00:50, pdr wrote:
> +cc chrishtr, because this is our first non-*Painter classes in painting.

Figured I'd try the waters a bit - aside from the use in hit-testing
(RenderSVGShape) this pretty firmly in the "painting" realm.

I'm travelling/OOO today, but hopefully I'll get to the below in the evening.

https://codereview.chromium.org/638933002/diff/20001/Source/core/paint/SVGPai...
File Source/core/paint/SVGPaintServer.cpp (right):

https://codereview.chromium.org/638933002/diff/20001/Source/core/paint/SVGPai...
Source/core/paint/SVGPaintServer.cpp:66: bool
SVGPaintServer::requestForRenderer(RenderObject& renderer, RenderStyle* style,
RenderSVGResourceModeFlags resourceModeFlags, SVGPaintServer& paintServer)
On 2014/10/09 01:00:50, pdr wrote:
> Can we create and return the paintserver from this function instead of passing
> it in? Using an OwnPtr<SVGPaintServer>, returning 0 can signify the failure
> mode.

Definitely possible.

> An alternative is to create the paint server in preparePaintServer which would
> let you set the gradient/color/pattern in the constructor so only a
> setFallbackColor function would be needed on SVGPaintServer.

I like the idea of reducing/removing the setters. I'll have to try it though.

https://codereview.chromium.org/638933002/diff/20001/Source/core/paint/SVGPai...
File Source/core/paint/SVGPaintServer.h (right):

https://codereview.chromium.org/638933002/diff/20001/Source/core/paint/SVGPai...
Source/core/paint/SVGPaintServer.h:26: static bool
updateGraphicsContext(GraphicsContext*, RenderStyle*, RenderObject&,
RenderSVGResourceModeFlags, GraphicsContextStateSaver&);
On 2014/10/09 01:00:50, pdr wrote:
> Passing in both a context and a statesaver that holds the context is messy.
Can
> we just expose the context on GraphicsContextStateSaver and just pass that in?

It's already exposed, I just don't find it particularly clean to do it that
way...

https://codereview.chromium.org/638933002/diff/20001/Source/core/paint/SVGPai...
Source/core/paint/SVGPaintServer.h:39: Color m_color;
On 2014/10/09 01:00:50, pdr wrote:
> This makes me a little sad because a paint server for the color black is 12
> bytes. Because this won't simultaneously contain a gradient and a pattern, can
> we do something smarter?

I'm glad you asked =). It'd be possible to add a common superclass for
Gradient/Pattern, and then we'd only need 8 bytes... (or 16 vs. 24 for 64-bit).
This is not intended for longer term (heap) storage though, so I'd
probably/actually worry more of number of destructors to call etc.

[chrishtr, 2014-10-09 17:41:56]

The parts of PaintServer that are for paint should be separate from the ones
that are not. It's an important property of the paint/ code that it be
self-contained, so we can more easily understand and refactor it.

How about two classes?

[fs, 2014-10-10 08:17:10]

On 2014/10/09 17:41:56, chrishtr wrote:
> The parts of PaintServer that are for paint should be separate from the ones
> that are not. It's an important property of the paint/ code that it be
> self-contained, so we can more easily understand and refactor it.
> 
> How about two classes?

In practice that just means dropping the existsForRenderer(...) function - which
is certainly doable. Will do that then.

[fs, 2014-10-10 13:08:59]

Stuff happened. PTAL.

https://codereview.chromium.org/638933002/diff/20001/Source/core/paint/SVGPai...
File Source/core/paint/SVGPaintServer.cpp (right):

https://codereview.chromium.org/638933002/diff/20001/Source/core/paint/SVGPai...
Source/core/paint/SVGPaintServer.cpp:66: bool
SVGPaintServer::requestForRenderer(RenderObject& renderer, RenderStyle* style,
RenderSVGResourceModeFlags resourceModeFlags, SVGPaintServer& paintServer)
On 2014/10/09 07:23:08, fs wrote:
> On 2014/10/09 01:00:50, pdr wrote:
> > Can we create and return the paintserver from this function instead of
passing
> > it in? Using an OwnPtr<SVGPaintServer>, returning 0 can signify the failure
> > mode.
> 
> Definitely possible.
> 
> > An alternative is to create the paint server in preparePaintServer which
would
> > let you set the gradient/color/pattern in the constructor so only a
> > setFallbackColor function would be needed on SVGPaintServer.
> 
> I like the idea of reducing/removing the setters. I'll have to try it though.

Made SVGPaintServer more RAII (per ~above), and started passing it via the
return-value instead of an out-parameter (definitely looks a bit nicer).

https://codereview.chromium.org/638933002/diff/20001/Source/core/paint/SVGPai...
File Source/core/paint/SVGPaintServer.h (right):

https://codereview.chromium.org/638933002/diff/20001/Source/core/paint/SVGPai...
Source/core/paint/SVGPaintServer.h:26: static bool
updateGraphicsContext(GraphicsContext*, RenderStyle*, RenderObject&,
RenderSVGResourceModeFlags, GraphicsContextStateSaver&);
On 2014/10/09 07:23:08, fs wrote:
> On 2014/10/09 01:00:50, pdr wrote:
> > Passing in both a context and a statesaver that holds the context is messy.
> Can
> > we just expose the context on GraphicsContextStateSaver and just pass that
in?
> 
> It's already exposed, I just don't find it particularly clean to do it that
> way...

Dropped GC*, moved GCSS in the argument list. (I hope to eventually be able to
split "fetch paint-server" from "apply paint-server".)

https://codereview.chromium.org/638933002/diff/20001/Source/core/paint/SVGPai...
Source/core/paint/SVGPaintServer.h:39: Color m_color;
On 2014/10/09 07:23:08, fs wrote:
> On 2014/10/09 01:00:50, pdr wrote:
> > This makes me a little sad because a paint server for the color black is 12
> > bytes. Because this won't simultaneously contain a gradient and a pattern,
can
> > we do something smarter?
> 
> I'm glad you asked =). It'd be possible to add a common superclass for
> Gradient/Pattern, and then we'd only need 8 bytes... (or 16 vs. 24 for
64-bit).
> This is not intended for longer term (heap) storage though, so I'd
> probably/actually worry more of number of destructors to call etc.

I haven't done anything about this. If "people" find the above suggestion (about
adding a common base for Gradient and Pattern) reasonable I'll upload a separate
CL for that (should shrink/simplify GC-state as well.)

[f(malita), 2014-10-10 15:49:07]

https://codereview.chromium.org/638933002/diff/170001/Source/core/paint/SVGIn...
File Source/core/paint/SVGInlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/638933002/diff/170001/Source/core/paint/SVGIn...
Source/core/paint/SVGInlineTextBoxPainter.cpp:339: if (hasShadow &&
!stateSaver.saved())
Could we saveIfNeeded in the hasShadow conditional above instead, to reduce GC
state micromanagement?

https://codereview.chromium.org/638933002/diff/170001/Source/core/paint/SVGPa...
File Source/core/paint/SVGPaintServer.cpp (right):

https://codereview.chromium.org/638933002/diff/170001/Source/core/paint/SVGPa...
Source/core/paint/SVGPaintServer.cpp:92: OwnPtr<SVGPaintServer> paintServer =
requestForRenderer(renderer, style, resourceModeFlags);
If we're not caching these, it seems wasteful to allocate them dynamically (even
with WTF_MAKE_FAST_ALLOCATED). I'd prefer to keep them stack-allocated for now.
If not, we should make sure the allocator churn is not hurting perf (we're
allocating/deallocating a paint server every time we paint an object now).

[fs, 2014-10-10 16:10:23]

https://codereview.chromium.org/638933002/diff/170001/Source/core/paint/SVGIn...
File Source/core/paint/SVGInlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/638933002/diff/170001/Source/core/paint/SVGIn...
Source/core/paint/SVGInlineTextBoxPainter.cpp:339: if (hasShadow &&
!stateSaver.saved())
On 2014/10/10 15:49:07, Florin Malita wrote:
> Could we saveIfNeeded in the hasShadow conditional above instead, to reduce GC
> state micromanagement?

Done.

https://codereview.chromium.org/638933002/diff/170001/Source/core/paint/SVGPa...
File Source/core/paint/SVGPaintServer.cpp (right):

https://codereview.chromium.org/638933002/diff/170001/Source/core/paint/SVGPa...
Source/core/paint/SVGPaintServer.cpp:92: OwnPtr<SVGPaintServer> paintServer =
requestForRenderer(renderer, style, resourceModeFlags);
On 2014/10/10 15:49:07, Florin Malita wrote:
> If we're not caching these, it seems wasteful to allocate them dynamically
(even
> with WTF_MAKE_FAST_ALLOCATED). I'd prefer to keep them stack-allocated for
now.
> If not, we should make sure the allocator churn is not hurting perf (we're
> allocating/deallocating a paint server every time we paint an object now).
> 

Aha! Reviewer inconsistency detected - parity error. Scrub attempted.

I suppose I can appease you both with something like:

class SVGPaintServer {
  ...
  bool operator!() const { return !(m_color.rgba() || m_gradient || m_pattern);
}
};

(works assuming no "weird" blend-modes - which I think can be assumed ATM.)

I'll upload that shortly, but I'd be nice to get a uniform reviewer opinion
here.

[f(malita), 2014-10-10 16:44:20]

On 2014/10/10 16:10:23, fs wrote:
>
https://codereview.chromium.org/638933002/diff/170001/Source/core/paint/SVGPa...
> Source/core/paint/SVGPaintServer.cpp:92: OwnPtr<SVGPaintServer> paintServer =
> requestForRenderer(renderer, style, resourceModeFlags);
> On 2014/10/10 15:49:07, Florin Malita wrote:
> > If we're not caching these, it seems wasteful to allocate them dynamically
> (even
> > with WTF_MAKE_FAST_ALLOCATED). I'd prefer to keep them stack-allocated for
> now.
> > If not, we should make sure the allocator churn is not hurting perf (we're
> > allocating/deallocating a paint server every time we paint an object now).
> > 
> 
> Aha! Reviewer inconsistency detected - parity error. Scrub attempted.

Hehe, it happens :)

A quick local test does seem to show a minor (2-4%) repaint regression
(SierpinskiCarpet.svg continuous repaint time for example goes ~74ms -> ~77ms).
I'm always wary of allocator overhead, and even though the fast allocator
mitigates most of it I would argue it's a good idea in general to avoid
heap-allocated transient objects.

[fs, 2014-10-10 16:44:42]

https://codereview.chromium.org/638933002/diff/170001/Source/core/paint/SVGPa...
File Source/core/paint/SVGPaintServer.cpp (right):

https://codereview.chromium.org/638933002/diff/170001/Source/core/paint/SVGPa...
Source/core/paint/SVGPaintServer.cpp:92: OwnPtr<SVGPaintServer> paintServer =
requestForRenderer(renderer, style, resourceModeFlags);
On 2014/10/10 16:10:23, fs wrote:
> On 2014/10/10 15:49:07, Florin Malita wrote:
> > If we're not caching these, it seems wasteful to allocate them dynamically
> (even
> > with WTF_MAKE_FAST_ALLOCATED). I'd prefer to keep them stack-allocated for
> now.
> > If not, we should make sure the allocator churn is not hurting perf (we're
> > allocating/deallocating a paint server every time we paint an object now).
> > 
> 
> Aha! Reviewer inconsistency detected - parity error. Scrub attempted.
> 
> I suppose I can appease you both with something like:
> 
> class SVGPaintServer {
>   ...
>   bool operator!() const { return !(m_color.rgba() || m_gradient ||
m_pattern);
> }
> };
> 
> (works assuming no "weird" blend-modes - which I think can be assumed ATM.)
> 
> I'll upload that shortly, but I'd be nice to get a uniform reviewer opinion
> here.

Can be found in PS7+ - I went with an explicit isValid instead of operator.
Could also just use a "bool m_isValid" if preferable.

[chrishtr, 2014-10-10 16:59:20]

https://codereview.chromium.org/638933002/diff/690001/Source/core/rendering/s...
File Source/core/rendering/svg/RenderSVGResourcePattern.cpp (right):

https://codereview.chromium.org/638933002/diff/690001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGResourcePattern.cpp:148: return
SVGPaintServer(patternData->pattern);
Why not just have a method that exposes the data needed to construct the
SVGPaintServer, and have the constructor call entirely in the paint/ directory?
That would seem to be a cleaner separation of style and details of painting
algorithms, and avoid the need to link against something in the paint/ directory
here.

[f(malita), 2014-10-10 17:33:08]

On 2014/10/10 16:44:42, fs wrote:
>
https://codereview.chromium.org/638933002/diff/170001/Source/core/paint/SVGPa...
> File Source/core/paint/SVGPaintServer.cpp (right):
> > I'll upload that shortly, but I'd be nice to get a uniform reviewer opinion
> > here.
> 
> Can be found in PS7+ - I went with an explicit isValid instead of operator.
> Could also just use a "bool m_isValid" if preferable.

Thanks, the new version looks good perf-wise. I'm fine with whatever API gets
endorsed by pdr & chrishtr.

[fs, 2014-10-11 12:05:51]

https://codereview.chromium.org/638933002/diff/690001/Source/core/rendering/s...
File Source/core/rendering/svg/RenderSVGResourcePattern.cpp (right):

https://codereview.chromium.org/638933002/diff/690001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGResourcePattern.cpp:148: return
SVGPaintServer(patternData->pattern);
On 2014/10/10 16:59:20, chrishtr wrote:
> Why not just have a method that exposes the data needed to construct the
> SVGPaintServer, and have the constructor call entirely in the paint/
directory?
> That would seem to be a cleaner separation of style and details of painting
> algorithms, and avoid the need to link against something in the paint/
directory
> here.

Then I'd have to think, that it'd be better to just move SVGPaintServer to
rendering/ and move the updateGraphicsContext helper to SVGRenderSupport (for
now - it should be possible to move to paint/ in due time). Otherwise, since
there exists no one type this method can return, we'd need to build another
parameter blob - either exactly the same (structurally) as SVGPaintServer or a
conglomerate of *GradientAttributes and similar for Pattern [1] ( essentially
PatternData/GradientData but w/ a common base-class and different contents).
With the per-renderer caching moved out the latter seems more tractable [2], but
I'm 1-2 CLs away from being in a decent position for doing that.

So does the suggestion in the first sentence sound good enough for now? I'd be
happy to try and work towards the "larger param-blob" thingy and the caching
separation (which I was planning to do anyway, since there some stuff that would
benefit from that already). What side should do the caching could probably be
discussed further.

I must say though, that it's pretty darn hard to adapt to a design that you're
fed piece-meal on a need-to-know basis - feels a bit like being back in the army
;-). (Requested access to the doc, but no luck accessing it yet.)

Anyway, thanks for looking. I'll try to get something uploaded monday
(discussions are unfortunately not very practical w/ a ~1h window and 20h
latency).

[1] transform + display list/imagebuffer (or maybe root/list of content
elements/renderers or something)
[2] I.e. then you'd have a context blob in, and a parameter blob out.

[pdr., 2014-10-11 21:32:40]

On 2014/10/11 at 12:05:51, fs wrote:
>
https://codereview.chromium.org/638933002/diff/690001/Source/core/rendering/s...
> File Source/core/rendering/svg/RenderSVGResourcePattern.cpp (right):
> 
>
https://codereview.chromium.org/638933002/diff/690001/Source/core/rendering/s...
> Source/core/rendering/svg/RenderSVGResourcePattern.cpp:148: return
SVGPaintServer(patternData->pattern);
> On 2014/10/10 16:59:20, chrishtr wrote:
> > Why not just have a method that exposes the data needed to construct the
> > SVGPaintServer, and have the constructor call entirely in the paint/
directory?
> > That would seem to be a cleaner separation of style and details of painting
> > algorithms, and avoid the need to link against something in the paint/
directory
> > here.
> 
> Then I'd have to think, that it'd be better to just move SVGPaintServer to
rendering/ and move the updateGraphicsContext helper to SVGRenderSupport (for
now - it should be possible to move to paint/ in due time). Otherwise, since
there exists no one type this method can return, we'd need to build another
parameter blob - either exactly the same (structurally) as SVGPaintServer or a
conglomerate of *GradientAttributes and similar for Pattern [1] ( essentially
PatternData/GradientData but w/ a common base-class and different contents).
With the per-renderer caching moved out the latter seems more tractable [2], but
I'm 1-2 CLs away from being in a decent position for doing that.
> 
> So does the suggestion in the first sentence sound good enough for now? I'd be
happy to try and work towards the "larger param-blob" thingy and the caching
separation (which I was planning to do anyway, since there some stuff that would
benefit from that already). What side should do the caching could probably be
discussed further.

Your suggestion makes sense to me. Lets keep this in rendering/ and move it to
painting in a followup.

> 
> I must say though, that it's pretty darn hard to adapt to a design that you're
fed piece-meal on a need-to-know basis - feels a bit like being back in the army
;-). (Requested access to the doc, but no luck accessing it yet.)

In the future, please make noise before things get to this level. Email me
directly the next time this happens.

This doc was unnecessarily restricted to chromium accounts. This was likely by
accident because that's the default access level. I just now learned the owner
didn't open it for you after I asked yesterday. I've created a copy just for you
at
https://docs.google.com/a/chromium.org/document/d/19Gzwqob59j5fOpOINCH-HjN2-h....
The doc is a bit stale but the big ides are there. If you would like a @chromium
account, I can get that for you too.

The mailing lists we are using are:
https://groups.google.com/a/chromium.org/forum/#!forum/silk-dev
and
https://groups.google.com/a/chromium.org/forum/#!forum/slimming-paint-reviews

> 
> Anyway, thanks for looking. I'll try to get something uploaded monday
(discussions are unfortunately not very practical w/ a ~1h window and 20h
latency).
> 
> [1] transform + display list/imagebuffer (or maybe root/list of content
elements/renderers or something)
> [2] I.e. then you'd have a context blob in, and a parameter blob out.

[fs, 2014-10-13 11:38:33]

Code shuffled.

On 2014/10/11 21:32:40, pdr wrote:
> On 2014/10/11 at 12:05:51, fs wrote:
> > I must say though, that it's pretty darn hard to adapt to a design that
you're
> fed piece-meal on a need-to-know basis - feels a bit like being back in the
army
> ;-). (Requested access to the doc, but no luck accessing it yet.)
> 
> In the future, please make noise before things get to this level. Email me
> directly the next time this happens.
> 
> This doc was unnecessarily restricted to chromium accounts. This was likely by
> accident because that's the default access level. I just now learned the owner
> didn't open it for you after I asked yesterday. I've created a copy just for
you
> at
>
https://docs.google.com/a/chromium.org/document/d/19Gzwqob59j5fOpOINCH-HjN2-h....
> The doc is a bit stale but the big ides are there. If you would like a
@chromium
> account, I can get that for you too.
> 
> The mailing lists we are using are:
> https://groups.google.com/a/chromium.org/forum/#!forum/silk-dev
> and
> https://groups.google.com/a/chromium.org/forum/#!forum/slimming-paint-reviews

Thanks for links and docs. I'm busy reading through all of it now.

[chrishtr, 2014-10-13 15:54:23]

Thanks for the updates. I'll defer to pdr@ for the final approval since he knows
this code better than me.

[pdr., 2014-10-13 17:08:56]

The CQ bit was checked by pdr@chromium.org

[pdr., 2014-10-13 17:08:57]

LGTM

https://codereview.chromium.org/638933002/diff/920001/Source/core/rendering/s...
File Source/core/rendering/svg/RenderSVGResource.h (right):

https://codereview.chromium.org/638933002/diff/920001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGResource.h:65: bool isValid() const { return
m_color != Color::transparent; }
(For a followup) What do you think about isNonTransparent() or similar? It seems
odd that a user can create an invalid color.

[commit-bot: I haz the power, 2014-10-13 17:09:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/638933002/920001

[commit-bot: I haz the power, 2014-10-13 17:13:50]

Committed patchset #10 (id:920001) as 183619