90630 2012-07-05 13:36:08 -0700 Drawing to accelerated 2D canvas causes compositor to recompute layer tree 2012-07-06 10:47:56 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 junov junov eric jamesr kbr senorblanco simon.fraser vangelis webkit.review.bot oldest_to_newest 663707 0 junov 2012-07-05 13:36:08 -0700 Drawing to accelerated 2D canvas causes compositor to recompute layer tree 663714 1 150979 junov 2012-07-05 13:52:07 -0700 Created attachment 150979 Patch 663717 2 junov 2012-07-05 13:57:31 -0700 A key piece of code that is relevant to this patch is method RenderLayer::contentChanged (not modified by this patch). This is the method that was triggering the rebuild of the compositor tree when a 2D canvas is drawn to. 663723 3 junov 2012-07-05 14:06:57 -0700 kbr expressed concern that we must make sure that accelerated canvas layers will still trigger the compositor in all situations with this change. I have verified that this is the case: RenderLayer::addChild will trigger the compositor (via RenderLayer::dirtyZOrderLists) when a new canvas is added. 663724 4 simon.fraser 2012-07-05 14:09:02 -0700 What is the code path that causes the full layer rebuild? Is it via RenderLayer::contentChanged()? 663731 5 junov 2012-07-05 14:37:44 -0700 (In reply to comment #4) > What is the code path that causes the full layer rebuild? Is it via RenderLayer::contentChanged()? Yes. All the methods in CanvasRenderingContext2D that draw something to the canvas end up calling CanvasRenderingContext2D::didDraw, which calls RenderBoxModelObject::contentChanged(), which calls RenderLAyer::contentChanged() 663739 6 simon.fraser 2012-07-05 14:43:49 -0700 But that should only cause a layer rebuild if updateLayerCompositingState() returns true. Does it? 664124 7 junov 2012-07-06 08:28:22 -0700 (In reply to comment #6) > But that should only cause a layer rebuild if updateLayerCompositingState() returns true. Does it? updateLayerCompositingState is returning true because RenderLayerBacking::updateGraphicsLayerConfiguration() is returning true because of this bit of code: else if (isAcceleratedCanvas(renderer)) { HTMLCanvasElement* canvas = static_cast<HTMLCanvasElement*>(renderer->node()); if (CanvasRenderingContext* context = canvas->renderingContext()) m_graphicsLayer->setContentsToCanvas(context->platformLayer()); layerConfigChanged = true; } BTW, an important benefit of this patch is that will bypass the call to updateLayerCompositingState which was a hot spot that I identified in a profiling experiment. When rendering a whole bunch of small primitives in a tight loop, all the calls to updateLayerCompositingState (can be thousands per frame) are often a much heavier burden than the compositor tree rebuild. In some experiments, I've seen up to 20% of CPU time being spent there. 664129 8 simon.fraser 2012-07-06 08:52:49 -0700 (In reply to comment #7) > (In reply to comment #6) > > But that should only cause a layer rebuild if updateLayerCompositingState() returns true. Does it? > > updateLayerCompositingState is returning true because RenderLayerBacking::updateGraphicsLayerConfiguration() is returning true because of this bit of code: > > else if (isAcceleratedCanvas(renderer)) { > HTMLCanvasElement* canvas = static_cast<HTMLCanvasElement*>(renderer->node()); > if (CanvasRenderingContext* context = canvas->renderingContext()) > m_graphicsLayer->setContentsToCanvas(context->platformLayer()); > layerConfigChanged = true; > } OK, for some reason Mac doesn't hit this code. I think your original patch is OK. > BTW, an important benefit of this patch is that will bypass the call to > updateLayerCompositingState which was a hot spot that I identified in a profiling experiment. When rendering a whole bunch of small primitives in a tight loop, all the calls to updateLayerCompositingState (can be thousands per frame) are often a much heavier burden than the compositor tree rebuild. In some experiments, I've seen up to 20% of CPU time being spent there. Please file a new bug on that. 664170 9 junov 2012-07-06 10:16:50 -0700 > > Please file a new bug on that. Done. https://bugs.webkit.org/show_bug.cgi?id=90695 664175 10 150979 simon.fraser 2012-07-06 10:23:15 -0700 Comment on attachment 150979 Patch What is the code path that causes the full layer rebuild? Is it via RenderLayer::contentChanged()? 664177 11 simon.fraser 2012-07-06 10:23:34 -0700 > What is the code path that causes the full layer rebuild? Is it via RenderLayer::contentChanged()? Ignore that comment. 664196 12 150979 webkit.review.bot 2012-07-06 10:47:51 -0700 Comment on attachment 150979 Patch Clearing flags on attachment: 150979 Committed r121987: <http://trac.webkit.org/changeset/121987> 664197 13 webkit.review.bot 2012-07-06 10:47:56 -0700 All reviewed patches have been landed. Closing bug. 150979 2012-07-05 13:52:07 -0700 2012-07-06 10:47:51 -0700 Patch bug-90630-20120705165205.patch text/plain 2979 junov SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEyMTkyMykKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIzIEBACisyMDEyLTA3LTA1ICBKdXN0aW4g Tm92b3NhZCAgPGp1bm92QGNocm9taXVtLm9yZz4KKworICAgICAgICBEcmF3aW5nIHRvIGFjY2Vs ZXJhdGVkIDJEIGNhbnZhcyBjYXVzZXMgY29tcG9zaXRvciB0byByZWNvbXB1dGUgbGF5ZXIgdHJl ZQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9OTA2MzAK KworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBObyBuZXcg dGVzdHM6IGNvdmVyZWQgYnkgZXhpc3RpbmcgY2FudmFzL2NvbXBvc2l0aW5nIGxheW91dCB0ZXN0 cworCisgICAgICAgIEJlZm9yZSB0aGlzIGNoYW5nZSwgdGhlcmUgd2FzIG5vIGRpc3RpbmN0aW9u IGJldHdlZW4gY2FudmFzIGNoYW5nZXMKKyAgICAgICAgdGhhdCByZXF1aXJlIHJlY29tcHV0aW5n IHRoZSBjb21wb3NpdG9yIHRyZWUgYW5kIGNhbnZhcyBjaGFuZ2VzIHRoYXQKKyAgICAgICAgb25s eSByZXF1aXJlIHJlLWRpc3BsYXkuIFRoZSBuZXcgQ2FudmFzUGl4ZWxzQ2hhbmdlZCBtZW1iZXIg b2YgZW51bQorICAgICAgICBDb250ZW50Q2hhbmdlVHlwZSByZXF1ZXN0cyByZS1kaXNwbGF5IHdp dGhvdXQgcmUtbGF5b3V0LgorCisgICAgICAgICogaHRtbC9jYW52YXMvQ2FudmFzUmVuZGVyaW5n Q29udGV4dDJELmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkNhbnZhc1JlbmRlcmluZ0NvbnRleHQy RDo6ZGlkRHJhdyk6CisgICAgICAgICogcmVuZGVyaW5nL1JlbmRlckJveE1vZGVsT2JqZWN0Lmg6 CisgICAgICAgICogcmVuZGVyaW5nL1JlbmRlckxheWVyQmFja2luZy5jcHA6CisgICAgICAgIChX ZWJDb3JlOjpSZW5kZXJMYXllckJhY2tpbmc6OmNvbnRlbnRDaGFuZ2VkKToKKwogMjAxMi0wNy0w NSAgWW9uZ2p1biBaaGFuZyAgPHlvbmdqdW5femhhbmdAYXBwbGUuY29tPgogCiAgICAgICAgIENv ZGUgcmVmYWN0b3Jpbmc6IG1vdmUgbWltZVR5cGVGcm9tVVRJVHJlZSB0byBhIHNlcGFyYXRlIGZp bGUuCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9odG1sL2NhbnZhcy9DYW52YXNSZW5kZXJpbmdDb250 ZXh0MkQuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL2h0bWwvY2FudmFzL0NhbnZh c1JlbmRlcmluZ0NvbnRleHQyRC5jcHAJKHJldmlzaW9uIDEyMTkyMikKKysrIFNvdXJjZS9XZWJD b3JlL2h0bWwvY2FudmFzL0NhbnZhc1JlbmRlcmluZ0NvbnRleHQyRC5jcHAJKHdvcmtpbmcgY29w eSkKQEAgLTE3NjgsNyArMTc2OCw3IEBAIHZvaWQgQ2FudmFzUmVuZGVyaW5nQ29udGV4dDJEOjpk aWREcmF3KGMKICAgICBpZiAoaXNBY2NlbGVyYXRlZCgpKSB7CiAgICAgICAgIFJlbmRlckJveCog cmVuZGVyQm94ID0gY2FudmFzKCktPnJlbmRlckJveCgpOwogICAgICAgICBpZiAocmVuZGVyQm94 ICYmIHJlbmRlckJveC0+aGFzQWNjZWxlcmF0ZWRDb21wb3NpdGluZygpKSB7Ci0gICAgICAgICAg ICByZW5kZXJCb3gtPmNvbnRlbnRDaGFuZ2VkKENhbnZhc0NoYW5nZWQpOworICAgICAgICAgICAg cmVuZGVyQm94LT5jb250ZW50Q2hhbmdlZChDYW52YXNQaXhlbHNDaGFuZ2VkKTsKICAgICAgICAg ICAgIGNhbnZhcygpLT5jbGVhckNvcGllZEltYWdlKCk7CiAgICAgICAgICAgICByZXR1cm47CiAg ICAgICAgIH0KSW5kZXg6IFNvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJCb3hNb2RlbE9i amVjdC5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJCb3hN b2RlbE9iamVjdC5oCShyZXZpc2lvbiAxMjE5MjIpCisrKyBTb3VyY2UvV2ViQ29yZS9yZW5kZXJp bmcvUmVuZGVyQm94TW9kZWxPYmplY3QuaAkod29ya2luZyBjb3B5KQpAQCAtNDQsNiArNDQsNyBA QCBlbnVtIENvbnRlbnRDaGFuZ2VUeXBlIHsKICAgICBJbWFnZUNoYW5nZWQsCiAgICAgTWFza0lt YWdlQ2hhbmdlZCwKICAgICBDYW52YXNDaGFuZ2VkLAorICAgIENhbnZhc1BpeGVsc0NoYW5nZWQs CiAgICAgVmlkZW9DaGFuZ2VkLAogICAgIEZ1bGxTY3JlZW5DaGFuZ2VkCiB9OwpJbmRleDogU291 cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlckxheWVyQmFja2luZy5jcHAKPT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQot LS0gU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlckxheWVyQmFja2luZy5jcHAJKHJldmlz aW9uIDEyMTkyMikKKysrIFNvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJMYXllckJhY2tp bmcuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0xMDMzLDcgKzEwMzMsNyBAQCB2b2lkIFJlbmRlckxh eWVyQmFja2luZzo6Y29udGVudENoYW5nZWQoCiAgICAgfQogCiAjaWYgRU5BQkxFKFdFQkdMKSB8 fCBFTkFCTEUoQUNDRUxFUkFURURfMkRfQ0FOVkFTKQotICAgIGlmICgoY2hhbmdlVHlwZSA9PSBD YW52YXNDaGFuZ2VkKSAmJiBpc0FjY2VsZXJhdGVkQ2FudmFzKHJlbmRlcmVyKCkpKSB7CisgICAg aWYgKChjaGFuZ2VUeXBlID09IENhbnZhc0NoYW5nZWQgfHwgY2hhbmdlVHlwZSA9PSBDYW52YXNQ aXhlbHNDaGFuZ2VkKSAmJiBpc0FjY2VsZXJhdGVkQ2FudmFzKHJlbmRlcmVyKCkpKSB7CiAgICAg ICAgIG1fZ3JhcGhpY3NMYXllci0+c2V0Q29udGVudHNOZWVkc0Rpc3BsYXkoKTsKICAgICAgICAg cmV0dXJuOwogICAgIH0K