compositor-worker: Force elements to grow a layer when a CompositorProxy is created.

compositor-worker: Force elements to grow a layer when a CompositorProxy is created.

Notable changes:
. A new StyleChangeReason is added as CompositorProxy.
. A new CompositingReason is added as CompositingReasonCompositorProxy.
. ComputedStyle keeps track of whether the element has a compositor proxy in
  StyleRareNonInheritedData::m_hasCompositorProxy.

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

[sadrul, 2015-04-07 16:32:58]

sadrul@chromium.org changed reviewers:
+ esprehn@chromium.org, vollick@chromium.org

[Ian Vollick, 2015-04-07 16:40:15]

This looks pretty reasonable (though I would love Elliott's opinion), but as we
chatted offline, I think we need a way to test that when the proxy goes away
that the layer disappears.

https://codereview.chromium.org/1060973003/diff/60001/Source/core/dom/Composi...
File Source/core/dom/CompositorProxy.cpp (right):

https://codereview.chromium.org/1060973003/diff/60001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.cpp:119: element.setHasCompositorProxy(true);
Where do we set this to false?

[sadrul, 2015-04-07 16:43:23]

https://codereview.chromium.org/1060973003/diff/60001/Source/core/dom/Composi...
File Source/core/dom/CompositorProxy.cpp (right):

https://codereview.chromium.org/1060973003/diff/60001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.cpp:119: element.setHasCompositorProxy(true);
On 2015/04/07 16:40:15, vollick wrote:
> Where do we set this to false?

We currently do not. The API will need to have a CompositorProxy::disconnect().
If all the CompositorProxy objects for an element (both in the main-thread and
the worker-thread) die, or call disconnect(), then we can safely set this flag
back to false. I am working on that in a separate CL.

[esprehn, 2015-04-07 17:59:49]

High level comment: you want to use ElementFlags. This is a rare feature on
a page, we shouldn't use one of the few remaining NodeFlags.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[sadrul, 2015-04-07 20:04:07]

On 2015/04/07 17:59:49, esprehn wrote:
> High level comment: you want to use ElementFlags. This is a rare feature on
> a page, we shouldn't use one of the few remaining NodeFlags.

For CompositorProxy::disconnect(), we need to keep track of the number of active
proxies for the element in ElementRareData. So we do not need either NodeFlags
or ElementFlags. So I have removed that code from the CL.

Patchset-5 in this CL is the unified patch that includes changes in
https://codereview.chromium.org/1064123002/, and patchset 6 excludes changes
related to .disconnect(). Please let me know if this makes sense.

[sadrul, 2015-04-07 22:45:24]

vollick@ Re our offline discussion: (1) looks like terminating the worker-thread
(which is equivalent to explicitly calling worker.terminate() in the
main-thread) does run the destructors for the V8 objects. So the proxy-count for
elements would still be correct if the compositor-worker is killed
unceremoniously. (2) it doesn't look like there is a way to know from the
main-thread if the worker-thread has/will-have handlers for onmessage.

[Ian Vollick, 2015-04-07 22:52:32]

On 2015/04/07 22:45:24, sadrul wrote:
> vollick@ Re our offline discussion: (1) looks like terminating the
worker-thread
> (which is equivalent to explicitly calling worker.terminate() in the
> main-thread) does run the destructors for the V8 objects. So the proxy-count
for
> elements would still be correct if the compositor-worker is killed
> unceremoniously. (2) it doesn't look like there is a way to know from the
> main-thread if the worker-thread has/will-have handlers for onmessage.

Thank you for doing that research. I'm really happy that the destructors run,
but pretty sad about the onmessage situation.

So the conclusion, then, is that it doesn't make sense to lie to the Element and
increment the count on the main thread because we can't really know if the ctor
will run?

[sadrul, 2015-04-07 22:59:13]

On 2015/04/07 22:52:32, vollick wrote:
> On 2015/04/07 22:45:24, sadrul wrote:
> > vollick@ Re our offline discussion: (1) looks like terminating the
> worker-thread
> > (which is equivalent to explicitly calling worker.terminate() in the
> > main-thread) does run the destructors for the V8 objects. So the proxy-count
> for
> > elements would still be correct if the compositor-worker is killed
> > unceremoniously. (2) it doesn't look like there is a way to know from the
> > main-thread if the worker-thread has/will-have handlers for onmessage.
> 
> Thank you for doing that research. I'm really happy that the destructors run,
> but pretty sad about the onmessage situation.
> 
> So the conclusion, then, is that it doesn't make sense to lie to the Element
and
> increment the count on the main thread because we can't really know if the
ctor
> will run?

For now, that seems to be the case, yes. We are going to have schedulers for
worker-threads in the future (http://crbug.com/463143), and at that point, it
may be possible to defer the teardown of CompositingReasonCompositorProxy layers
in an idle-task, so that if an Element has a layer, and disconnect() is called
on the last proxy, we don't immediately remove the layer in case. It's not very
nice, but it shouldn't be too horrible.

On a sidenote: what happens for web-animations currently? Is the layer removed
immediately after the animation ends?

[esprehn, 2015-04-08 00:13:10]

On 2015/04/07 at 22:59:13, sadrul wrote:
> ...
> 
> On a sidenote: what happens for web-animations currently? Is the layer removed
immediately after the animation ends?

Yes immediately. Until slimming paint we can't just add random layers, they
cause differences in painting order due to the fundamental compositing bug. You
certainly wouldn't want non-deterministic layer behavior.

[esprehn, 2015-04-08 00:17:14]

You can't have a 300ms wait in a test.

https://codereview.chromium.org/1060973003/diff/140001/LayoutTests/fast/compo...
File
LayoutTests/fast/compositorworker/compositor-proxy-disconnect-worker-terminate.html
(right):

https://codereview.chromium.org/1060973003/diff/140001/LayoutTests/fast/compo...
LayoutTests/fast/compositorworker/compositor-proxy-disconnect-worker-terminate.html:8:
testRunner.dumpAsText();
doesn't testharness take care of this?

https://codereview.chromium.org/1060973003/diff/140001/LayoutTests/fast/compo...
LayoutTests/fast/compositorworker/compositor-proxy-disconnect-worker-terminate.html:10:
var test = async_test("This test checks that an element's compositor proxy
cannot be mutated after disconnect().");
async_test("...", function() {

});

https://codereview.chromium.org/1060973003/diff/140001/LayoutTests/fast/compo...
LayoutTests/fast/compositorworker/compositor-proxy-disconnect-worker-terminate.html:28:
}, 400);
This is not acceptable, this test is going to take 400ms every time which means
all tests take half a second longer.

https://codereview.chromium.org/1060973003/diff/140001/LayoutTests/fast/compo...
File LayoutTests/fast/compositorworker/resources/proxy-idle.js (right):

https://codereview.chromium.org/1060973003/diff/140001/LayoutTests/fast/compo...
LayoutTests/fast/compositorworker/resources/proxy-idle.js:2: onmessage =
function(msg) {
event

[Ian Vollick, 2015-04-08 02:33:10]

On 2015/04/08 00:13:10, esprehn wrote:
> On 2015/04/07 at 22:59:13, sadrul wrote:
> > ...
> > 
> > On a sidenote: what happens for web-animations currently? Is the layer
removed
> immediately after the animation ends?
> 
> Yes immediately.

The animations are ticked on both the main and compositor threads. When the
animation finishes on the main thread, if the element has no other reason to be
composited, it will be demoted.

> Until slimming paint we can't just add random layers, they
> cause differences in painting order due to the fundamental compositing bug.
You
> certainly wouldn't want non-deterministic layer behavior.

Are suggesting that promoting due to having an associated compositor proxy is
adding a random layer, or are you just referring to having the composited layer
linger a few frames longer than it should?

Having an associated compositor proxy should be thought of as having an
accelerated animation -- the new samples are just provided by JS rather than
other machinery. The effect on compositing should be similar. It's also
analagous to will-change in that we declare what properties will be modified
ahead of time (and again, this has a similar effect on compositing).

[sadrul, 2015-04-08 02:35:04]

https://codereview.chromium.org/1060973003/diff/140001/LayoutTests/fast/compo...
File
LayoutTests/fast/compositorworker/compositor-proxy-disconnect-worker-terminate.html
(right):

https://codereview.chromium.org/1060973003/diff/140001/LayoutTests/fast/compo...
LayoutTests/fast/compositorworker/compositor-proxy-disconnect-worker-terminate.html:8:
testRunner.dumpAsText();
On 2015/04/08 00:17:13, esprehn wrote:
> doesn't testharness take care of this?

You are right. Done.

https://codereview.chromium.org/1060973003/diff/140001/LayoutTests/fast/compo...
LayoutTests/fast/compositorworker/compositor-proxy-disconnect-worker-terminate.html:10:
var test = async_test("This test checks that an element's compositor proxy
cannot be mutated after disconnect().");
On 2015/04/08 00:17:13, esprehn wrote:
> async_test("...", function() {
> 
> });

Done.

https://codereview.chromium.org/1060973003/diff/140001/LayoutTests/fast/compo...
LayoutTests/fast/compositorworker/compositor-proxy-disconnect-worker-terminate.html:28:
}, 400);
On 2015/04/08 00:17:13, esprehn wrote:
> This is not acceptable, this test is going to take 400ms every time which
means
> all tests take half a second longer.

Removed. I didn't know about waitUntilWorkerThreadsExit(); used that in the test
instead.

https://codereview.chromium.org/1060973003/diff/140001/LayoutTests/fast/compo...
File LayoutTests/fast/compositorworker/resources/proxy-idle.js (right):

https://codereview.chromium.org/1060973003/diff/140001/LayoutTests/fast/compo...
LayoutTests/fast/compositorworker/resources/proxy-idle.js:2: onmessage =
function(msg) {
On 2015/04/08 00:17:13, esprehn wrote:
> event

Done.

[esprehn, 2015-04-08 02:36:46]

On 2015/04/08 at 02:33:10, vollick wrote:
> On 2015/04/08 00:13:10, esprehn wrote:
> > ...
> 
> > Until slimming paint we can't just add random layers, they
> > cause differences in painting order due to the fundamental compositing bug.
You
> > certainly wouldn't want non-deterministic layer behavior.
> 
> Are suggesting that promoting due to having an associated compositor proxy is
adding a random layer, or are you just referring to having the composited layer
linger a few frames longer than it should?

The latter.

> 
> Having an associated compositor proxy should be thought of as having an
accelerated animation -- the new samples are just provided by JS rather than
other machinery. The effect on compositing should be similar. It's also
analagous to will-change in that we declare what properties will be modified
ahead of time (and again, this has a similar effect on compositing).

Right, having a proxy is like having an animation. Having a proxy cause a layer
is totally reasonable, but having the lifetime of the layer be non-deterministic
(ex. be related to GC) is not. Hopefully we can ship slimming paint and unchain
ourselves of the fundamental compositing bug. Once that happens being composited
for a longer period of time than was strictly necessary won't be an issue, but
until then we need stricter timing requirements.

[Ian Vollick, 2015-04-08 02:37:38]

On 2015/04/08 02:36:46, esprehn wrote:
> On 2015/04/08 at 02:33:10, vollick wrote:
> > On 2015/04/08 00:13:10, esprehn wrote:
> > > ...
> > 
> > > Until slimming paint we can't just add random layers, they
> > > cause differences in painting order due to the fundamental compositing
bug.
> You
> > > certainly wouldn't want non-deterministic layer behavior.
> > 
> > Are suggesting that promoting due to having an associated compositor proxy
is
> adding a random layer, or are you just referring to having the composited
layer
> linger a few frames longer than it should?
> 
> The latter.
> 
> > 
> > Having an associated compositor proxy should be thought of as having an
> accelerated animation -- the new samples are just provided by JS rather than
> other machinery. The effect on compositing should be similar. It's also
> analagous to will-change in that we declare what properties will be modified
> ahead of time (and again, this has a similar effect on compositing).
> 
> Right, having a proxy is like having an animation. Having a proxy cause a
layer
> is totally reasonable, but having the lifetime of the layer be
non-deterministic
> (ex. be related to GC) is not. Hopefully we can ship slimming paint and
unchain
> ourselves of the fundamental compositing bug. Once that happens being
composited
> for a longer period of time than was strictly necessary won't be an issue, but
> until then we need stricter timing requirements.

Ok, cool. I totally agree with you on that.

[esprehn, 2015-04-09 19:32:44]

lgtm okay

[sadrul, 2015-04-10 02:05:34]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2015-04-10 02:05:34]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1060973003/#ps220001
(title: "tot-merge")

[commit-bot: I haz the power, 2015-04-10 02:05:54]

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

[commit-bot: I haz the power, 2015-04-10 03:57:25]

Committed patchset #12 (id:220001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193506