Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
910613003:
Trigger hardware accelerated canvas GC more aggressively. (Closed)
|
Created:
5 years, 10 months ago by Daniel Bratell Modified:
5 years, 10 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, dglazkov+blink Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionTrigger hardware accelerated canvas GC more aggressively.
A hardware accelerated canvas indirectly owns a number of GPU resources
and if they become too numerous we see drastic slowdowns. At the very
least we should let the GC know about the cost of an HTMLCanvasElement
so that it can correctly schedule garbage collects.
This makes test timeouts caused by buildup of HTMLCanvasElements in
canvas tests slightly less likely.
BUG=450699
R=jochen@chromium.org,junov@chromium.org
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190135
Patch Set 1 #Patch Set 2 : Lowered the constant to 2 and explained it. #Patch Set 3 : Better comment #Messages
Total messages: 17 (4 generated)
bratell@opera.com changed reviewers: + junov@chromium.org, mkwst@chromium.org
Please take a look. This is not a final, root, fix, but it's something that should be done anyway.
Deferring to junov@; I'm not a good reviewer for this code.
Let's see where the discussion blink-dev goes before landing this. I support this change, for lack of a better solution, That being said, I do have concerns about it. One thing I do not like about it is that it decreases the likelihood of the bug occurring, but does not actually guarantee that the issue is fixed, so the stability of this code remains at the mercy of changes to the GC trigger heuristics. Also, this change may be harmfully increasing GC frequency in some cases. We'd need more data to support this, or get some expert advice from reviewers who know GC/V8 better than me.
After reading your comment on the bug https://code.google.com/p/chromium/issues/detail?id=450699#c46 I wonder if we shouldn't be fudging the raw byte value instead of the buffer count. Something like: const size_t minCanvasCostInBytes = v8_external_memory_allocation_limit / canvas_limit; externallyAllocatedMemory = max(externallyAllocatedMemory, minCanvasCostInBytes);
jochen@chromium.org changed reviewers: + jochen@chromium.org
I think it would be preferable to either discard the texture memory more aggressively, or, if we can't do that, introduce a new API where we can communicate the GPU memory pressure to V8. We shouldn't report fake memory statistics to tip the V8 GC heuristic over, because the heuristic changes every now and then, and then this wrong reporting will result in some other, unexpected behavior
On 2015/02/10 19:47:43, jochen (slow) wrote: > I think it would be preferable to either discard the texture memory more > aggressively, or, if we can't do that, introduce a new API where we can > communicate the GPU memory pressure to V8. > > We shouldn't report fake memory statistics to tip the V8 GC heuristic over, > because the heuristic changes every now and then, and then this wrong reporting > will result in some other, unexpected behavior I think something extra should be reported to v8 for accelerated canvases because they are heavier on resources. The magical constant 8 is probably too high if we want it to reflect "truth" though. I just picked a smallish constant and checked if it fixed the problems. Now I know *why* increasing this fixes the problems and that is because enough "externally allocated memory" and we'll increase past the v8 kExternalAllocationLimit and then there will be a garbage collect and v8 will update its gc thresholds and it will start working as it should have done from the start. So I suggest changing the constant 8 to 2 which I fill reflect the true cost.
On 2015/02/11 at 09:46:16, bratell wrote: > On 2015/02/10 19:47:43, jochen (slow) wrote: > > I think it would be preferable to either discard the texture memory more > > aggressively, or, if we can't do that, introduce a new API where we can > > communicate the GPU memory pressure to V8. > > > > We shouldn't report fake memory statistics to tip the V8 GC heuristic over, > > because the heuristic changes every now and then, and then this wrong reporting > > will result in some other, unexpected behavior > > I think something extra should be reported to v8 for accelerated canvases because they are heavier on resources. The magical constant 8 is probably too high if we want it to reflect "truth" though. I just picked a smallish constant and checked if it fixed the problems. Now I know *why* increasing this fixes the problems and that is because enough "externally allocated memory" and we'll increase past the v8 kExternalAllocationLimit and then there will be a garbage collect and v8 will update its gc thresholds and it will start working as it should have done from the start. > > So I suggest changing the constant 8 to 2 which I fill reflect the true cost. if that's indeed the actual costs this sgtm (but wouldn't it be one additional buffer - i.e. the texture memory only)?
On 2015/02/12 08:28:09, jochen (slow) wrote: > On 2015/02/11 at 09:46:16, bratell wrote: > > So I suggest changing the constant 8 to 2 which I fill reflect the true cost. > > if that's indeed the actual costs this sgtm (but wouldn't it be one additional > buffer - i.e. the texture memory only)? It is a bit hard to judge and I am in no way an expert here, but it has seemed to me that there are two textures. One surface that is the "storage" of the image and one scratch texture which is saved in a cache so not temporary. I would not be surprised if this will change over time and will depend on exactly what you do with the canvas so I just consider it a small move in the right direction. Long term, if we are going to have many garbage collected GPU resources, v8 will have to learn about the resource limitation of GPUs and GPU drivers.
lgtm I guess we'll need to address the GPU resources problem indeed, as oilpan will also delay the release of resources.
On 2015/02/12 12:01:13, jochen (slow) wrote: > lgtm > > I guess we'll need to address the GPU resources problem indeed, as oilpan will > also delay the release of resources. The scratch textures are temp buffers that only exist while performing some specific types of tasks. I don<t think they are even worth mentioning. On the other hand, GPU-accelerated canvases are double and sometimes triple buffered when the contents are animated, this is due to how they interact with the compositor in Canvas2DLayerBridge. Once a canvas is no longer displayed however, it goes back to being single-buffered. So technically, a canvas that is awaiting GC should only ever be holding a single texture. That being said, we should have live canvases applying representative (triple) pressure on the GC heuristics in order to push the zombie canvases out. So accounting for triple buffering (+=2) on live animated canvases is totally justified. Applying the +=2 on all accelerated canvases is just being conservative, and it avoids the overhead of making continuous adjustments to the reported externalAllocatedMemeory as a canvas oscillates between being single, double and triple buffered. So lgtm with a more accurate comment.
On 2015/02/12 15:42:06, junov wrote: > So lgtm with a more accurate comment. Done.
New patchsets have been uploaded after l-g-t-m from jochen@chromium.org,junov@chromium.org
The CQ bit was checked by bratell@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/910613003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://src.chromium.org/viewvc/blink?view=rev&revision=190135 |
