75637 2012-01-05 11:25:12 -0800 [Chromium] Protect visible textures before updating them 2012-02-09 11:26:30 -0800 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) Unspecified Unspecified RESOLVED INVALID P2 Normal --- 72078 1 wangxianzhu wangxianzhu cc-bugs danakj enne jamesr klobag nduca reveman vangelis webkit.review.bot oldest_to_newest 530467 0 wangxianzhu 2012-01-05 11:25:12 -0800 Currently, tile textures are reserved (requested for new ones or protected for existing ones) in TiledLayerChromium::prepareToUpdateTiles(). If a new tile causes the memory to exceed the preferred memory limit, a least used texture will be replaced. However, sometimes the texture being replaced will be used soon. (In fact, at the time, the least used texture is more likely to be used soon.) For example, a layer has the following tiles to be drawn in the current cycle: 1 2 3 4 5 6 7 8 Tiles 1 to 4 don't have textures, and 5 to 8 have. And the layer also have tiles 9 to 12 which have textures but are no longer visible. Assume the preferred memory limit can contain 6 textures. TextureManager will do the following for the tiles: 1: replaceTexture(5 => 1) 2: replaceTexture(6 => 2) 3: replaceTexture(7 => 3) 4: replaceTexture(8 => 4) 5: replaceTexture(9 => 5) 6: replaceTexture(10 => 6) 7: replaceTexture(11 => 7) 8: replaceTexture(12 => 8) The textures of tiles 5 to 8 are replaced and then recreated and repainted. If we protect all the visible tiles (5 to 8) of all layers before CCLayerTreeHost::paintLayerContents(), TextureManager will do the following: 1: replaceTexture(9 => 1) 2: replaceTexture(10 => 2) 3: replaceTexture(11 => 3) 4: replaceTexture(12 => 4) 5: do nothing 6: do nothing 7: do nothing 8: do nothing 530749 1 klobag 2012-01-05 16:14:41 -0800 Currently replaceTexture is based on m_preferredMemoryLimitBytes. If we use the max(m_preferredMemoryLimitBytes, currentlyProtectedByes) and currentlyProtectedByes is set after each call to reduceMemoryToLimit(), will this help? 531230 2 wangxianzhu 2012-01-06 12:01:59 -0800 (In reply to comment #1) > Currently replaceTexture is based on m_preferredMemoryLimitBytes. If we use the max(m_preferredMemoryLimitBytes, currentlyProtectedByes) and currentlyProtectedByes is set after each call to reduceMemoryToLimit(), will this help? If I understood your method correct, using my example to simulate shows it doesn't work well: as used = 8 textures and max(preferred, protected) = 8 textures, any request of new texture will still trigger replaceTexture, so what happens is just the same as the current code. If we further raise the threshold of replaceTexture, remove/add will happen which costs even more than replaceTexture. I prefer doing explicit protection before updating the layers. With it, the textures that are no longer visible are not protected and will be replaced or removed when needed, and visible textures won't be replaced. 531366 3 jamesr 2012-01-06 13:52:53 -0800 (In reply to comment #2) > (In reply to comment #1) > > Currently replaceTexture is based on m_preferredMemoryLimitBytes. If we use the max(m_preferredMemoryLimitBytes, currentlyProtectedByes) and currentlyProtectedByes is set after each call to reduceMemoryToLimit(), will this help? > > If I understood your method correct, using my example to simulate shows it doesn't work well: as used = 8 textures and max(preferred, protected) = 8 textures, any request of new texture will still trigger replaceTexture, so what happens is just the same as the current code. If we further raise the threshold of replaceTexture, remove/add will happen which costs even more than replaceTexture. > > I prefer doing explicit protection before updating the layers. With it, the textures that are no longer visible are not protected and will be replaced or removed when needed, and visible textures won't be replaced. I think you're right. The intent with the preferred/max limits is that currently visible textures should always get priority and kick out anything below the preferred limit if there's contention, and that we should only keep non-visible textures around if the total use is under the preferred limit. 552641 4 126192 wangxianzhu 2012-02-08 16:55:25 -0800 Created attachment 126192 patch 552673 5 126192 danakj 2012-02-08 17:28:19 -0800 Comment on attachment 126192 patch View in context: https://bugs.webkit.org/attachment.cgi?id=126192&action=review > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:460 > + if (it.representsTargetRenderSurface() || it->alwaysReserveTextures()) I think you want !it.representsItself() Right now you protect both in the representsContributingSurface() and representsItself() case. A layer that owns a surface and draws will be protected twice. You care only to catch layers that draw something IIUC so you just want representsItself(). 552684 6 126192 wangxianzhu 2012-02-08 17:39:10 -0800 Comment on attachment 126192 patch View in context: https://bugs.webkit.org/attachment.cgi?id=126192&action=review >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:460 >> + if (it.representsTargetRenderSurface() || it->alwaysReserveTextures()) > > I think you want !it.representsItself() > > Right now you protect both in the representsContributingSurface() and representsItself() case. A layer that owns a surface and draws will be protected twice. You care only to catch layers that draw something IIUC so you just want representsItself(). Does the code in CCLayerTreeHost::reserveTextures() (where I copied some code :) have the same problem? 552694 7 danakj 2012-02-08 17:47:05 -0800 If I am understanding correctly, yes it does. @enne does a contributing layer need to reserve textures if it doesn't draw? 552733 8 126192 vangelis 2012-02-08 18:29:58 -0800 Comment on attachment 126192 patch View in context: https://bugs.webkit.org/attachment.cgi?id=126192&action=review The texture recycling patch was reverted yesterday: http://trac.webkit.org/changeset/107014 as it was causing a lot of unnecessary repaints. Your patch will need to first re-enable texture recycling. But before we do that we should make sure that things work as expected and we don't end up churning through textures again. >>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:460 >>> + if (it.representsTargetRenderSurface() || it->alwaysReserveTextures()) >> >> I think you want !it.representsItself() >> >> Right now you protect both in the representsContributingSurface() and representsItself() case. A layer that owns a surface and draws will be protected twice. You care only to catch layers that draw something IIUC so you just want representsItself(). > > Does the code in CCLayerTreeHost::reserveTextures() (where I copied some code :) have the same problem? I don't think it will be protected twice as regardless of whether the layer has a RS attached to it or not, it should only appear once in the list. 552746 9 danakj 2012-02-08 18:53:47 -0800 (In reply to comment #8) > I don't think it will be protected twice as regardless of whether the layer has a RS attached to it or not, it should only appear once in the list. If a layer draws and owns a surface also, it will be in the list for the RenderSurface it owns, and in the list for the RenderSurface it contributes to. The iterator class will visit the layer in both places, with the "representsX" flag to differentiate (also a third time for visiting the the layer's render surface as a target but that's already skipped). 552865 10 vangelis 2012-02-08 22:16:49 -0800 (In reply to comment #9) > (In reply to comment #8) > > I don't think it will be protected twice as regardless of whether the layer has a RS attached to it or not, it should only appear once in the list. > > If a layer draws and owns a surface also, it will be in the list for the RenderSurface it owns, and in the list for the RenderSurface it contributes to. The iterator class will visit the layer in both places, with the "representsX" flag to differentiate (also a third time for visiting the the layer's render surface as a target but that's already skipped). Hmm, the way the code is setup, if a layer owns a RenderSurface, it also contributes to it. See: http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp&exact_package=chromium&q=layertreehostcommon&type=cs&l=320 So it will only be visited twice, once in the list of the RS it owns and once in the list of its ancestor's RS. If we bail out for it.representsTargetRenderSurface() we should only be protecting it exactly once. 553192 11 danakj 2012-02-09 08:35:46 -0800 (In reply to comment #10) > Hmm, the way the code is setup, if a layer owns a RenderSurface, it also contributes to it. See: > > http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp&exact_package=chromium&q=layertreehostcommon&type=cs&l=320 > > So it will only be visited twice, once in the list of the RS it owns and once in the list of its ancestor's RS. If we bail out for it.representsTargetRenderSurface() we should only be protecting it exactly once. Sorry this is derailing things a bit Xianzhu, I guess it's worth getting things right. Vangelis, you're absolutely right in your understanding of the renderSurfaceLayerList structure. What's missing is how the iterators run over this structure. http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebKit/chromium/tests/CCLayerIteratorTest.cpp&type=cs&l=259 This layer is hit once in each of the three states during a back-to-front iteration. The iterators do this so that you can easily act differently on targets, contributing surfaces, and drawing surfaces. Most uses of the iterators only care to act on layers when representsItself(), while maybe doing something wrt the surfaces when representsContributingRenderSurface() or representsTargetRenderSurface(). 553276 12 wangxianzhu 2012-02-09 10:21:57 -0800 (In reply to comment #11) > Sorry this is derailing things a bit Xianzhu, I guess it's worth getting things right. Vangelis, you're absolutely right in your understanding of the renderSurfaceLayerList structure. What's missing is how the iterators run over this structure. > No problem. In deed I learned from the discussion :) > The texture recycling patch was reverted yesterday: > http://trac.webkit.org/changeset/107014 > > as it was causing a lot of unnecessary repaints. Your patch will need to first re-enable texture recycling. But before we do that we should make sure that things work as expected and we don't end up churning through textures again. > I noticed the revert but I thought the problem remained with only replace/recreate changed to remove/add. Now I've realized that they are different at the memory limits, so this bug no longer exists. Will file another bug about TiledLayerChromium::protectVisibleTileTextures(). 553355 13 enne 2012-02-09 11:26:30 -0800 (In reply to comment #5) > (From update of attachment 126192 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=126192&action=review > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:460 > > + if (it.representsTargetRenderSurface() || it->alwaysReserveTextures()) > > I think you want !it.representsItself() You're quite right. Here's https://bugs.webkit.org/show_bug.cgi?id=78258 to fix the reserveTextures part of that. 126192 2012-02-08 16:55:25 -0800 2012-02-09 10:22:14 -0800 patch 75637 text/plain 3662 wangxianzhu SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEwNzEzMikKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIxIEBACisyMDEyLTAyLTA4ICBYaWFuemh1 IFdhbmcgIDx3YW5neGlhbnpodUBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgW0Nocm9taXVtXSBQ cm90ZWN0IHZpc2libGUgdGV4dHVyZXMgYmVmb3JlIHVwZGF0aW5nIHRoZW0KKyAgICAgICAgaHR0 cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTc1NjM3CisKKyAgICAgICAgVGhp cyBjYW4gcHJldmVudCBzb21lIHRleHR1cmVzIHRoYXQgd2lsbCBiZSB1c2VkIGZyb20gYmVpbmcg cmVwbGFjZWQKKyAgICAgICAgYW5kIHJlY3JlYXRlZCBhZ2Fpbi4KKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIHBsYXRmb3JtL2dyYXBoaWNzL2Nocm9t aXVtL1RpbGVkTGF5ZXJDaHJvbWl1bS5jcHA6CisgICAgICAgIChXZWJDb3JlOjpUaWxlZExheWVy Q2hyb21pdW06OnByb3RlY3RWaXNpYmxlVGlsZVRleHR1cmVzKToKKyAgICAgICAgKiBwbGF0Zm9y bS9ncmFwaGljcy9jaHJvbWl1bS9jYy9DQ0xheWVyVHJlZUhvc3QuY3BwOgorICAgICAgICAoV2Vi Q29yZTo6Q0NMYXllclRyZWVIb3N0Ojp1cGRhdGVMYXllcnMpOgorICAgICAgICAoV2ViQ29yZTo6 Q0NMYXllclRyZWVIb3N0Ojpwcm90ZWN0VmlzaWJsZVRpbGVUZXh0dXJlcyk6CisgICAgICAgIChX ZWJDb3JlKToKKyAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy9jaHJvbWl1bS9jYy9DQ0xheWVy VHJlZUhvc3QuaDoKKwogMjAxMi0wMi0wOCAgU2hhd24gU2luZ2ggIDxzaGF3bnNpbmdoQGNocm9t aXVtLm9yZz4KIAogICAgICAgICBbY2hyb21pdW1dIFJlbW92ZSBpbmNvcnJlY3QgZWFybHkgZXhp dCBpbiBDQ0RhbWFnZVRyYWNrZXIKSW5kZXg6IFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBo aWNzL2Nocm9taXVtL1RpbGVkTGF5ZXJDaHJvbWl1bS5jcHAKPT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNl L1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21pdW0vVGlsZWRMYXllckNocm9taXVtLmNw cAkocmV2aXNpb24gMTA3MTA4KQorKysgU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3Mv Y2hyb21pdW0vVGlsZWRMYXllckNocm9taXVtLmNwcAkod29ya2luZyBjb3B5KQpAQCAtMzI5LDcg KzMyOSw3IEBAIHZvaWQgVGlsZWRMYXllckNocm9taXVtOjppbnZhbGlkYXRlUmVjdCgKIAogdm9p ZCBUaWxlZExheWVyQ2hyb21pdW06OnByb3RlY3RWaXNpYmxlVGlsZVRleHR1cmVzKCkKIHsKLSAg ICBwcm90ZWN0VGlsZVRleHR1cmVzKEludFJlY3QoSW50UG9pbnQ6Onplcm8oKSwgY29udGVudEJv dW5kcygpKSk7CisgICAgcHJvdGVjdFRpbGVUZXh0dXJlcyh2aXNpYmxlTGF5ZXJSZWN0KCkpOwog fQogCiB2b2lkIFRpbGVkTGF5ZXJDaHJvbWl1bTo6cHJvdGVjdFRpbGVUZXh0dXJlcyhjb25zdCBJ bnRSZWN0JiBsYXllclJlY3QpCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGlj cy9jaHJvbWl1bS9jYy9DQ0xheWVyVHJlZUhvc3QuY3BwCj09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9X ZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL2Nocm9taXVtL2NjL0NDTGF5ZXJUcmVlSG9zdC5jcHAJ KHJldmlzaW9uIDEwNzEwOCkKKysrIFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL2No cm9taXVtL2NjL0NDTGF5ZXJUcmVlSG9zdC5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTQxNiw2ICs0 MTYsNyBAQCB2b2lkIENDTGF5ZXJUcmVlSG9zdDo6dXBkYXRlTGF5ZXJzKExheWVyCiAgICAgfQog CiAgICAgcmVzZXJ2ZVRleHR1cmVzKCk7CisgICAgcHJvdGVjdFZpc2libGVUaWxlVGV4dHVyZXMo KTsKIAogICAgIHBhaW50TGF5ZXJDb250ZW50cyhtX3VwZGF0ZUxpc3QsIFBhaW50VmlzaWJsZSk7 CiAgICAgaWYgKCFtX3RyaWdnZXJJZGxlUGFpbnRzKQpAQCAtNDQ4LDYgKzQ0OSwyMCBAQCB2b2lk IENDTGF5ZXJUcmVlSG9zdDo6cmVzZXJ2ZVRleHR1cmVzKCkKICAgICB9CiB9CiAKK3ZvaWQgQ0NM YXllclRyZWVIb3N0Ojpwcm90ZWN0VmlzaWJsZVRpbGVUZXh0dXJlcygpCit7CisgICAgLy8gVXNl IEJhY2tUb0Zyb250IHNpbmNlIGl0J3MgY2hlYXAgYW5kIHRoaXMgaXNuJ3Qgb3JkZXItZGVwZW5k ZW50LgorICAgIHR5cGVkZWYgQ0NMYXllckl0ZXJhdG9yPExheWVyQ2hyb21pdW0sIFJlbmRlclN1 cmZhY2VDaHJvbWl1bSwgQ0NMYXllckl0ZXJhdG9yQWN0aW9uczo6QmFja1RvRnJvbnQ+IENDTGF5 ZXJJdGVyYXRvclR5cGU7CisKKyAgICBDQ0xheWVySXRlcmF0b3JUeXBlIGVuZCA9IENDTGF5ZXJJ dGVyYXRvclR5cGU6OmVuZCgmbV91cGRhdGVMaXN0KTsKKyAgICBmb3IgKENDTGF5ZXJJdGVyYXRv clR5cGUgaXQgPSBDQ0xheWVySXRlcmF0b3JUeXBlOjpiZWdpbigmbV91cGRhdGVMaXN0KTsgaXQg IT0gZW5kOyArK2l0KSB7CisgICAgICAgIC8vIEFzc3VtZSB0aGUgbGF5ZXJzIHdpdGggYWx3YXlz UmVzZXJ2ZVRleHR1cmVzIGhhdmUgcmVzZXJ2ZWQgdGhlaXIgdGV4dHVyZXMuCisgICAgICAgIGlm IChpdC5yZXByZXNlbnRzVGFyZ2V0UmVuZGVyU3VyZmFjZSgpIHx8IGl0LT5hbHdheXNSZXNlcnZl VGV4dHVyZXMoKSkKKyAgICAgICAgICAgIGNvbnRpbnVlOworICAgICAgICBpdC0+cHJvdGVjdFZp c2libGVUaWxlVGV4dHVyZXMoKTsKKyAgICB9Cit9CisKIC8vIHN0YXRpYwogdm9pZCBDQ0xheWVy VHJlZUhvc3Q6OnBhaW50Q29udGVudHNJZkRpcnR5KExheWVyQ2hyb21pdW0qIGxheWVyLCBQYWlu dFR5cGUgcGFpbnRUeXBlLCBjb25zdCBSZWdpb24mIG9jY2x1ZGVkU2NyZWVuU3BhY2UpCiB7Cklu ZGV4OiBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9jaHJvbWl1bS9jYy9DQ0xheWVy VHJlZUhvc3QuaAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGlj cy9jaHJvbWl1bS9jYy9DQ0xheWVyVHJlZUhvc3QuaAkocmV2aXNpb24gMTA3MTA4KQorKysgU291 cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21pdW0vY2MvQ0NMYXllclRyZWVIb3N0 LmgJKHdvcmtpbmcgY29weSkKQEAgLTIxMiw2ICsyMTIsNyBAQCBwcml2YXRlOgogICAgIHZvaWQg dXBkYXRlTGF5ZXJzKExheWVyQ2hyb21pdW0qKTsKICAgICAvLyBQcmUtcmVzZXJ2ZSB0ZXh0dXJl cyBmb3IgYW55IGxheWVyIG1hcmtlZCAiYWx3YXlzIHJlc2VydmUgdGV4dHVyZXMiCiAgICAgdm9p ZCByZXNlcnZlVGV4dHVyZXMoKTsKKyAgICB2b2lkIHByb3RlY3RWaXNpYmxlVGlsZVRleHR1cmVz KCk7CiAgICAgdm9pZCBjbGVhclBlbmRpbmdVcGRhdGUoKTsKIAogICAgIGludCBtX2NvbXBvc2l0 b3JJZGVudGlmaWVyOwo=