45845 2010-09-15 15:55:40 -0700 [chromium] WebViewImpl should not destroy accelerated compositor when compositing is not needed 2010-09-28 16:35:03 -0700 1 1 1 Unclassified WebKit WebKit Misc. 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 vangelis vangelis jamesr kbr oldest_to_newest 279788 0 vangelis 2010-09-15 15:55:40 -0700 Currently the LayerRendererChromium created for the WebViewImpl gets destroyed when compositing is no longer needed for the page. Pages that turn compositing on and off regularly (maps.google.com is one of them when you zoom the map) suffer as every time the compositor is initialized it has to create its shader programs, VBOs etc. There's really little overhead keeping the compositor around even if it doesn't get used. 279840 1 67744 vangelis 2010-09-15 17:15:53 -0700 Created attachment 67744 proposed patch 279855 2 67744 jamesr 2010-09-15 17:49:12 -0700 Comment on attachment 67744 proposed patch Looks like a great optimization but I think the logic is off. View in context: https://bugs.webkit.org/attachment.cgi?id=67744&action=prettypatch > WebKit/chromium/src/WebViewImpl.cpp:317 > + m_layerRenderer.clear(); You don't have to do this. m_layerRenderer is an OwnPtr() and will delete what it points to when it is destroyed. > WebKit/chromium/src/WebViewImpl.cpp:2313 > m_layerRenderer = LayerRendererChromium::create(getOnscreenGLES2Context()); > if (m_layerRenderer) { > m_isAcceleratedCompositingActive = true; > + m_compositorCreationFailed = false; I don't quite understand - if someone calls setIsAcceleratedCompositing(false) then setIsAcceleratedCompositing(true), then on the second call the assignment to m_layerRenderer will still destroy the old LayerRendererChromium and then create a new one. Did you mean to null-check m_layerRenderer before creating a new one? 279891 3 vangelis 2010-09-15 18:46:39 -0700 (In reply to comment #2) > (From update of attachment 67744 [details]) > Looks like a great optimization but I think the logic is off. You are right!! > > View in context: https://bugs.webkit.org/attachment.cgi?id=67744&action=prettypatch > > > WebKit/chromium/src/WebViewImpl.cpp:317 > > + m_layerRenderer.clear(); > You don't have to do this. m_layerRenderer is an OwnPtr() and will delete what it points to when it is destroyed. The problem I was trying to solve here is that the layerRenderer needs to be destroyed before the gles2Context which is also owned by the WebView as it needs to clean up after itself. We could change the ownership of the context but it's not worth doing it now as kbr's upcoming change removed the GLES2Context class altogether. > > > WebKit/chromium/src/WebViewImpl.cpp:2313 > > m_layerRenderer = LayerRendererChromium::create(getOnscreenGLES2Context()); > > if (m_layerRenderer) { > > m_isAcceleratedCompositingActive = true; > > + m_compositorCreationFailed = false; > I don't quite understand - if someone calls setIsAcceleratedCompositing(false) then setIsAcceleratedCompositing(true), then on the second call the assignment to m_layerRenderer will still destroy the old LayerRendererChromium and then create a new one. Did you mean to null-check m_layerRenderer before creating a new one? Yikes, thanks for catching this one... You are right, a test for null is required. I'll hold off on this CL until kbr checks in his changes. 286398 4 69075 vangelis 2010-09-28 11:28:57 -0700 Created attachment 69075 patch fixing previous issues 286427 5 69075 kbr 2010-09-28 12:06:15 -0700 Comment on attachment 69075 patch fixing previous issues Basically looks fine. WebKit style prefers early returns, so it might be better to restructure things to peel off cases one at a time and return. For example, if (!active) { m_isAcceleratedCompositingActive = false; return; } if (m_layerRenderer) { m_isAcceleratedCompositingActive = true; return; } and so on. 286619 6 vangelis 2010-09-28 16:35:03 -0700 Committed r68606: <http://trac.webkit.org/changeset/68606> 67744 2010-09-15 17:15:53 -0700 2010-09-28 11:28:57 -0700 proposed patch keepRenderer_45845.txt text/plain 1965 vangelis SW5kZXg6IFdlYktpdC9jaHJvbWl1bS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViS2l0L2No cm9taXVtL0NoYW5nZUxvZwkocmV2aXNpb24gNjc1NzQpCisrKyBXZWJLaXQvY2hyb21pdW0vQ2hh bmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTkgQEAKKzIwMTAtMDktMTUgIFZhbmdl bGlzIEtva2tldmlzICA8dmFuZ2VsaXNAY2hyb21pdW0ub3JnPgorCisgICAgICAgIFJldmlld2Vk IGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFtjaHJvbWl1bV0gS2VlcCB0aGUgYWNjZWxl cmF0ZWQgY29tcG9zaXRvciAoTGF5ZXJSZW5kZXJlckNocm9taXVtKQorICAgICAgICBhcm91bmQg ZXZlbiBhZnRlciB0aGUgV2ViVmlldyBzd2l0Y2hlcyBiYWNrIHRvIHNvZnR3YXJlIGNvbXBvc2l0 aW5nLgorICAgICAgICBUaGlzIG1ha2VzIHN1YnNlcXVlbnQgaW52b2NhdGlvbnMgb2YgdGhlIGNv bXBvc2l0b3Igb24gdGhlIHNhbWUgV2ViVmlldworICAgICAgICBmYXN0ZXIgd2hpY2ggaXMgaW1w b3J0YW50IGZvciBwYWdlcyB0aGF0IGZyZXF1ZW50bHkgc3dpdGNoIGJldHdlZW4gc29mdHdhcmUK KyAgICAgICAgYW5kIGhhcmR3YXJlIGNvbXBvc2l0aW5nIChlLmcuIHBhZ2VzIHRoYXQgdHJpZ2dl ciB0aGUgY29tcG9zaXRvciB2aWEgQ1NTIGFuaW1hdGlvbnMpLgorICAgICAgICBodHRwczovL2J1 Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NDU4NDUKKyAgICAgICAgCisKKyAgICAgICAg KiBzcmMvV2ViVmlld0ltcGwuY3BwOgorICAgICAgICAoV2ViS2l0OjpXZWJWaWV3SW1wbDo6fldl YlZpZXdJbXBsKToKKyAgICAgICAgKFdlYktpdDo6V2ViVmlld0ltcGw6OnNldElzQWNjZWxlcmF0 ZWRDb21wb3NpdGluZ0FjdGl2ZSk6CisKIDIwMTAtMDktMTQgIFBhdmVsIEZlbGRtYW4gIDxwZmVs ZG1hbkBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgWXVyeSBTZW1pa2hhdHNr eS4KSW5kZXg6IFdlYktpdC9jaHJvbWl1bS9zcmMvV2ViVmlld0ltcGwuY3BwCj09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0K LS0tIFdlYktpdC9jaHJvbWl1bS9zcmMvV2ViVmlld0ltcGwuY3BwCShyZXZpc2lvbiA2NzU2NykK KysrIFdlYktpdC9jaHJvbWl1bS9zcmMvV2ViVmlld0ltcGwuY3BwCSh3b3JraW5nIGNvcHkpCkBA IC0zMTQsNiArMzE0LDcgQEAgV2ViVmlld0ltcGw6OldlYlZpZXdJbXBsKFdlYlZpZXdDbGllbnQq IAogV2ViVmlld0ltcGw6On5XZWJWaWV3SW1wbCgpCiB7CiAgICAgQVNTRVJUKCFtX3BhZ2UpOwor ICAgIG1fbGF5ZXJSZW5kZXJlci5jbGVhcigpOwogfQogCiBSZW5kZXJUaGVtZSogV2ViVmlld0lt cGw6OnRoZW1lKCkgY29uc3QKQEAgLTIzMDksMTIgKzIzMTAsMTIgQEAgdm9pZCBXZWJWaWV3SW1w bDo6c2V0SXNBY2NlbGVyYXRlZENvbXBvcwogICAgICAgICBtX2xheWVyUmVuZGVyZXIgPSBMYXll clJlbmRlcmVyQ2hyb21pdW06OmNyZWF0ZShnZXRPbnNjcmVlbkdMRVMyQ29udGV4dCgpKTsKICAg ICAgICAgaWYgKG1fbGF5ZXJSZW5kZXJlcikgewogICAgICAgICAgICAgbV9pc0FjY2VsZXJhdGVk Q29tcG9zaXRpbmdBY3RpdmUgPSB0cnVlOworICAgICAgICAgICAgbV9jb21wb3NpdG9yQ3JlYXRp b25GYWlsZWQgPSBmYWxzZTsKICAgICAgICAgfSBlbHNlIHsKICAgICAgICAgICAgIG1faXNBY2Nl bGVyYXRlZENvbXBvc2l0aW5nQWN0aXZlID0gZmFsc2U7CiAgICAgICAgICAgICBtX2NvbXBvc2l0 b3JDcmVhdGlvbkZhaWxlZCA9IHRydWU7CiAgICAgICAgIH0KICAgICB9IGVsc2UgewotICAgICAg ICBtX2xheWVyUmVuZGVyZXIgPSAwOwogICAgICAgICBtX2lzQWNjZWxlcmF0ZWRDb21wb3NpdGlu Z0FjdGl2ZSA9IGZhbHNlOwogICAgIH0KIH0K 69075 2010-09-28 11:28:57 -0700 2010-09-28 12:06:15 -0700 patch fixing previous issues keepCompositor_45845.txt text/plain 2866 vangelis SW5kZXg6IFdlYktpdC9jaHJvbWl1bS9DaGFuZ2VMb2cNCj09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0NCi0tLSBXZWJLaXQv Y2hyb21pdW0vQ2hhbmdlTG9nCShyZXZpc2lvbiA2ODU0NSkKKysrIFdlYktpdC9jaHJvbWl1bS9D aGFuZ2VMb2cJKHdvcmtpbmcgY29weSkKQEAgLTEsMyArMSwxNSBAQAorMjAxMC0wOS0yOCAgVmFu Z2VsaXMgS29ra2V2aXMgIDx2YW5nZWxpc0BjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgW2Nocm9taXVtXSBLZWVwIHRoZSBhY2Nl bGVyYXRlZCBjb21wb3NpdG9yIGFyb3VuZCBldmVuIGFmdGVyIGEgcGFnZSBpcworICAgICAgICBk b25lIHVzaW5nIGl0IHRvIGF2b2lkIHN0YXJ0dXAgY29zdHMgaW4gcGFnZXMgdGhhdCBmcmVxdWVu dGx5IHN3aXRjaAorICAgICAgICB0aGUgY29tcG9zaXRvciBvbiBhbmQgb2ZmLgorICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NDU4NDUKKworICAgICAgICAq IHNyYy9XZWJWaWV3SW1wbC5jcHA6CisgICAgICAgIChXZWJLaXQ6OldlYlZpZXdJbXBsOjpzZXRJ c0FjY2VsZXJhdGVkQ29tcG9zaXRpbmdBY3RpdmUpOgorCiAyMDEwLTA5LTI4ICBBbmRyZWkgUG9w ZXNjdSAgPGFuZHJlaXBAZ29vZ2xlLmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBieSBKZXJlbXkg T3Jsb3cuCkluZGV4OiBXZWJLaXQvY2hyb21pdW0vc3JjL1dlYlZpZXdJbXBsLmNwcA0KPT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PQ0KLS0tIFdlYktpdC9jaHJvbWl1bS9zcmMvV2ViVmlld0ltcGwuY3BwCShyZXZpc2lvbiA2 ODQ0NSkKKysrIFdlYktpdC9jaHJvbWl1bS9zcmMvV2ViVmlld0ltcGwuY3BwCSh3b3JraW5nIGNv cHkpCkBAIC0yMjk3LDI1ICsyMjk3LDI1IEBAIHZvaWQgV2ViVmlld0ltcGw6OnNldElzQWNjZWxl cmF0ZWRDb21wb3MKICAgICAgICAgcmV0dXJuOwogCiAgICAgaWYgKGFjdGl2ZSkgewotICAgICAg ICBPd25QdHI8R3JhcGhpY3NDb250ZXh0M0Q+IGNvbnRleHQgPSBtX3RlbXBvcmFyeU9uc2NyZWVu R3JhcGhpY3NDb250ZXh0M0QucmVsZWFzZSgpOwotICAgICAgICBpZiAoIWNvbnRleHQpIHsKLSAg ICAgICAgICAgIGNvbnRleHQgPSBHcmFwaGljc0NvbnRleHQzRDo6Y3JlYXRlKEdyYXBoaWNzQ29u dGV4dDNEOjpBdHRyaWJ1dGVzKCksIG1fcGFnZS0+Y2hyb21lKCksIEdyYXBoaWNzQ29udGV4dDNE OjpSZW5kZXJEaXJlY3RseVRvSG9zdFdpbmRvdyk7Ci0gICAgICAgICAgICBpZiAoY29udGV4dCkK LSAgICAgICAgICAgICAgICBjb250ZXh0LT5yZXNoYXBlKHN0ZDo6bWF4KDEsIG1fc2l6ZS53aWR0 aCksIHN0ZDo6bWF4KDEsIG1fc2l6ZS5oZWlnaHQpKTsKLSAgICAgICAgfQotICAgICAgICBtX2xh eWVyUmVuZGVyZXIgPSBMYXllclJlbmRlcmVyQ2hyb21pdW06OmNyZWF0ZShjb250ZXh0LnJlbGVh c2UoKSk7Ci0gICAgICAgIGlmIChtX2xheWVyUmVuZGVyZXIpIHsKKyAgICAgICAgaWYgKCFtX2xh eWVyUmVuZGVyZXIpIHsKKyAgICAgICAgICAgIE93blB0cjxHcmFwaGljc0NvbnRleHQzRD4gY29u dGV4dCA9IG1fdGVtcG9yYXJ5T25zY3JlZW5HcmFwaGljc0NvbnRleHQzRC5yZWxlYXNlKCk7Cisg ICAgICAgICAgICBpZiAoIWNvbnRleHQpIHsKKyAgICAgICAgICAgICAgICBjb250ZXh0ID0gR3Jh cGhpY3NDb250ZXh0M0Q6OmNyZWF0ZShHcmFwaGljc0NvbnRleHQzRDo6QXR0cmlidXRlcygpLCBt X3BhZ2UtPmNocm9tZSgpLCBHcmFwaGljc0NvbnRleHQzRDo6UmVuZGVyRGlyZWN0bHlUb0hvc3RX aW5kb3cpOworICAgICAgICAgICAgICAgIGlmIChjb250ZXh0KQorICAgICAgICAgICAgICAgICAg ICBjb250ZXh0LT5yZXNoYXBlKHN0ZDo6bWF4KDEsIG1fc2l6ZS53aWR0aCksIHN0ZDo6bWF4KDEs IG1fc2l6ZS5oZWlnaHQpKTsKKyAgICAgICAgICAgIH0KKyAgICAgICAgICAgIG1fbGF5ZXJSZW5k ZXJlciA9IExheWVyUmVuZGVyZXJDaHJvbWl1bTo6Y3JlYXRlKGNvbnRleHQucmVsZWFzZSgpKTsK KyAgICAgICAgICAgIGlmIChtX2xheWVyUmVuZGVyZXIpIHsKKyAgICAgICAgICAgICAgICBtX2lz QWNjZWxlcmF0ZWRDb21wb3NpdGluZ0FjdGl2ZSA9IHRydWU7CisgICAgICAgICAgICAgICAgbV9j b21wb3NpdG9yQ3JlYXRpb25GYWlsZWQgPSBmYWxzZTsKKyAgICAgICAgICAgIH0gZWxzZSB7Cisg ICAgICAgICAgICAgICAgbV9pc0FjY2VsZXJhdGVkQ29tcG9zaXRpbmdBY3RpdmUgPSBmYWxzZTsK KyAgICAgICAgICAgICAgICBtX2NvbXBvc2l0b3JDcmVhdGlvbkZhaWxlZCA9IHRydWU7CisgICAg ICAgICAgICB9CisgICAgICAgIH0gZWxzZQogICAgICAgICAgICAgbV9pc0FjY2VsZXJhdGVkQ29t cG9zaXRpbmdBY3RpdmUgPSB0cnVlOwotICAgICAgICB9IGVsc2UgewotICAgICAgICAgICAgbV9p c0FjY2VsZXJhdGVkQ29tcG9zaXRpbmdBY3RpdmUgPSBmYWxzZTsKLSAgICAgICAgICAgIG1fY29t cG9zaXRvckNyZWF0aW9uRmFpbGVkID0gdHJ1ZTsKLSAgICAgICAgfQotICAgIH0gZWxzZSB7Ci0g ICAgICAgIGlmIChtX2xheWVyUmVuZGVyZXIpCi0gICAgICAgICAgICBtX2xheWVyUmVuZGVyZXIt PnNldFJvb3RMYXllcigwKTsKLSAgICAgICAgbV9sYXllclJlbmRlcmVyID0gMDsKKyAgICB9IGVs c2UKICAgICAgICAgbV9pc0FjY2VsZXJhdGVkQ29tcG9zaXRpbmdBY3RpdmUgPSBmYWxzZTsKLSAg ICB9CiB9CiAKIHZvaWQgV2ViVmlld0ltcGw6OnVwZGF0ZVJvb3RMYXllckNvbnRlbnRzKGNvbnN0 IEludFJlY3QmIHJlY3QpCg==