cc: Reject CompositorFrames to old child surfaces

cc: Reject CompositorFrames to old child surfaces

Currently, if a parent is blocked on a child SurfaceID that does not 
yet have an active CompositorFrame, the child may continue to submit 
CompositorFrames to the old SurfaceID. This eats up CPU cycles that 
could be better spent letting the child catch up to the parent's 
synchronization request.

With this CL, when a parent submits a CompositorFrame that is blocked 
on unresolved dependencies, then existing referenced surfaces with 
corresponding dependencies (of the same FrameSinkId) will be "closed", 
preventing further CompositorFrames from being submitted to them. 
Instead, resources will be immediately returned to the client, and a
DidReceiveCompositorFrameAck will be immediately issued. This reduces 
the latency for a child client to catch up to the parent's request. 

BUG=672962, 711948
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2831213004
Cr-Commit-Position: refs/heads/master@{#469798}
Committed: https://chromium.googlesource.com/chromium/src/+/980c8ee750096770976476654d13b5ecf412bee4

[Fady Samuel, 2017-04-20 22:53:06]

Description was changed from

==========
WIP: Lock Fallback Surfaces during synchronization

BUG=
==========

to

==========
WIP: Lock Fallback Surfaces during synchronization

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Fady Samuel, 2017-05-01 20:34:32]

Description was changed from

==========
WIP: Lock Fallback Surfaces during synchronization

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Reject CompositorFrames to old child surfaces while parent blocked

Currently, if a parent is blocked on a child SurfaceID that does not yet have an
active CompositorFrame, the child may continue to submit CompositorFrames to
the old SurfaceID. This eats up CPU cycles that could be better spent letting
the
child catch up to the parent's synchronization request.

With this CL, when a parent submits a CompositorFrame that is blocked on 
unresolved dependencies, then existing referenced surfaces (which HAVE active
frames) will be "locked", preventing further CompositorFrames from being
submitted
to them. Instead, resources will be immediately returned to the client, and a
DidReceiveCompositorFrameAck will be immediately issued. This reduces the
latency
for a child client to catch up to the parent's request. Once a parent
CompositorFrame
activates then we allow submitting CompositorFrames to old surface IDs again.
This
is useful in surface synchronization if a deadline hits, and the primary surface
is not
yet available. By unlocking the old surface, we keep the child animated until it
catches
up with the parent.

BUG=672962
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Fady Samuel, 2017-05-01 20:35:09]

fsamuel@chromium.org changed reviewers:
+ danakj@chromium.org

[Fady Samuel, 2017-05-01 20:35:09]

PTAL Dana! This is similar to skipping frames in DelegatedFrameHost today.
Thanks!

[Fady Samuel, 2017-05-01 20:35:24]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 20:36:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Fady Samuel, 2017-05-01 20:52:41]

Note that I'm planning an optimization after this. If a surface ID's framesink
ID is a reference but not a dependency then we don't lock its surface,.

[commit-bot: I haz the power, 2017-05-01 21:15:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-01 21:16:00]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[danakj, 2017-05-01 21:33:30]

On 2017/05/01 20:35:09, Fady Samuel wrote:
> PTAL Dana! This is similar to skipping frames in DelegatedFrameHost today.
> Thanks!

Im confused by the part about showing frames on the old child ID when the new
parent ID frame arrives. That would just gutter immediately?

[Fady Samuel, 2017-05-01 21:42:01]

On 2017/05/01 21:33:30, danakj wrote:
> On 2017/05/01 20:35:09, Fady Samuel wrote:
> > PTAL Dana! This is similar to skipping frames in DelegatedFrameHost today.
> > Thanks!
> 
> Im confused by the part about showing frames on the old child ID when the new
> parent ID frame arrives. That would just gutter immediately?

We start accepting frames to the old child ID when the parent activates because
the activation may have been caused by a deadline. I believe DelegatedFrameHost
does something similar. 

Note a DCHECK is failing in debug builds at the moment. I'm investigating.
However, please review at a high level and I'll fix and ask for another review.

[danakj, 2017-05-01 22:15:35]

On Mon, May 1, 2017 at 5:42 PM, <fsamuel@chromium.org> wrote:

> On 2017/05/01 21:33:30, danakj wrote:
> > On 2017/05/01 20:35:09, Fady Samuel wrote:
> > > PTAL Dana! This is similar to skipping frames in DelegatedFrameHost
> today.
> > > Thanks!
> >
> > Im confused by the part about showing frames on the old child ID when
> the new
> > parent ID frame arrives. That would just gutter immediately?
>
> We start accepting frames to the old child ID when the parent activates
> because
> the activation may have been caused by a deadline. I believe
> DelegatedFrameHost
> does something similar.
>

It accepts one frame if the lock times out.
https://cs.chromium.org/chromium/src/content/browser/renderer_host/delegated_...

 When you say "may have", what else may have caused the activation?


>
> Note a DCHECK is failing in debug builds at the moment. I'm investigating.
> However, please review at a high level and I'll fix and ask for another
> review.
>
> https://codereview.chromium.org/2831213004/
>

-- 
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.

[Fady Samuel, 2017-05-01 22:28:50]

On 2017/05/01 22:15:35, danakj wrote:
> On Mon, May 1, 2017 at 5:42 PM, <mailto:fsamuel@chromium.org> wrote:
> 
> > On 2017/05/01 21:33:30, danakj wrote:
> > > On 2017/05/01 20:35:09, Fady Samuel wrote:
> > > > PTAL Dana! This is similar to skipping frames in DelegatedFrameHost
> > today.
> > > > Thanks!
> > >
> > > Im confused by the part about showing frames on the old child ID when
> > the new
> > > parent ID frame arrives. That would just gutter immediately?
> >
> > We start accepting frames to the old child ID when the parent activates
> > because
> > the activation may have been caused by a deadline. I believe
> > DelegatedFrameHost
> > does something similar.
> >
> 
> It accepts one frame if the lock times out.
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/delegated_...
> 
>  When you say "may have", what else may have caused the activation?
> 
> 
> >
> > Note a DCHECK is failing in debug builds at the moment. I'm investigating.
> > However, please review at a high level and I'll fix and ask for another
> > review.
> >
> > https://codereview.chromium.org/2831213004/
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

If dependencies have been resolved, then the CompositorFrame will activate, but
in that case, unlocking the old surface ID doesn't matter, but I didn't think it
was worth special casing things because this seems harmless?

[Fady Samuel, 2017-05-01 23:24:08]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 23:24:55]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Fady Samuel, 2017-05-01 23:32:28]

PTAL Dana! I've fixed the unit tests. The problem was most of the tests
conflated dependencies and references. A dependency is a set of surface Ids that
a CompositorFrame may block on, whereas a reference is a mechanism by which we
keep existing surfaces alive. A dependency may or may not exist yet, whereas a
reference MUST exist.

That last sentence above is what unit tests tripped over. Because they conflated
the two, they were looking for Surfaces to things that haven't been submitted
yet and either we DCHECK because the surface doesn't exist or Lock/Unlock do not
match up.

This CL updates unit tests to do the right thing.

[commit-bot: I haz the power, 2017-05-02 00:36:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-02 00:36:11]

Dry run: This issue passed the CQ dry run.

[danakj, 2017-05-02 15:09:31]

On 2017/05/01 22:28:50, Fady Samuel wrote:
> On 2017/05/01 22:15:35, danakj wrote:
> > On Mon, May 1, 2017 at 5:42 PM, <mailto:fsamuel@chromium.org> wrote:
> > 
> > > On 2017/05/01 21:33:30, danakj wrote:
> > > > On 2017/05/01 20:35:09, Fady Samuel wrote:
> > > > > PTAL Dana! This is similar to skipping frames in DelegatedFrameHost
> > > today.
> > > > > Thanks!
> > > >
> > > > Im confused by the part about showing frames on the old child ID when
> > > the new
> > > > parent ID frame arrives. That would just gutter immediately?
> > >
> > > We start accepting frames to the old child ID when the parent activates
> > > because
> > > the activation may have been caused by a deadline. I believe
> > > DelegatedFrameHost
> > > does something similar.
> > >
> > 
> > It accepts one frame if the lock times out.
> >
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/delegated_...
> > 
> >  When you say "may have", what else may have caused the activation?
> > 
> > 
> > >
> > > Note a DCHECK is failing in debug builds at the moment. I'm investigating.
> > > However, please review at a high level and I'll fix and ask for another
> > > review.
> > >
> > > https://codereview.chromium.org/2831213004/
> > >
> > 
> > -- 
> > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> If dependencies have been resolved, then the CompositorFrame will activate,
but
> in that case, unlocking the old surface ID doesn't matter, but I didn't think
it
> was worth special casing things because this seems harmless?

I see. So here's the difference:

- Animating compositor thread renderer.
- Slowwww main thread.
- Browser resizes, times out.
- Gutter, allow renderer frame

Before: allow one renderer frame, draw it, reject further compositor thread
animation frames, preventing work from being done, and preventing the tab
contents from animating while the UI is janking on the user.

After: allow multiple compositor thread frames, creating a visual effect where
the window stops sticking to your mouse but the contents are stopping for a bit,
then animating, then stopping for a bit, then animating.

After seems worse to me? Wdyt?

[Fady Samuel, 2017-05-03 00:36:18]

Description was changed from

==========
cc: Reject CompositorFrames to old child surfaces while parent blocked

Currently, if a parent is blocked on a child SurfaceID that does not yet have an
active CompositorFrame, the child may continue to submit CompositorFrames to
the old SurfaceID. This eats up CPU cycles that could be better spent letting
the
child catch up to the parent's synchronization request.

With this CL, when a parent submits a CompositorFrame that is blocked on 
unresolved dependencies, then existing referenced surfaces (which HAVE active
frames) will be "locked", preventing further CompositorFrames from being
submitted
to them. Instead, resources will be immediately returned to the client, and a
DidReceiveCompositorFrameAck will be immediately issued. This reduces the
latency
for a child client to catch up to the parent's request. Once a parent
CompositorFrame
activates then we allow submitting CompositorFrames to old surface IDs again.
This
is useful in surface synchronization if a deadline hits, and the primary surface
is not
yet available. By unlocking the old surface, we keep the child animated until it
catches
up with the parent.

BUG=672962
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Reject CompositorFrames to old child surfaces while parent blocked

Currently, if a parent is blocked on a child SurfaceID that does not 
yet have an active CompositorFrame, the child may continue to submit 
CompositorFrames to the old SurfaceID. This eats up CPU cycles that 
could be better spent letting the child catch up to the parent's 
synchronization request.

With this CL, when a parent submits a CompositorFrame that is blocked 
on unresolved dependencies, then existing referenced surfaces with 
corresponding dependencies (of the same FrameSinkId) will be "closed", 
preventing further CompositorFrames from being submitted to them. Instead,
resources will be immediately returned to the client, and a
DidReceiveCompositorFrameAck will be immediately issued. This reduces the
latency for a child client to catch up to the parent's request. 

BUG=672962
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Fady Samuel, 2017-05-03 00:36:36]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 00:37:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Fady Samuel, 2017-05-03 00:37:58]

PTAL Dana! I changed the concept from locking/unlocking 
surfaces to closing a surface once and for all! There is a prerequisite CL to
make this work right that I also sent you.

[Fady Samuel, 2017-05-03 00:38:14]

Description was changed from

==========
cc: Reject CompositorFrames to old child surfaces while parent blocked

Currently, if a parent is blocked on a child SurfaceID that does not 
yet have an active CompositorFrame, the child may continue to submit 
CompositorFrames to the old SurfaceID. This eats up CPU cycles that 
could be better spent letting the child catch up to the parent's 
synchronization request.

With this CL, when a parent submits a CompositorFrame that is blocked 
on unresolved dependencies, then existing referenced surfaces with 
corresponding dependencies (of the same FrameSinkId) will be "closed", 
preventing further CompositorFrames from being submitted to them. Instead,
resources will be immediately returned to the client, and a
DidReceiveCompositorFrameAck will be immediately issued. This reduces the
latency for a child client to catch up to the parent's request. 

BUG=672962
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Reject CompositorFrames to old child surfaces while parent blocked

Currently, if a parent is blocked on a child SurfaceID that does not 
yet have an active CompositorFrame, the child may continue to submit 
CompositorFrames to the old SurfaceID. This eats up CPU cycles that 
could be better spent letting the child catch up to the parent's 
synchronization request.

With this CL, when a parent submits a CompositorFrame that is blocked 
on unresolved dependencies, then existing referenced surfaces with 
corresponding dependencies (of the same FrameSinkId) will be "closed", 
preventing further CompositorFrames from being submitted to them. 
Instead, resources will be immediately returned to the client, and a
DidReceiveCompositorFrameAck will be immediately issued. This reduces 
the latency for a child client to catch up to the parent's request. 

BUG=672962
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[commit-bot: I haz the power, 2017-05-03 01:42:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-03 01:42:05]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2017-05-03 18:58:31]

Description was changed from

==========
cc: Reject CompositorFrames to old child surfaces while parent blocked

Currently, if a parent is blocked on a child SurfaceID that does not 
yet have an active CompositorFrame, the child may continue to submit 
CompositorFrames to the old SurfaceID. This eats up CPU cycles that 
could be better spent letting the child catch up to the parent's 
synchronization request.

With this CL, when a parent submits a CompositorFrame that is blocked 
on unresolved dependencies, then existing referenced surfaces with 
corresponding dependencies (of the same FrameSinkId) will be "closed", 
preventing further CompositorFrames from being submitted to them. 
Instead, resources will be immediately returned to the client, and a
DidReceiveCompositorFrameAck will be immediately issued. This reduces 
the latency for a child client to catch up to the parent's request. 

BUG=672962
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Reject CompositorFrames to old child surfaces while parent blocked

Currently, if a parent is blocked on a child SurfaceID that does not 
yet have an active CompositorFrame, the child may continue to submit 
CompositorFrames to the old SurfaceID. This eats up CPU cycles that 
could be better spent letting the child catch up to the parent's 
synchronization request.

With this CL, when a parent submits a CompositorFrame that is blocked 
on unresolved dependencies, then existing referenced surfaces with 
corresponding dependencies (of the same FrameSinkId) will be "closed", 
preventing further CompositorFrames from being submitted to them. 
Instead, resources will be immediately returned to the client, and a
DidReceiveCompositorFrameAck will be immediately issued. This reduces 
the latency for a child client to catch up to the parent's request. 

BUG=672962, 711948
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[danakj, 2017-05-04 21:39:13]

https://codereview.chromium.org/2831213004/diff/100001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support_unittest.cc (right):

https://codereview.chromium.org/2831213004/diff/100001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support_unittest.cc:1355: // This test
verifies CompositorFrames to a surface referenced by a parent
submitted to a surface? tbh this sentence got me all messed up, what does a
surface referenced by a parent client mean?

https://codereview.chromium.org/2831213004/diff/100001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support_unittest.cc:1392: // Resources will be
returned immediately because |child_id2|'s surface is
did u mean child_id1?

https://codereview.chromium.org/2831213004/diff/100001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support_unittest.cc:1419: // Resources will be
returned immediately because |child_id2|'s surface is
did you mean child_id1 here too?

https://codereview.chromium.org/2831213004/diff/100001/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (right):

https://codereview.chromium.org/2831213004/diff/100001/cc/surfaces/surface.cc...
cc/surfaces/surface.cc:87: // Reject CompositorFrames to surfaces referenced
from CompositorFrame while
> // Reject CompositorFrames to surfaces referenced from CompositorFrame while
> // it is blocked.

CompositorFrame is a type, you're using it in this sentence like it's an object,
and I can't tell if there is two of them or what. I suggest giving the things ur
talking about in here names and refer to them that way, like algebra.

https://codereview.chromium.org/2831213004/diff/100001/cc/surfaces/surface.cc...
cc/surfaces/surface.cc:97:
embedded_frame_sink_ids.count(surface_id.frame_sink_id()) > 0;
Didnt ur other CL just make it not possible to be in both lists?

[Fady Samuel, 2017-05-04 22:44:04]

PTAL Dana!

https://codereview.chromium.org/2831213004/diff/100001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support_unittest.cc (right):

https://codereview.chromium.org/2831213004/diff/100001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support_unittest.cc:1355: // This test
verifies CompositorFrames to a surface referenced by a parent
On 2017/05/04 21:39:13, danakj wrote:
> submitted to a surface? tbh this sentence got me all messed up, what does a
> surface referenced by a parent client mean?

Fixed comment.

https://codereview.chromium.org/2831213004/diff/100001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support_unittest.cc:1392: // Resources will be
returned immediately because |child_id2|'s surface is
On 2017/05/04 21:39:13, danakj wrote:
> did u mean child_id1?

Done.

https://codereview.chromium.org/2831213004/diff/100001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support_unittest.cc:1419: // Resources will be
returned immediately because |child_id2|'s surface is
On 2017/05/04 21:39:13, danakj wrote:
> did you mean child_id1 here too?

Done.

https://codereview.chromium.org/2831213004/diff/100001/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (right):

https://codereview.chromium.org/2831213004/diff/100001/cc/surfaces/surface.cc...
cc/surfaces/surface.cc:87: // Reject CompositorFrames to surfaces referenced
from CompositorFrame while
On 2017/05/04 21:39:13, danakj wrote:
> > // Reject CompositorFrames to surfaces referenced from CompositorFrame while
> > // it is blocked.
> 
> CompositorFrame is a type, you're using it in this sentence like it's an
object,
> and I can't tell if there is two of them or what. I suggest giving the things
ur
> talking about in here names and refer to them that way, like algebra.

I'm not sure what you mean. I do agree this is confusing, because the comment is
a bit stale. I've updated the comment. Hopefully it's clearer now.

https://codereview.chromium.org/2831213004/diff/100001/cc/surfaces/surface.cc...
cc/surfaces/surface.cc:97:
embedded_frame_sink_ids.count(surface_id.frame_sink_id()) > 0;
On 2017/05/04 21:39:13, danakj wrote:
> Didnt ur other CL just make it not possible to be in both lists?

I'm comparing FrameSinkIds not SurfaceIds.

Basically, the primary and fallback SurfaceInfo should have the same
FrameSinkId. If we find a surface ID with a FrameSinkId from
referenced_surfaces, then we know the referenced_surfaces SurfaceId is a
fallback.

[Fady Samuel, 2017-05-05 15:37:51]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 15:38:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 15:43:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-05 15:43:03]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[Fady Samuel, 2017-05-05 17:14:36]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[Fady Samuel, 2017-05-05 17:14:47]

PTAL Dana! I've rebased after a few CLs landed.

[commit-bot: I haz the power, 2017-05-05 17:15:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2017-05-05 18:06:08]

https://codereview.chromium.org/2831213004/diff/140001/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (right):

https://codereview.chromium.org/2831213004/diff/140001/cc/surfaces/surface.cc...
cc/surfaces/surface.cc:94: // If this |surface_id| corresponds to a fallback
surface referenced by
This comment just says what the code below does, which does not add anything for
me. What I'd like is a comment explaining why a surface being in both
activation_dependencies and in referenced_surfaces means it is a fallback
surface.

[commit-bot: I haz the power, 2017-05-05 18:14:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-05 18:14:44]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2017-05-05 18:55:11]

I've improved the comment! PTAL Dana!

https://codereview.chromium.org/2831213004/diff/140001/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (right):

https://codereview.chromium.org/2831213004/diff/140001/cc/surfaces/surface.cc...
cc/surfaces/surface.cc:94: // If this |surface_id| corresponds to a fallback
surface referenced by
On 2017/05/05 18:06:08, danakj wrote:
> This comment just says what the code below does, which does not add anything
for
> me. What I'd like is a comment explaining why a surface being in both
> activation_dependencies and in referenced_surfaces means it is a fallback
> surface.

I've improved the comment, hopefully.

[Fady Samuel, 2017-05-05 18:55:37]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 18:56:19]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2017-05-05 19:51:35]

LGTM

https://codereview.chromium.org/2831213004/diff/160001/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (right):

https://codereview.chromium.org/2831213004/diff/160001/cc/surfaces/surface.cc...
cc/surfaces/surface.cc:90: // Note: |activation_dependencies| and
|referenced_surfaces| are disjointed
This part of the comment seems like it belongs in CompositorFrameMetadata, which
is already there so not sure it is needed, unless you wanna buff up the comments
there.

https://codereview.chromium.org/2831213004/diff/160001/cc/surfaces/surface.cc...
cc/surfaces/surface.cc:97: //       A surface ID in |referenced_surfaces| that
has a corresponding
Thanks, this is what I was looking for. Can you move this to L106 and replace
the comment there with this? The comment there right now can go away, it only
says what the code is doing, and repeats what is said on L88.

[Fady Samuel, 2017-05-05 20:20:36]

Description was changed from

==========
cc: Reject CompositorFrames to old child surfaces while parent blocked

Currently, if a parent is blocked on a child SurfaceID that does not 
yet have an active CompositorFrame, the child may continue to submit 
CompositorFrames to the old SurfaceID. This eats up CPU cycles that 
could be better spent letting the child catch up to the parent's 
synchronization request.

With this CL, when a parent submits a CompositorFrame that is blocked 
on unresolved dependencies, then existing referenced surfaces with 
corresponding dependencies (of the same FrameSinkId) will be "closed", 
preventing further CompositorFrames from being submitted to them. 
Instead, resources will be immediately returned to the client, and a
DidReceiveCompositorFrameAck will be immediately issued. This reduces 
the latency for a child client to catch up to the parent's request. 

BUG=672962, 711948
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Reject CompositorFrames to old child surfaces

Currently, if a parent is blocked on a child SurfaceID that does not 
yet have an active CompositorFrame, the child may continue to submit 
CompositorFrames to the old SurfaceID. This eats up CPU cycles that 
could be better spent letting the child catch up to the parent's 
synchronization request.

With this CL, when a parent submits a CompositorFrame that is blocked 
on unresolved dependencies, then existing referenced surfaces with 
corresponding dependencies (of the same FrameSinkId) will be "closed", 
preventing further CompositorFrames from being submitted to them. 
Instead, resources will be immediately returned to the client, and a
DidReceiveCompositorFrameAck will be immediately issued. This reduces 
the latency for a child client to catch up to the parent's request. 

BUG=672962, 711948
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Fady Samuel, 2017-05-05 20:25:02]

CQ'ing! Thanks Dana!

https://codereview.chromium.org/2831213004/diff/160001/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (right):

https://codereview.chromium.org/2831213004/diff/160001/cc/surfaces/surface.cc...
cc/surfaces/surface.cc:90: // Note: |activation_dependencies| and
|referenced_surfaces| are disjointed
On 2017/05/05 19:51:35, danakj wrote:
> This part of the comment seems like it belongs in CompositorFrameMetadata,
which
> is already there so not sure it is needed, unless you wanna buff up the
comments
> there.

Done. Moved this to CompositorFrameMetadata.

https://codereview.chromium.org/2831213004/diff/160001/cc/surfaces/surface.cc...
cc/surfaces/surface.cc:97: //       A surface ID in |referenced_surfaces| that
has a corresponding
On 2017/05/05 19:51:35, danakj wrote:
> Thanks, this is what I was looking for. Can you move this to L106 and replace
> the comment there with this? The comment there right now can go away, it only
> says what the code is doing, and repeats what is said on L88.

Done.

[Fady Samuel, 2017-05-05 20:25:08]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2017-05-05 20:25:08]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2831213004/#ps180001
(title: "Updated comments as per danakj's suggestions")

[commit-bot: I haz the power, 2017-05-05 20:26:13]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 21:41:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-05 21:41:49]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[Fady Samuel, 2017-05-05 21:43:19]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2017-05-05 21:43:19]

The CQ bit was unchecked by fsamuel@chromium.org

[Fady Samuel, 2017-05-05 21:43:19]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2017-05-05 21:44:17]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 23:29:01]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1494020599748460,
"parent_rev": "b2d3efd2ec18ffd52c3dacfece9862366b565a3d", "commit_rev":
"980c8ee750096770976476654d13b5ecf412bee4"}

[commit-bot: I haz the power, 2017-05-05 23:30:36]

Description was changed from

==========
cc: Reject CompositorFrames to old child surfaces

Currently, if a parent is blocked on a child SurfaceID that does not 
yet have an active CompositorFrame, the child may continue to submit 
CompositorFrames to the old SurfaceID. This eats up CPU cycles that 
could be better spent letting the child catch up to the parent's 
synchronization request.

With this CL, when a parent submits a CompositorFrame that is blocked 
on unresolved dependencies, then existing referenced surfaces with 
corresponding dependencies (of the same FrameSinkId) will be "closed", 
preventing further CompositorFrames from being submitted to them. 
Instead, resources will be immediately returned to the client, and a
DidReceiveCompositorFrameAck will be immediately issued. This reduces 
the latency for a child client to catch up to the parent's request. 

BUG=672962, 711948
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Reject CompositorFrames to old child surfaces

Currently, if a parent is blocked on a child SurfaceID that does not 
yet have an active CompositorFrame, the child may continue to submit 
CompositorFrames to the old SurfaceID. This eats up CPU cycles that 
could be better spent letting the child catch up to the parent's 
synchronization request.

With this CL, when a parent submits a CompositorFrame that is blocked 
on unresolved dependencies, then existing referenced surfaces with 
corresponding dependencies (of the same FrameSinkId) will be "closed", 
preventing further CompositorFrames from being submitted to them. 
Instead, resources will be immediately returned to the client, and a
DidReceiveCompositorFrameAck will be immediately issued. This reduces 
the latency for a child client to catch up to the parent's request. 

BUG=672962, 711948
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2831213004
Cr-Commit-Position: refs/heads/master@{#469798}
Committed:
https://chromium.googlesource.com/chromium/src/+/980c8ee750096770976476654d13...
==========

[commit-bot: I haz the power, 2017-05-05 23:30:37]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/980c8ee750096770976476654d13...