client-goodbye

client-goodbye

- GraphicsLayer invalidation uses InvalidationDebugging to access LayoutObject
- PaintController doesn't work with LayoutObjects tho for similar things.. idk
- Most recorders take the info up front instead of taking a client
- Not all.. DrawingRecorder.. *cough*
- And they should only take the info up front in DCHECKs mode.

R=enne@chromium.org, pdr@chromium.org
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

[danakj, 2017-02-23 00:16:08]

Description was changed from

==========
client-goodbye

R=enne@chromium.org, pdr@chromium.org
BUG=
==========

to

==========
client-goodbye

R=enne@chromium.org, pdr@chromium.org
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[danakj, 2017-02-23 00:19:24]

Description was changed from

==========
client-goodbye

R=enne@chromium.org, pdr@chromium.org
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
client-goodbye

- GraphicsLayer invalidation uses InvalidationDebugging to access LayoutObject
- PaintController doesn't work with LayoutObjects tho for similar things.. idk
- Most recorders take the info up front instead of taking a client
- Not all.. DrawingRecorder.. *cough*
- And they should only take the info up front in DCHECKs mode.

R=enne@chromium.org, pdr@chromium.org
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[pdr., 2017-02-23 00:58:19]

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

[pdr., 2017-02-23 00:58:28]

+reviewer Xianzhu

[pdr., 2017-02-23 02:06:15]

I think InvalidationDebugging.h got left out of this review.


Xianzhu and I talked about this a bit offline.

Just to make sure I understand--is the goal to make the display item list
serializable, or are there other requirements?

It will likely be too expensive to call debugName in non-dcheck code such as the
recorders. To reduce the amount of code change, could we keep using
DisplayItemClient as the interface to the recorders, then unpack the data
inside? I'm thinking something like:
DanaRecorder::DanaRecorder(GraphicsContext& context,
                                     const DisplayItemClient& client,
                                     const AffineTransform& transform)
    : m_context(context), m_client(client) {
  m_skipRecordingForIdentityTransform = transform.isIdentity();
  if (m_skipRecordingForIdentityTransform)
    return;

  m_context.getPaintController().createAndAppend<BeginDanaDisplayItem>(
      transform,
      client.visualRect(),
      client.paintedOutputOfObjectHasNoEffectRegardlessOfSize()
#if DCHECK_IS_ON()
      , client.debugName()
#endif
      );
}

We could then make DisplayItemClient::debugName DCHECK-only. An exception to
this is the raster invalidation tracking code, but I think we could still use
DisplayItemClient as the interface for this. I'm thinking something like:
void GraphicsLayer::trackRasterInvalidation(const DisplayItemClient& client,
                                            const IntRect& rect,
                                            PaintInvalidationReason reason) {
  ...
  if (m_isTrackingRasterInvalidations) {
    ...
    info.clientDebugName = client.debugNameForInvalidationTesting();
    ...
  }
}

I think the DrawingRecorder issues you're hitting may be for a feature called
"under-invalidation checking". It's not today, but we can put all of that behind
DCHECK.

[danakj, 2017-02-23 21:39:15]

On Wed, Feb 22, 2017 at 9:06 PM, <pdr@chromium.org> wrote:

> I think InvalidationDebugging.h got left out of this review.
>

Oh ya, it's just a one-method interface fwiw.


>
> Xianzhu and I talked about this a bit offline.
>
> Just to make sure I understand--is the goal to make the display item list
> serializable, or are there other requirements?
>

The goal is to replace SkPicture, SkPaint, and related skia structures with
a flat/serializable and introspectible structure.


>
> It will likely be too expensive to call debugName in non-dcheck code such
> as the
> recorders. To reduce the amount of code change, could we keep using
> DisplayItemClient as the interface to the recorders, then unpack the data
> inside?
>

The reason I started going this way is that we can measure the impact of
gathering all the data up front. What you're suggesting also does, but then
requires changing all the internal stuff immediately to no longer use
displayclient, and I was planning to poke at that a bit more incrementally.
Like push the DisplayItemClient creation further inside until it disappears.

Are you saying you'd want to keep an interface to get this data instead of
just constructing and passing it in? That seems undesirable as it requires
methods on the things creating DIsplayItems, and that requires the objects
to hold state in order to return from the methods. Pushing the data instead
of pulling it avoids a round trip anti-pattern and avoids state leaking
into the object's definition from out of the paint method.

wdyt?


> I'm thinking something like:
> DanaRecorder::DanaRecorder(GraphicsContext& context,
> const DisplayItemClient& client,
> const AffineTransform& transform)
> : m_context(context), m_client(client) {
> m_skipRecordingForIdentityTransform = transform.isIdentity();
> if (m_skipRecordingForIdentityTransform)
> return;
>
> m_context.getPaintController().createAndAppend<BeginDanaDisplayItem>(
> transform,
> client.visualRect(),
> client.paintedOutputOfObjectHasNoEffectRegardlessOfSize()
> #if DCHECK_IS_ON()
> , client.debugName()
> #endif
> );
> }
>
> We could then make DisplayItemClient::debugName DCHECK-only. An exception
> to
> this is the raster invalidation tracking code, but I think we could still
> use
> DisplayItemClient as the interface for this. I'm thinking something like:
> void GraphicsLayer::trackRasterInvalidation(const DisplayItemClient&
> client,
> const IntRect& rect,
> PaintInvalidationReason reason) {
> ...
> if (m_isTrackingRasterInvalidations) {
> ...
> info.clientDebugName = client.debugNameForInvalidationTesting();
> ...
> }
> }
>
> I think the DrawingRecorder issues you're hitting may be for a feature
> called
> "under-invalidation checking". It's not today, but we can put all of that
> behind
> DCHECK.
>
> https://codereview.chromium.org/2713673005/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[danakj, 2017-02-23 21:45:02]

On Wed, Feb 22, 2017 at 9:06 PM, <pdr@chromium.org> wrote:

> I think InvalidationDebugging.h got left out of this review.
>

Oh ya, it's just a one-method interface fwiw.


>
> Xianzhu and I talked about this a bit offline.
>
> Just to make sure I understand--is the goal to make the display item list
> serializable, or are there other requirements?
>

The goal is to replace SkPicture, SkPaint, and related skia structures with
a flat/serializable and introspectible structure.


>
> It will likely be too expensive to call debugName in non-dcheck code such
> as the
> recorders. To reduce the amount of code change, could we keep using
> DisplayItemClient as the interface to the recorders, then unpack the data
> inside?
>

The reason I started going this way is that we can measure the impact of
gathering all the data up front. What you're suggesting also does, but then
requires changing all the internal stuff immediately to no longer use
displayclient, and I was planning to poke at that a bit more incrementally.
Like push the DisplayItemClient creation further inside until it disappears.

Are you saying you'd want to keep an interface to get this data instead of
just constructing and passing it in? That seems undesirable as it requires
methods on the things creating DIsplayItems, and that requires the objects
to hold state in order to return from the methods. Pushing the data instead
of pulling it avoids a round trip anti-pattern and avoids state leaking
into the object's definition from out of the paint method.

wdyt?


> I'm thinking something like:
> DanaRecorder::DanaRecorder(GraphicsContext& context,
> const DisplayItemClient& client,
> const AffineTransform& transform)
> : m_context(context), m_client(client) {
> m_skipRecordingForIdentityTransform = transform.isIdentity();
> if (m_skipRecordingForIdentityTransform)
> return;
>
> m_context.getPaintController().createAndAppend<BeginDanaDisplayItem>(
> transform,
> client.visualRect(),
> client.paintedOutputOfObjectHasNoEffectRegardlessOfSize()
> #if DCHECK_IS_ON()
> , client.debugName()
> #endif
> );
> }
>
> We could then make DisplayItemClient::debugName DCHECK-only. An exception
> to
> this is the raster invalidation tracking code, but I think we could still
> use
> DisplayItemClient as the interface for this. I'm thinking something like:
> void GraphicsLayer::trackRasterInvalidation(const DisplayItemClient&
> client,
> const IntRect& rect,
> PaintInvalidationReason reason) {
> ...
> if (m_isTrackingRasterInvalidations) {
> ...
> info.clientDebugName = client.debugNameForInvalidationTesting();
> ...
> }
> }
>
> I think the DrawingRecorder issues you're hitting may be for a feature
> called
> "under-invalidation checking". It's not today, but we can put all of that
> behind
> DCHECK.
>
> https://codereview.chromium.org/2713673005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[pdr., 2017-02-23 22:20:25]

On 2017/02/23 at 21:45:02, danakj wrote:
> On Wed, Feb 22, 2017 at 9:06 PM, <pdr@chromium.org> wrote:
> 
> > I think InvalidationDebugging.h got left out of this review.
> >
> 
> Oh ya, it's just a one-method interface fwiw.
> 
> 
> >
> > Xianzhu and I talked about this a bit offline.
> >
> > Just to make sure I understand--is the goal to make the display item list
> > serializable, or are there other requirements?
> >
> 
> The goal is to replace SkPicture, SkPaint, and related skia structures with
> a flat/serializable and introspectible structure.
> 
> 
> >
> > It will likely be too expensive to call debugName in non-dcheck code such
> > as the
> > recorders. To reduce the amount of code change, could we keep using
> > DisplayItemClient as the interface to the recorders, then unpack the data
> > inside?
> >
> 
> The reason I started going this way is that we can measure the impact of
> gathering all the data up front. What you're suggesting also does, but then
> requires changing all the internal stuff immediately to no longer use
> displayclient, and I was planning to poke at that a bit more incrementally.
> Like push the DisplayItemClient creation further inside until it disappears.
> 
> Are you saying you'd want to keep an interface to get this data instead of
> just constructing and passing it in? That seems undesirable as it requires
> methods on the things creating DIsplayItems, and that requires the objects
> to hold state in order to return from the methods. Pushing the data instead
> of pulling it avoids a round trip anti-pattern and avoids state leaking
> into the object's definition from out of the paint method.
> 
> wdyt?

My main concern with this approach is you're calling debugName in
non-dcheck/debug builds. Won't that be too slow, even if it's stubbed out to
return an empty string?

[danakj, 2017-02-23 23:32:20]

On Thu, Feb 23, 2017 at 5:20 PM, <pdr@chromium.org> wrote:

> On 2017/02/23 at 21:45:02, danakj wrote:
>
> > On Wed, Feb 22, 2017 at 9:06 PM, <pdr@chromium.org&gt; wrote:
> >
> > > I think InvalidationDebugging.h got left out of this review.
> > >
> >
> > Oh ya, it's just a one-method interface fwiw.
> >
> >
> > >
> > > Xianzhu and I talked about this a bit offline.
> > >
> > > Just to make sure I understand--is the goal to make the display item
> list
> > > serializable, or are there other requirements?
> > >
> >
> > The goal is to replace SkPicture, SkPaint, and related skia structures
> with
> > a flat/serializable and introspectible structure.
> >
> >
> > >
> > > It will likely be too expensive to call debugName in non-dcheck code
> such
> > > as the
> > > recorders. To reduce the amount of code change, could we keep using
> > > DisplayItemClient as the interface to the recorders, then unpack the
> data
> > > inside?
> > >
> >
> > The reason I started going this way is that we can measure the impact of
> > gathering all the data up front. What you're suggesting also does, but
> then
> > requires changing all the internal stuff immediately to no longer use
> > displayclient, and I was planning to poke at that a bit more
> incrementally.
> > Like push the DisplayItemClient creation further inside until it
> disappears.
> >
> > Are you saying you'd want to keep an interface to get this data instead
> of
> > just constructing and passing it in? That seems undesirable as it
> requires
> > methods on the things creating DIsplayItems, and that requires the
> objects
> > to hold state in order to return from the methods. Pushing the data
> instead
> > of pulling it avoids a round trip anti-pattern and avoids state leaking
> > into the object's definition from out of the paint method.
> >
> > wdyt?
>
> My main concern with this approach is you're calling debugName in
> non-dcheck/debug builds. Won't that be too slow, even if it's stubbed out
> to
> return an empty string?
>

Ah I didn't sort that out yet. I have a idea I mean to try where instead of
passing debugname+bool we pass a struct. In non-dcheck the struct has no
fields. So non-dcheck builds wouldn't call debugName and fill the field. I
am not sure how it will play out in practice yet.


>
> https://codereview.chromium.org/2713673005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2017-02-23 23:32:20]

On Thu, Feb 23, 2017 at 5:20 PM, <pdr@chromium.org> wrote:

> On 2017/02/23 at 21:45:02, danakj wrote:
>
> > On Wed, Feb 22, 2017 at 9:06 PM, <pdr@chromium.org&gt; wrote:
> >
> > > I think InvalidationDebugging.h got left out of this review.
> > >
> >
> > Oh ya, it's just a one-method interface fwiw.
> >
> >
> > >
> > > Xianzhu and I talked about this a bit offline.
> > >
> > > Just to make sure I understand--is the goal to make the display item
> list
> > > serializable, or are there other requirements?
> > >
> >
> > The goal is to replace SkPicture, SkPaint, and related skia structures
> with
> > a flat/serializable and introspectible structure.
> >
> >
> > >
> > > It will likely be too expensive to call debugName in non-dcheck code
> such
> > > as the
> > > recorders. To reduce the amount of code change, could we keep using
> > > DisplayItemClient as the interface to the recorders, then unpack the
> data
> > > inside?
> > >
> >
> > The reason I started going this way is that we can measure the impact of
> > gathering all the data up front. What you're suggesting also does, but
> then
> > requires changing all the internal stuff immediately to no longer use
> > displayclient, and I was planning to poke at that a bit more
> incrementally.
> > Like push the DisplayItemClient creation further inside until it
> disappears.
> >
> > Are you saying you'd want to keep an interface to get this data instead
> of
> > just constructing and passing it in? That seems undesirable as it
> requires
> > methods on the things creating DIsplayItems, and that requires the
> objects
> > to hold state in order to return from the methods. Pushing the data
> instead
> > of pulling it avoids a round trip anti-pattern and avoids state leaking
> > into the object's definition from out of the paint method.
> >
> > wdyt?
>
> My main concern with this approach is you're calling debugName in
> non-dcheck/debug builds. Won't that be too slow, even if it's stubbed out
> to
> return an empty string?
>

Ah I didn't sort that out yet. I have a idea I mean to try where instead of
passing debugname+bool we pass a struct. In non-dcheck the struct has no
fields. So non-dcheck builds wouldn't call debugName and fill the field. I
am not sure how it will play out in practice yet.


>
> https://codereview.chromium.org/2713673005/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[pdr., 2017-02-23 23:36:37]

On 2017/02/23 at 23:32:20, danakj wrote:
> Ah I didn't sort that out yet. I have a idea I mean to try where instead of
> passing debugname+bool we pass a struct. In non-dcheck the struct has no
> fields. So non-dcheck builds wouldn't call debugName and fill the field. I
> am not sure how it will play out in practice yet.

Cool, my code suggestion above was just a way to do this without having to make
all of the callsites ugly. This isn't a blocker to me, but it seems like it may
be easier for you to just modify the recorders internals (and pass
DisplayItemClient) instead of changing all the callsites. Introducing a
different wrapper (your struct approach) sounds reasonable too.

[danakj, 2017-02-23 23:45:58]

On Thu, Feb 23, 2017 at 6:36 PM, <pdr@chromium.org> wrote:

> On 2017/02/23 at 23:32:20, danakj wrote:
> > Ah I didn't sort that out yet. I have a idea I mean to try where instead
> of
> > passing debugname+bool we pass a struct. In non-dcheck the struct has no
> > fields. So non-dcheck builds wouldn't call debugName and fill the field.
> I
> > am not sure how it will play out in practice yet.
>
> Cool, my code suggestion above was just a way to do this without having to
> make
> all of the callsites ugly. This isn't a blocker to me, but it seems like
> it may
> be easier for you to just modify the recorders internals (and pass
> DisplayItemClient) instead of changing all the callsites. Introducing a
> different wrapper (your struct approach) sounds reasonable too.
>

Ok cool. The trick will be to do it without a struct client.. lol :) It may
end up being DisplayItemClient round 2 XD


>
> https://codereview.chromium.org/2713673005/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[danakj, 2017-02-23 23:51:45]

On Thu, Feb 23, 2017 at 6:36 PM, <pdr@chromium.org> wrote:

> On 2017/02/23 at 23:32:20, danakj wrote:
> > Ah I didn't sort that out yet. I have a idea I mean to try where instead
> of
> > passing debugname+bool we pass a struct. In non-dcheck the struct has no
> > fields. So non-dcheck builds wouldn't call debugName and fill the field.
> I
> > am not sure how it will play out in practice yet.
>
> Cool, my code suggestion above was just a way to do this without having to
> make
> all of the callsites ugly. This isn't a blocker to me, but it seems like
> it may
> be easier for you to just modify the recorders internals (and pass
> DisplayItemClient) instead of changing all the callsites. Introducing a
> different wrapper (your struct approach) sounds reasonable too.
>

Ok cool. The trick will be to do it without a struct client.. lol :) It may
end up being DisplayItemClient round 2 XD


>
> https://codereview.chromium.org/2713673005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.