Bug 72672

Created attachment 116882 [details] Patch

Comment on attachment 116882 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116882&action=review So, just to sanity check, let me talk out loud what I'm seeing this patch do. Forgive me being long-winded, but hopefully this description will be helpful for others: The general idea is that uploads to resources onscreen need to be done into separate textures so that we don't get visible discrepancies. At the end of commit, all the layers/tiles that are visible get put in a list to the side. New managed textures are allocated for all of these resources so they get new texture ids. The commit completes and all contents textures are unprotected. On the next frame, the paint then happens on the main thread. When the next commit starts, we reduce memory to the texture limit, possibly evicting and deleting some of these previously visible textures in that list on the side. Incremental upload occurs. Tree sync happens. At the end of commit, we clear the off-to-the-side texture list, which puts them on the eviction list (if they haven't been deleted), providing a potential cache of evicted textures to use when requesting textures for the next upload. There's additional logic to reuse these potentially evicted textures to prevent texture churn. Then, we start the process over again with moving all currently visible tile/layer textures into that list and replacing them, etc. I have a couple of questions: We currently keep around all the visible in-use tiles, but I'm not totally convinced this is sufficient for offscreen tiles. For example, what if you have tiles A and B that are both offscreen on frame n, but become visible in frame n+1 due to scrolling from either thread. Because they weren't visible on frame n, that means that their textures haven't been replaced. Therefore, they will upload into the same texture ids on frame n+1, possibly causing a visual discrepancy if A uploads and a frame is drawn before B uploads. I'm not sure we want to put *all* textures onto a list every frame, but maybe there's some other more elegant solution. How would this interact with prepainting? Would we consider those textures "in use"? I would suspect you'd need to, otherwise you could scroll on the impl thread onto prepainted but now visible tiles that were being incrementally uploaded as you watched. I almost wonder if it would be more straightforwward to just ask for new textures ahead of time when you know you're going to modify them, rather than trying to preserve and replace them afterwards. Did you consider that sort of approach? I wonder if that might solve the previous problem as well. What prevents a texture in the m_usedTextureList from getting evicted due to memory pressure, getting recycled during requestTexture, and then uploaded on the very next frame. Wouldn't that cause an onscreen texture to get modified while in use? (Or worse, what would prevent a currently onscreen texture from getting deleted during deleteEvictedTextures in beginCommitOnImplThread? I suppose this could happen right now before this patch.) Do we need to reserve all of these textures in m_usedTextureList after we unreserve all the contents textures? I'd feel much better with a test case that bumped up against memory limits and exercised these edge cases. Am I reading it right that all the visible textures go into the used list, even the ones that aren't being modified that frame? If we do end up needing to reserve all the textures in the used list, maybe we should only put the ones that are modified there, so that we don't end up with 2x the average contents texture memory usage. > Source/WebCore/platform/graphics/chromium/ManagedTexture.cpp:69 > + m_textureId = m_textureManager->textureId(m_token); Can you add a comment that the texture id might not be valid here? > Source/WebCore/platform/graphics/chromium/ManagedTexture.h:43 > - static PassOwnPtr<ManagedTexture> create(TextureManager* manager) > + static PassRefPtr<ManagedTexture> create(TextureManager* manager) I'm probably just missing something obvious, but why does this need to become a RefPtr instead of an OwnPtr? It looks like in all cases you transfer ownership from the layer/updater into the m_usedTextureList, which then gets cleared on the next frame. I continue to be skeptical of RefPtr usage (largely because it makes it hard to reason about when and where destruction occurs), and would prefer the clearer semantics of OwnPtr when possible.

Created attachment 117840 [details] Patch

Thanks for having a look at this. (In reply to comment #2) > (From update of attachment 116882 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=116882&action=review > > So, just to sanity check, let me talk out loud what I'm seeing this patch do. Forgive me being long-winded, but hopefully this description will be helpful for others: > > The general idea is that uploads to resources onscreen need to be done into separate textures so that we don't get visible discrepancies. At the end of commit, all the layers/tiles that are visible get put in a list to the side. New managed textures are allocated for all of these resources so they get new texture ids. The commit completes and all contents textures are unprotected. On the next frame, the paint then happens on the main thread. When the next commit starts, we reduce memory to the texture limit, possibly evicting and deleting some of these previously visible textures in that list on the side. Incremental upload occurs. Tree sync happens. At the end of commit, we clear the off-to-the-side texture list, which puts them on the eviction list (if they haven't been deleted), providing a potential cache of evicted textures to use when requesting textures for the next upload. There's additional logic to reuse these potentially evicted textures to prevent texture churn. Then, we start the process over again with moving all currently visible tile/layer textures into that list and replacing them, etc. I think you got it just right. The new patch will only put the textures being updated in the used texture list instead of all textures. Things are the same other than that. > > I have a couple of questions: > > We currently keep around all the visible in-use tiles, but I'm not totally convinced this is sufficient for offscreen tiles. For example, what if you have tiles A and B that are both offscreen on frame n, but become visible in frame n+1 due to scrolling from either thread. Because they weren't visible on frame n, that means that their textures haven't been replaced. Therefore, they will upload into the same texture ids on frame n+1, possibly causing a visual discrepancy if A uploads and a frame is drawn before B uploads. I'm not sure we want to put *all* textures onto a list every frame, but maybe there's some other more elegant solution. Yes, this has been fixed in latest patch. > > How would this interact with prepainting? Would we consider those textures "in use"? I would suspect you'd need to, otherwise you could scroll on the impl thread onto prepainted but now visible tiles that were being incrementally uploaded as you watched. I almost wonder if it would be more straightforwward to just ask for new textures ahead of time when you know you're going to modify them, rather than trying to preserve and replace them afterwards. Did you consider that sort of approach? I wonder if that might solve the previous problem as well. I think initially we should just consider prepainted tiles as normal tiles in relation to this patch. I don't understand what you mean by "ask for new textures ahead of time when you know you're going to modify them, rather than trying to preserve and replace them afterwards.". Can you elaborate some more? > > What prevents a texture in the m_usedTextureList from getting evicted due to memory pressure, getting recycled during requestTexture, and then uploaded on the very next frame. Wouldn't that cause an onscreen texture to get modified while in use? (Or worse, what would prevent a currently onscreen texture from getting deleted during deleteEvictedTextures in beginCommitOnImplThread? I suppose this could happen right now before this patch.) Do we need to reserve all of these textures in m_usedTextureList after we unreserve all the contents textures? I'd feel much better with a test case that bumped up against memory limits and exercised these edge cases. A list of textures currently used by the impl thread would be helpful here. I guess we just need to make sure that textures currently used by the impl thread are never evicted. I'll work on putting together some unit tests for this. > > Am I reading it right that all the visible textures go into the used list, even the ones that aren't being modified that frame? If we do end up needing to reserve all the textures in the used list, maybe we should only put the ones that are modified there, so that we don't end up with 2x the average contents texture memory usage. I changed this in the latest patch. Only texture that are updated are put in the list now. > > > Source/WebCore/platform/graphics/chromium/ManagedTexture.cpp:69 > > + m_textureId = m_textureManager->textureId(m_token); > > Can you add a comment that the texture id might not be valid here? Done. > > > Source/WebCore/platform/graphics/chromium/ManagedTexture.h:43 > > - static PassOwnPtr<ManagedTexture> create(TextureManager* manager) > > + static PassRefPtr<ManagedTexture> create(TextureManager* manager) > > I'm probably just missing something obvious, but why does this need to become a RefPtr instead of an OwnPtr? It looks like in all cases you transfer ownership from the layer/updater into the m_usedTextureList, which then gets cleared on the next frame. I continue to be skeptical of RefPtr usage (largely because it makes it hard to reason about when and where destruction occurs), and would prefer the clearer semantics of OwnPtr when possible. Yeah, some initial work I did on this patch required RefPtr but the current version doesn't. I changed it to OwnPtr in latest patch.

(In reply to comment #4) > Thanks for having a look at this. > > (In reply to comment #2) > > How would this interact with prepainting? Would we consider those textures "in use"? I would suspect you'd need to, otherwise you could scroll on the impl thread onto prepainted but now visible tiles that were being incrementally uploaded as you watched. I almost wonder if it would be more straightforwward to just ask for new textures ahead of time when you know you're going to modify them, rather than trying to preserve and replace them afterwards. Did you consider that sort of approach? I wonder if that might solve the previous problem as well. > > I don't understand what you mean by "ask for new textures ahead of time when you know you're going to modify them, rather than trying to preserve and replace them afterwards.". Can you elaborate some more? So, right now, at the end of commit, you're replacing textures that are in use. My suggestion was to allocate new tokens/textures directly at the point you know you're going to change their contents. If you're uploading into a tile texture, for instance, you could call some special "reserveNew" function that put the old texture token on some eviction list (one that couldn't get reused this frame) and generated a new token for that managed texture. If the tile contents are still valid, you could just use the old reserve function and not allocate a new texture for it. > > What prevents a texture in the m_usedTextureList from getting evicted due to memory pressure, getting recycled during requestTexture, and then uploaded on the very next frame. Wouldn't that cause an onscreen texture to get modified while in use? (Or worse, what would prevent a currently onscreen texture from getting deleted during deleteEvictedTextures in beginCommitOnImplThread? I suppose this could happen right now before this patch.) Do we need to reserve all of these textures in m_usedTextureList after we unreserve all the contents textures? I'd feel much better with a test case that bumped up against memory limits and exercised these edge cases. > > A list of textures currently used by the impl thread would be helpful here. I guess we just need to make sure that textures currently used by the impl thread are never evicted. I think all the textures that the impl thread knows about are potentially usable. I think reserving textures is the mechanism that already exists to prevent these textures that are being used from being evicted. > I'll work on putting together some unit tests for this. Excellent.

(In reply to comment #5) > (In reply to comment #4) > > Thanks for having a look at this. > > > > (In reply to comment #2) > > > > How would this interact with prepainting? Would we consider those textures "in use"? I would suspect you'd need to, otherwise you could scroll on the impl thread onto prepainted but now visible tiles that were being incrementally uploaded as you watched. I almost wonder if it would be more straightforwward to just ask for new textures ahead of time when you know you're going to modify them, rather than trying to preserve and replace them afterwards. Did you consider that sort of approach? I wonder if that might solve the previous problem as well. > > > > I don't understand what you mean by "ask for new textures ahead of time when you know you're going to modify them, rather than trying to preserve and replace them afterwards.". Can you elaborate some more? > > So, right now, at the end of commit, you're replacing textures that are in use. My suggestion was to allocate new tokens/textures directly at the point you know you're going to change their contents. If you're uploading into a tile texture, for instance, you could call some special "reserveNew" function that put the old texture token on some eviction list (one that couldn't get reused this frame) and generated a new token for that managed texture. If the tile contents are still valid, you could just use the old reserve function and not allocate a new texture for it. This is pretty much how the new version of patch works. Though I still use the replaceTexture mechanism. I like the idea of having this controlled from outside the texture manager as there will be situations where we don't need new textures for commits (CCSingleThreadProxy) and when we can release the textures used by the impl thread will be different when we land the upcoming async commit patch. > > > > What prevents a texture in the m_usedTextureList from getting evicted due to memory pressure, getting recycled during requestTexture, and then uploaded on the very next frame. Wouldn't that cause an onscreen texture to get modified while in use? (Or worse, what would prevent a currently onscreen texture from getting deleted during deleteEvictedTextures in beginCommitOnImplThread? I suppose this could happen right now before this patch.) Do we need to reserve all of these textures in m_usedTextureList after we unreserve all the contents textures? I'd feel much better with a test case that bumped up against memory limits and exercised these edge cases. > > > > A list of textures currently used by the impl thread would be helpful here. I guess we just need to make sure that textures currently used by the impl thread are never evicted. > > I think all the textures that the impl thread knows about are potentially usable. I think reserving textures is the mechanism that already exists to prevent these textures that are being used from being evicted. Yea, the problem is that textures currently used by the impl thread are not reserved, right?

Created attachment 118415 [details] Patch Protect visible textures properly.

Comment on attachment 118415 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118415&action=review (What patch is this supposed to apply against?) > Source/WebCore/platform/graphics/chromium/ManagedTexture.cpp:70 > + // This will return 0 if texture has not yet been allocated. > + m_textureId = m_textureManager->textureId(m_token); This is unnecessary, given line 65 above. > Source/WebCore/platform/graphics/chromium/TextureManager.cpp:316 > + info.textureId = m_evictedTextures[i].textureId; You should also set the textureId reference here to return this to the caller. > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:381 > + if (tile->m_firstUpdate) { > tile->m_dirtyLayerRect = m_tiler->tileLayerRect(tile); Don't we set the dirty layer rect in createTile? Why do we need this flag? > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:-387 > - if (!tile->managedTexture()->reserve(m_tiler->tileSize(), m_textureFormat)) { > - m_skipsDraw = true; > - cleanupResources(); > - return; Where does this logic move to? > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:389 > + if (tile->m_dirtyLayerRect.isEmpty()) > + continue; > + > + if (!tile->managedTexture()->isValid(m_tiler->tileSize(), m_textureFormat)) > + tile->m_dirtyLayerRect = m_tiler->tileLayerRect(tile); I think these checks are in the wrong order? Or, the early out for an empty dirty rect shouldn't be there? If a tile texture isn't valid but that tile hasn't been invalidated, doesn't it still need to be painted and considered dirty? > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:401 > + // Unprotect all textures prior to calling protectTextures(). > + // protectTextures() will reserve all visible textures and leave > + // the rest unprotected. > + m_layerTreeHost->contentsTextureManager()->unprotectAllTextures(); Am I right in understanding that you unprotect/protect here so that visible textures from the previous frame and the current frame are still reserved and won't get evicted and deleted at the beginning of the frame? What about textures visible in frame n-2 but not visible now? I guess I'm just not sure that "visible" is the right set to be protecting here. Any texture that the impl tree knows about shouldn't be evicted and deleted, right?

Sorry for delayed response. (In reply to comment #8) > (From update of attachment 118415 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=118415&action=review > > (What patch is this supposed to apply against?) I just split 72752 into two patches so this patch now depends on both 72752 and 72192. > > > Source/WebCore/platform/graphics/chromium/ManagedTexture.cpp:70 > > + // This will return 0 if texture has not yet been allocated. > > + m_textureId = m_textureManager->textureId(m_token); > > This is unnecessary, given line 65 above. Oh, I didn't notice a textureId reference being passed to requestTexture. That's exactly what we need here. > > > Source/WebCore/platform/graphics/chromium/TextureManager.cpp:316 > > + info.textureId = m_evictedTextures[i].textureId; > > You should also set the textureId reference here to return this to the caller. > > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:381 > > + if (tile->m_firstUpdate) { > > tile->m_dirtyLayerRect = m_tiler->tileLayerRect(tile); > > Don't we set the dirty layer rect in createTile? Why do we need this flag? Fixed. > > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:-387 > > - if (!tile->managedTexture()->reserve(m_tiler->tileSize(), m_textureFormat)) { > > - m_skipsDraw = true; > > - cleanupResources(); > > - return; > > Where does this logic move to? Should have moved to protectTileTextures. Fixed in new patch. > > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:389 > > + if (tile->m_dirtyLayerRect.isEmpty()) > > + continue; > > + > > + if (!tile->managedTexture()->isValid(m_tiler->tileSize(), m_textureFormat)) > > + tile->m_dirtyLayerRect = m_tiler->tileLayerRect(tile); > > I think these checks are in the wrong order? Or, the early out for an empty dirty rect shouldn't be there? If a tile texture isn't valid but that tile hasn't been invalidated, doesn't it still need to be painted and considered dirty? I was trying to use the valid state of textures to determine whether they can be partially updated or not. I realized that this doesn't work as textures can then be invalid for two reasons: 1. Been reused because they are not visible. 2. Requires non-partial update. The new patch that I'm about to upload does this differently. First a quick explanation of what this patch needs to accomplish; if partial texture uploads are allowed or not should be controlled by the CCProxy. The CCProxy also needs to be able to control what textures are protected as they need to be protected until they are no longer used by the impl thread. The tricky part is that the layer area that needs to be updated depend on if partial updates can be used or not. So this information must be available to prepareToUpdateIfDirty(). My new patch does the following: In CCThreadProxy::beginFrameAndCommit(): 1. Call unprotectAllTextures() 2. Call prepareToUpdate(). prepareToUpdateIfDirty() can tell if partial updates are necessary based on if they are protected or not. In this case none of them are protected. [1] 3. Steal all textures that have been determined to need update as these textures, if valid, are currently used by the impl thread. 4. Protect all impl side textures that are about to be updated. 5. Call protectTextures(). This will reserve and protect all visible textures including the ones that we're about to update. 6. prepareFrameResourcesOnImplThread. This is the step that runs synchronously on the impl side and allows the texture updater to allocate compositor resources. 7. Call updateContents() to paint and start uploading of new textures. 8. Commit. 9. Release textures previously used by the impl thread. Here's the simpler CCSingleThreadProxy method: 1. Call prepareToUpdate(). All currently used textures are protected so prepareToUpdateIfDirty() knows that partial updates can be used. 2. Call unprotectAllTextures(). 3. Call protectTextures(). Reserve and protect currently visible textures. 7. prepareFrameResourcesOnImplThread. 7. Call updateContents() to paint and start uploading of new textures. 8. Commit. Let me know if there's any part of this that doesn't seem correct. [1] We might want to use a different property than isProtected to tell if partial update is possible.

Created attachment 118715 [details] Patch

(In reply to comment #9) I'm replying to your strategy here and not the patch itself. > My new patch does the following: > > In CCThreadProxy::beginFrameAndCommit(): > > 1. Call unprotectAllTextures() This might be minor, but if possible can we can we do this at the end of commit? I think the logic is easier to understand if we don't carry texture reservation between frames. > 2. Call prepareToUpdate(). prepareToUpdateIfDirty() can tell if partial updates are necessary based on if they are protected or not. In this case none of them are protected. [1] > 3. Steal all textures that have been determined to need update as these textures, if valid, are currently used by the impl thread. > 4. Protect all impl side textures that are about to be updated. Just for clarification, do you mean protect the old textures that were replaced or the new replacement textures? > 5. Call protectTextures(). This will reserve and protect all visible textures including the ones that we're about to update. > 6. prepareFrameResourcesOnImplThread. This is the step that runs synchronously on the impl side and allows the texture updater to allocate compositor resources. > 7. Call updateContents() to paint and start uploading of new textures. > 8. Commit. > 9. Release textures previously used by the impl thread. By release, do you mean evict? Or delete? Or both? In general, where does texture deletion happen in this strategy? > Here's the simpler CCSingleThreadProxy method: > > 1. Call prepareToUpdate(). All currently used textures are protected so prepareToUpdateIfDirty() knows that partial updates can be used. > 2. Call unprotectAllTextures(). > 3. Call protectTextures(). Reserve and protect currently visible textures. > 7. prepareFrameResourcesOnImplThread. > 7. Call updateContents() to paint and start uploading of new textures. > 8. Commit. > > > [1] We might want to use a different property than isProtected to tell if partial update is possible. Yes, please. Maybe pass an explicit setting to the layer or something? I think I'd just prefer the two proxies to behave as similarly as is reasonably possible with respect to commit behavior, just for sanity's sake. > Let me know if there's any part of this that doesn't seem correct. Two things: klobag added a texture recycling patch not so long ago (see TextureManager::replaceTexture) that, when allocating textures over the reclamation limit pulls non-protected (and non-evicted) textures off the LRU list and reuses them. It seems like with the above scheme we could replace an in-use texture on the impl thread that wasn't visible and protected. I'm not sure that patch is compatible at all with non-atomic updates. Do you have any thoughts on how to make these work together? What about LRU and texture reservation? It seems like we want to definitely evict the old version of any modified textures at the end of the frame, but if we reserved them (in step #4) then they'd move to the front of the LRU queue and other (still valid) textures might get evicted first. How is this handled?

(In reply to comment #11) > (In reply to comment #9) > > I'm replying to your strategy here and not the patch itself. Should we have a phone call to hammer our strategy out here? High level, we're going on 2 weeks on this bug...

(In reply to comment #11) > (In reply to comment #9) > > I'm replying to your strategy here and not the patch itself. > > > My new patch does the following: > > > > In CCThreadProxy::beginFrameAndCommit(): > > > > 1. Call unprotectAllTextures() > > This might be minor, but if possible can we can we do this at the end of commit? I think the logic is easier to understand if we don't carry texture reservation between frames. The reason I did it here is that I want to make sure that textures used by the impl thread are protected between frames. I guess it would be safe to move this to the end of commit if I can make the assumption that no texture reservation is done except for when LayerChromium::protectTextures() is called. Btw, with the async commit support I'm working on, end of commit becomes a bit more fuzzy.. > > > 2. Call prepareToUpdate(). prepareToUpdateIfDirty() can tell if partial updates are necessary based on if they are protected or not. In this case none of them are protected. [1] > > 3. Steal all textures that have been determined to need update as these textures, if valid, are currently used by the impl thread. > > 4. Protect all impl side textures that are about to be updated. > > Just for clarification, do you mean protect the old textures that were replaced or the new replacement textures? New replacement textures and old textures that are visible but don't need update. > > > 5. Call protectTextures(). This will reserve and protect all visible textures including the ones that we're about to update. > > 6. prepareFrameResourcesOnImplThread. This is the step that runs synchronously on the impl side and allows the texture updater to allocate compositor resources. > > 7. Call updateContents() to paint and start uploading of new textures. > > 8. Commit. > > 9. Release textures previously used by the impl thread. > > By release, do you mean evict? Or delete? Or both? In general, where does texture deletion happen in this strategy? By release, I mean that the texture manager tokens are release as a result of deleting ManagedTexture instances. Texture manager puts textures in the evicted textures list when tokens are released. Texture deletion is happening in the same place as before: CCLayerTreeHost::beginCommitOnImplThread. > > > Here's the simpler CCSingleThreadProxy method: > > > > 1. Call prepareToUpdate(). All currently used textures are protected so prepareToUpdateIfDirty() knows that partial updates can be used. > > 2. Call unprotectAllTextures(). > > 3. Call protectTextures(). Reserve and protect currently visible textures. > > 7. prepareFrameResourcesOnImplThread. > > 7. Call updateContents() to paint and start uploading of new textures. > > 8. Commit. > > > > > > [1] We might want to use a different property than isProtected to tell if partial update is possible. > > Yes, please. Maybe pass an explicit setting to the layer or something? I think I'd just prefer the two proxies to behave as similarly as is reasonably possible with respect to commit behavior, just for sanity's sake. I think it makes more sense to have this be a ManagedTexture or TextureManager setting rather than part of the layer. Asynchronous commit support will require the commit and texture protection process to be a bit different between the two proxies. So one of goals of this patch is to allow some of that difference. No more than necessary though. > > > Let me know if there's any part of this that doesn't seem correct. > > Two things: > > klobag added a texture recycling patch not so long ago (see TextureManager::replaceTexture) that, when allocating textures over the reclamation limit pulls non-protected (and non-evicted) textures off the LRU list and reuses them. It seems like with the above scheme we could replace an in-use texture on the impl thread that wasn't visible and protected. I'm not sure that patch is compatible at all with non-atomic updates. Do you have any thoughts on how to make these work together? Yes, we can't allow that to happen. My current approach of keeping all visible textures protected between frames should make sure this isn't a problem. The texture recycling can still make use of non-visible textures as they wont be protected. > > What about LRU and texture reservation? It seems like we want to definitely evict the old version of any modified textures at the end of the frame, but if we reserved them (in step #4) then they'd move to the front of the LRU queue and other (still valid) textures might get evicted first. How is this handled? The old version of the modified textures are always evicted as soon as is safe to do so. They should be last in the LRU list as they are protected before all other textures but it shouldn't matter as the ManagedTexture instances are deleted which forces them to be evicted independent of their LRU position.

(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #9) > > > > I'm replying to your strategy here and not the patch itself. > > Should we have a phone call to hammer our strategy out here? High level, we're going on 2 weeks on this bug... Sounds good. I'll schedule something asap.

(In reply to comment #13) > (In reply to comment #11) Re: phonecall. I think this is on the right path, and I hope it doesn't seem like I'm trying to block this patch. It's more that I got this wrong originally, and I want to make sure that there's not some corner case where we're going to be displaying inconsistent textures to the user. I suspect it'll be extremely difficult to repro such an interaction between commit and texture eviction, so I'd like to make sure we get this right. > > klobag added a texture recycling patch not so long ago (see TextureManager::replaceTexture) that, when allocating textures over the reclamation limit pulls non-protected (and non-evicted) textures off the LRU list and reuses them. It seems like with the above scheme we could replace an in-use texture on the impl thread that wasn't visible and protected. I'm not sure that patch is compatible at all with non-atomic updates. Do you have any thoughts on how to make these work together? > > Yes, we can't allow that to happen. My current approach of keeping all visible textures protected between frames should make sure this isn't a problem. The texture recycling can still make use of non-visible textures as they wont be protected. ...but the non-visible/non-protected textures could become visible on the impl thread, via impl-only scrolling, and get reused for some entirely different texture while it's visible to the user. That's my worry. Am I missing something?

(In reply to comment #15) > (In reply to comment #13) > > (In reply to comment #11) > > Re: phonecall. I think this is on the right path, and I hope it doesn't seem like I'm trying to block this patch. It's more that I got this wrong originally, and I want to make sure that there's not some corner case where we're going to be displaying inconsistent textures to the user. I suspect it'll be extremely difficult to repro such an interaction between commit and texture eviction, so I'd like to make sure we get this right. I think I have something that works well now but wouldn't be comfortable landing this unless all of us understand exactly how it works and agree that it's accurate. A phone call seems like a good way to get there. > > > > klobag added a texture recycling patch not so long ago (see TextureManager::replaceTexture) that, when allocating textures over the reclamation limit pulls non-protected (and non-evicted) textures off the LRU list and reuses them. It seems like with the above scheme we could replace an in-use texture on the impl thread that wasn't visible and protected. I'm not sure that patch is compatible at all with non-atomic updates. Do you have any thoughts on how to make these work together? > > > > Yes, we can't allow that to happen. My current approach of keeping all visible textures protected between frames should make sure this isn't a problem. The texture recycling can still make use of non-visible textures as they wont be protected. > > ...but the non-visible/non-protected textures could become visible on the impl thread, via impl-only scrolling, and get reused for some entirely different texture while it's visible to the user. > > That's my worry. Am I missing something? OK, I wasn't sure about this. We just need to protect all textures then. This is a minor change. hm, this makes the texture manager (except the reuse of evicted textures part) sort of useless though.

The dependencies listed on this bug still bother me. This needs to be independent of https://bugs.webkit.org/show_bug.cgi?id=72192 and of https://bugs.webkit.org/show_bug.cgi?id=72752, if for no other reason than we need atomicity on some platforms where we don't want to initialize before painting or upload in parallel with painting. Is there an actual code dependency on the features those patches implement?

(In reply to comment #17) > The dependencies listed on this bug still bother me. This needs to be independent of https://bugs.webkit.org/show_bug.cgi?id=72192 and of https://bugs.webkit.org/show_bug.cgi?id=72752, if for no other reason than we need atomicity on some platforms where we don't want to initialize before painting or upload in parallel with painting. Is there an actual code dependency on the features those patches implement? Yes, this depends on features provided by those patches. I'm working on a way around that but it's probably going to take significantly longer to get that ready than landing those patches.

(In reply to comment #18) > (In reply to comment #17) > > The dependencies listed on this bug still bother me. This needs to be independent of https://bugs.webkit.org/show_bug.cgi?id=72192 and of https://bugs.webkit.org/show_bug.cgi?id=72752, if for no other reason than we need atomicity on some platforms where we don't want to initialize before painting or upload in parallel with painting. Is there an actual code dependency on the features those patches implement? > > Yes, this depends on features provided by those patches. I'm working on a way around that but it's probably going to take significantly longer to get that ready than landing those patches. We aren't going to enable the _features_ in those patches (like parallel uploading) on all platforms, but we still need atomicity. So what's the plan?

(In reply to comment #18) > (In reply to comment #17) > > The dependencies listed on this bug still bother me. This needs to be independent of https://bugs.webkit.org/show_bug.cgi?id=72192 and of https://bugs.webkit.org/show_bug.cgi?id=72752, if for no other reason than we need atomicity on some platforms where we don't want to initialize before painting or upload in parallel with painting. Is there an actual code dependency on the features those patches implement? > > Yes, this depends on features provided by those patches. I'm working on a way around that but it's probably going to take significantly longer to get that ready than landing those patches. Or perhaps by features do you mean code constructs (like the iterators), but not actual features in terms of new behaviors? If so then please hoist those into this patch and make it the root of the dependency tree. We need atomicity in order to enable the already-checked-in-but-disabled incremental upload logic. As far as I know we do not need any other features in order to have incremental uploads.

Created attachment 119619 [details] Patch

New version without dependencies.

Comment on attachment 119619 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119619&action=review The basic idea seems good but there's some logic here that is either incomplete or that I don't understand. > Source/WebCore/platform/graphics/chromium/ManagedTexture.cpp:110 > + ManagedTexture* texture = new ManagedTexture(m_textureManager, m_token, m_size, m_format, m_textureId); in any constructor function like this you should adoptPtr() and store into a local OwnPtr<> immediately. > Source/WebCore/platform/graphics/chromium/ManagedTexture.h:65 > + PassOwnPtr<ManagedTexture> steal(); What does 'steal()' mean? This needs some docs > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:386 > + if (tile->isDirty() && layerTreeHost() && !layerTreeHost()->settings().partialUpdates) > + layerTreeHost()->appendUsedTextureOnImplThread(tile->managedTexture()->steal()); so if I understand correctly, if we hit this logic we 'steal' the texture to make sure that the isValid() check on the next line fails. this seems a bit indirect - could we instead swap out the tiler's ManagedTexture for a new one in a less magical fashion? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:97 > + // Update m_settings based on partial update capability. > + m_settings.partialUpdates = m_proxy->partialUpdateCapability(); if it's a capability, why is it on CCLayerTreeHost::settings() instead of CCLayerTreeHost::capabilities() ? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:512 > +void CCLayerTreeHost::appendUsedTextureOnImplThread(PassOwnPtr<ManagedTexture> texture) > +{ > + m_usedTexturesOnImplThread.append(texture); if you have a function like this that is expected to be called on a particular thread, please check that with an ASSERT(CCProxy::isImplThread()). > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:243 > + TextureList m_usedTexturesOnImplThread; what's this used for? I see a call to append entries to this, and I see a call to clear it, but I don't see any logic that uses this vector. > Source/WebCore/platform/graphics/chromium/cc/CCProxy.h:83 > + virtual bool partialUpdateCapability() const = 0; > + I'm not sure what this means and the name isn't helping me. What does it mean to have a 'partial update capability'? Naively I would expect this to mean that the proxy is capable of doing partial updates, but the implementations return true for CCSingleThreadProxy and false for CCThreadProxy which is the exact opposite of what I would expect. > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:47 > +static const size_t updatesPerFrame = 5; the only place that (usefully) uses this value is in scheduleActionUpdateMoreResources(), so I think it'd be clearer to leave this value a function static const in there > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:578 > + return updatesPerFrame == numeric_limits<size_t>::max(); this doesn't make any sense - both constants you are comparing here are defined inside this .cpp and clearly aren't equal. Why isn't this just 'return false'? > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:181 > + std::vector<WebGLId> m_textures; > + std::set<WebGLId> m_usedTextures; using namespace std; please

Comment on attachment 119619 [details] Patch Setting r- for now until feedback is addressed.

(In reply to comment #23) > (From update of attachment 119619 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=119619&action=review > > The basic idea seems good but there's some logic here that is either incomplete or that I don't understand. > > > Source/WebCore/platform/graphics/chromium/ManagedTexture.cpp:110 > > + ManagedTexture* texture = new ManagedTexture(m_textureManager, m_token, m_size, m_format, m_textureId); > > in any constructor function like this you should adoptPtr() and store into a local OwnPtr<> immediately. Sure, I'll fix that if we want to keep this constructor. > > > Source/WebCore/platform/graphics/chromium/ManagedTexture.h:65 > > + PassOwnPtr<ManagedTexture> steal(); > > What does 'steal()' mean? This needs some docs > > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:386 > > + if (tile->isDirty() && layerTreeHost() && !layerTreeHost()->settings().partialUpdates) > > + layerTreeHost()->appendUsedTextureOnImplThread(tile->managedTexture()->steal()); > > so if I understand correctly, if we hit this logic we 'steal' the texture to make sure that the isValid() check on the next line fails. this seems a bit indirect - could we instead swap out the tiler's ManagedTexture for a new one in a less magical fashion? You can find an alternative to the "steal()" approach in the first attachment of this bug. That patch instead adds a LayerTextureUpdater::Texture::replaceTexture function. Do you prefer that? I'm not a huge fan of either approach so let me know if you have any other ideas. > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:97 > > + // Update m_settings based on partial update capability. > > + m_settings.partialUpdates = m_proxy->partialUpdateCapability(); > > if it's a capability, why is it on CCLayerTreeHost::settings() instead of CCLayerTreeHost::capabilities() ? It's not a renderer capability so I don't think it should be part of CCLayerTreeHost::layerRendererCapabilities(). Are you suggesting that we add a CCLayerTreeHost::capabilities()? Sure, I can do that if you prefer that over putting it in CCLayerTreeHost::settings(). > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:512 > > +void CCLayerTreeHost::appendUsedTextureOnImplThread(PassOwnPtr<ManagedTexture> texture) > > +{ > > + m_usedTexturesOnImplThread.append(texture); > > if you have a function like this that is expected to be called on a particular thread, please check that with an ASSERT(CCProxy::isImplThread()). hm, need a better name for that function. It's not actually supposed to be called on the impl thread. It's called on the main thread and adds a texture to the list of textures used on the impl thread. So the "OnImplThread" prefix is accurate but confusing as already used to indicate that the function should be called on the impl thread. > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:243 > > + TextureList m_usedTexturesOnImplThread; > > what's this used for? I see a call to append entries to this, and I see a call to clear it, but I don't see any logic that uses this vector. It's just a list of textures that can't be released because they might be used by the impl thread for intermediate drawing between texture uploads. Textures are added to this list during "prepareToUpdate" and then released after commit completion when we know that they are not used by impl thread anymore. > > > Source/WebCore/platform/graphics/chromium/cc/CCProxy.h:83 > > + virtual bool partialUpdateCapability() const = 0; > > + > > I'm not sure what this means and the name isn't helping me. What does it mean to have a 'partial update capability'? Naively I would expect this to mean that the proxy is capable of doing partial updates, but the implementations return true for CCSingleThreadProxy and false for CCThreadProxy which is the exact opposite of what I would expect. It means that the proxy supports partial texture updates. If this is not supported by the proxy, full texture updates are required. Maybe partialTextureUpdateCapability() is a better name? > > > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:47 > > +static const size_t updatesPerFrame = 5; > > the only place that (usefully) uses this value is in scheduleActionUpdateMoreResources(), so I think it'd be clearer to leave this value a function static const in there Well, we need it to determine if partial texture updates are possible. See below. > > > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:578 > > + return updatesPerFrame == numeric_limits<size_t>::max(); > > this doesn't make any sense - both constants you are comparing here are defined inside this .cpp and clearly aren't equal. Why isn't this just 'return false'? The code was slightly adjusted to allow updatesPerFrame to be configurable. A value of std::numeric_limits<size_t>::max() would be equal to disabling interleaved texture updates. Being able to experiment with different updatesPerFrame values by just changing the updatesPerFrame constant and not having to remember to adjust the value returned by CCThreadProxy::partialUpdateCapability() is what I prefer. I'll change this to "return false" and make "updatesPerFrame" a function static const if you prefer that. > > > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:181 > > + std::vector<WebGLId> m_textures; > > + std::set<WebGLId> m_usedTextures; > > using namespace std; please Sure. FYI, std:: is currently used instead of "using namespace std" everywhere in chromium/tests except for ScrollAnimatorNoneTest.cpp.

I see, and the ManagedTextures are held alive artificially so that their texture IDs aren't reused by the TextureManager? This needs some comments at least. Is there no better way to deal with texture reuse?

Created attachment 121449 [details] Patch

(In reply to comment #26) > I see, and the ManagedTextures are held alive artificially so that their texture IDs aren't reused by the TextureManager? This needs some comments at least. Is there no better way to deal with texture reuse? It's not really a reuse issue. It's just that the textures used on the impl thread can't be deleted until the commit completes. Updated patch includes these changes: - Have ManagedTexture::steal() store in local OwnPtr<> immediately. - CCLayerTreeHost::appendUsedTextureOnImplThread renamed to CCLayerTreeHost::deleteTextureAfterCommit. - Rename partialUpdateCapability() to partialTextureUpdateCapability(). - Rename CCThreadProxy.cpp: updatesPerFrame to textureUpdatesPerFrame. - Add "using namespace std" to CCLayerTreeHostTest.cpp. - Add comments to explain ManagedTexture::steal() and its use.

Comment on attachment 121449 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121449&action=review > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:380 > + if (tile->isDirty() && layerTreeHost() && !layerTreeHost()->settings().partialTextureUpdates) > + layerTreeHost()->deleteTextureAfterCommit(tile->managedTexture()->steal()); one concern i have with this is that it'll force us to do full tile paints + uploads even when on the last upload, for instance if we damage less than 5 tiles total. Example: open this page data:text/html;charset=utf-8,%3C!DOCTYPE%20html%3E%3Cinput%3E and put focus in the input area so the cursor blinks. without this patch, the paints+uploads are 3x16 pixels. with this patch, they are 256x256 and if you get really unlucky with tile boundaries they grow to 1024x1024. that's a pretty serious regression, and I think invalidations that are under the incremental upload limit are the common case. can we somehow make the growth only apply to uploads before the final batch?

Comment on attachment 121449 [details] Patch R-, the performance impact here is too heavy.

(In reply to comment #29) > (From update of attachment 121449 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=121449&action=review > > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:380 > > + if (tile->isDirty() && layerTreeHost() && !layerTreeHost()->settings().partialTextureUpdates) > > + layerTreeHost()->deleteTextureAfterCommit(tile->managedTexture()->steal()); > > one concern i have with this is that it'll force us to do full tile paints + uploads even when on the last upload, for instance if we damage less than 5 tiles total. > > Example: open this page data:text/html;charset=utf-8,%3C!DOCTYPE%20html%3E%3Cinput%3E and put focus in the input area so the cursor blinks. without this patch, the paints+uploads are 3x16 pixels. with this patch, they are 256x256 and if you get really unlucky with tile boundaries they grow to 1024x1024. that's a pretty serious regression, and I think invalidations that are under the incremental upload limit are the common case. > > can we somehow make the growth only apply to uploads before the final batch? Sure, that would be a nice improvement to this patch. How about I update this patch so the functionality can be tested without turning on incremental updates and implement the "final batch" improvement in a follow up patch?

Created attachment 121676 [details] Patch

(In reply to comment #31) > (In reply to comment #29) > > (From update of attachment 121449 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=121449&action=review > > > > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:380 > > > + if (tile->isDirty() && layerTreeHost() && !layerTreeHost()->settings().partialTextureUpdates) > > > + layerTreeHost()->deleteTextureAfterCommit(tile->managedTexture()->steal()); > > > > one concern i have with this is that it'll force us to do full tile paints + uploads even when on the last upload, for instance if we damage less than 5 tiles total. > > > > Example: open this page data:text/html;charset=utf-8,%3C!DOCTYPE%20html%3E%3Cinput%3E and put focus in the input area so the cursor blinks. without this patch, the paints+uploads are 3x16 pixels. with this patch, they are 256x256 and if you get really unlucky with tile boundaries they grow to 1024x1024. that's a pretty serious regression, and I think invalidations that are under the incremental upload limit are the common case. > > > > can we somehow make the growth only apply to uploads before the final batch? > > Sure, that would be a nice improvement to this patch. How about I update this patch so the functionality can be tested without turning on incremental updates and implement the "final batch" improvement in a follow up patch? I think that's a great approach to take. Can you file a bug about that?

Comment on attachment 121676 [details] Patch R=me

Created attachment 123378 [details] Patch

Rebased patch and created: https://bugs.webkit.org/show_bug.cgi?id=76740

Comment on attachment 123378 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123378&action=review Very close, but tests need a bit of love still. > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:136 > +class CompositorFakeWebGraphicsContext3D : public FakeWebGraphicsContext3D { how is this different from the one in CompositorFakeWebGraphicsContext3D.h? can you use that one instead, or base this fake off of that fake to add the additional functionality you need (like texture tracking)? > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:186 > + vector<WebGLId> m_textures; > + set<WebGLId> m_usedTextures; we don't use STL containers in WebKit, even in unit tests. Use the WTF types

Created attachment 123393 [details] Patch

Comment on attachment 123393 [details] Patch R=me, thanks

Comment on attachment 123393 [details] Patch Clearing flags on attachment: 123393 Committed r105564: <http://trac.webkit.org/changeset/105564>

All reviewed patches have been landed. Closing bug.

This appears not to compile: ____Ld ../../../../../xcodebuild/Debug/webkit_unit_tests [...] Undefined symbols: "__ZN7WebCore19CCLayerTreeHostImpl23startPageScaleAnimationERKNS_7IntSizeEbfdd", referenced from: __ZTVN12_GLOBAL__N_121MockLayerTreeHostImplE in CCLayerTreeHostTest.o ld: symbol(s) not found clang:error: linker command failed with exit code 1 (use -v to see invocation)

(On Chromium Mac)

(In reply to comment #42) > This appears not to compile: > > > ____Ld ../../../../../xcodebuild/Debug/webkit_unit_tests > [...] > Undefined symbols: > "__ZN7WebCore19CCLayerTreeHostImpl23startPageScaleAnimationERKNS_7IntSizeEbfdd", referenced from: > __ZTVN12_GLOBAL__N_121MockLayerTreeHostImplE in CCLayerTreeHostTest.o > ld: symbol(s) not found > clang:error: linker command failed with exit code 1 (use -v to see invocation) You sure this patch is causing this? Looks like r105566 is causing it: https://bugs.webkit.org/show_bug.cgi?id=71529

Reopening to attach new patch.

Created attachment 123465 [details] Patch

Latest patch avoids use of std::numeric_limits to fix this build issue: ../platform/graphics/chromium/cc/CCThreadProxy.cpp:47:21: error: declaration requires a global constructor [-Werror,-Wglobal-constructors] static const size_t textureUpdatesPerFrame = numeric_limits<size_t>::max(); ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Comment on attachment 123465 [details] Patch Clearing flags on attachment: 123465 Committed r105583: <http://trac.webkit.org/changeset/105583>

All reviewed patches have been landed. Closing bug.