Limit the number of WebGL contexts that are active at any given time

Limit the number of WebGL contexts that are active at any given time

Context count is limited per render process, and is limited to either an
arbitrary count (currently 16) or a cumulative pixel count
(currently 16 * 1024 * 1024). If either limit is exceeded the oldest context
will be forcibly lost to allow the new one to be created. If an active context
is removed a previously lost context will attempt to restore.

BUG=226868
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=149135

[bajones, 2013-04-15 18:15:49]

Found a bug that I'll be addressing: If the number of canvases is limited by
pixel storage instead of the max context number removing a canvas (via garbage
collection) will cause all inactive canvases to cycle through a context restore,
not just the ones that fit in the pixel allocation.

[Ken Russell (switch to Gerrit), 2013-04-16 02:57:52]

Good first step. We've talked offline about this already; a couple of comments
based on the current state of the code.

https://codereview.chromium.org/14217005/diff/1/Source/WebCore/html/canvas/We...
File Source/WebCore/html/canvas/WebGLRenderingContext.cpp (right):

https://codereview.chromium.org/14217005/diff/1/Source/WebCore/html/canvas/We...
Source/WebCore/html/canvas/WebGLRenderingContext.cpp:103:
if(activeContexts().size()) {
Add space between if and ( everywhere please.

https://codereview.chromium.org/14217005/diff/1/Source/WebCore/html/canvas/We...
Source/WebCore/html/canvas/WebGLRenderingContext.cpp:105:
activeContexts().remove(0);
The implicit assumption here is that contexts' age equals creation order. It
would be better to sort them by actual use. I think that
cleanupAfterGraphicsCall can be used to (relatively cheaply) track which is the
most recently accessed context. Perhaps it can increment a static variable and
store the result inside the context. Then they can be properly ranked.

https://codereview.chromium.org/14217005/diff/1/Source/WebCore/html/canvas/We...
Source/WebCore/html/canvas/WebGLRenderingContext.cpp:144:
if(oldestInactiveContext->m_restoreAllowed) {
If restoration isn't allowed, we lose track of oldestInactiveContext. Maybe this
is OK because the intent is that contexts with m_restoreAllowed == false never
show up in the inactive context list anyway? Is that a strong enough guarantee
that an assert is desired?

https://codereview.chromium.org/14217005/diff/1/Source/WebCore/html/canvas/We...
Source/WebCore/html/canvas/WebGLRenderingContext.cpp:5865:
deactivateContext(this, m_restoreAllowed);
This doesn't seem right. If a context receives a RealLostContext, then it
shouldn't be added to the inactive list, should it? It's going to be reactivated
later via a different mechanism.

https://codereview.chromium.org/14217005/diff/1/Source/WebCore/html/canvas/We...
File Source/WebCore/html/canvas/WebGLRenderingContext.h (right):

https://codereview.chromium.org/14217005/diff/1/Source/WebCore/html/canvas/We...
Source/WebCore/html/canvas/WebGLRenderingContext.h:327: static void
forceLostOldestContext();
The naming convention is getting confusing; I suggest
"forciblyLoseOldestContext" for this, even though it doesn't quite match the
other related methods (which could perhaps be renamed).

https://codereview.chromium.org/14217005/diff/1/Source/WebCore/html/canvas/We...
Source/WebCore/html/canvas/WebGLRenderingContext.h:793: static void
removeContext(WebGLRenderingContext*);
"removeContext" is too generic a name. "willDestroyContext" might be better.

[greggman, 2013-04-16 04:07:01]

I've said it before but I disagree with these limits.

Here's a page using 12 contexts
https://www.shadertoy.com/browse

Here's a page which if I scroll down a few pages I can easily get more than 16
instances of flash on the page. 

It's sad that we'll be hobbling WebGL with this CL more than we hobble Flash
even though Flash has similar issues.

[bajones, 2013-04-16 05:42:17]

On 2013/04/16 04:07:01, greggman wrote:
> I've said it before but I disagree with these limits.

I don't think anybody feels like this is the final solution to this problem, but
we do need to get something in place to handle worst-case scenarios like the
context-creation-and-destruction conformance test. Once this is in place we'll
be in a better position to work on a more elegant system.

That's a topic that deserves it's own discussion, though. I'll send out an email
momentarily.

[bajones, 2013-04-20 05:19:59]

Updated CL to address some earlier feedback and to rebase on the recent
DrawingBuffer changes, which simplifies the logic of this patch significantly.

https://codereview.chromium.org/14217005/diff/9001/Source/core/platform/graph...
File Source/core/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/14217005/diff/9001/Source/core/platform/graph...
Source/core/platform/graphics/gpu/DrawingBuffer.cpp:42: #include
"WebGLRenderingContext.h"
Ken already pointed out in a different patch review that this violates layering
rules, but I'm not sure what the appropriate approach is to allow this. Include
is currently required to provide access to
WebGLRenderingContext::forciblyLoseOldestContext();

[Ken Russell (switch to Gerrit), 2013-04-23 02:21:28]

I assume that Blink will carry forward WebKit's layering policies, but even if
not, it seems like a violation of abstraction barriers to have DrawingBuffer
making direct calls into WebGLRenderingContext. Can you add a simple abstract
class encompassing these two calls, and have WebGLRenderingContext register an
instance which dispatches them to its static methods?

[jamesr, 2013-04-23 03:33:09]

Yes, same rules apply.  DrawingBuffer is more abstract and can be used by things
other than WebGL so assuming that a given DrawingBuffer always maps to a WebGL
rendering context is not guaranteed to always be true in the future, either.

[jamesr, 2013-04-23 04:05:56]

Alternately, since WebGLRenderingContext has the calls to
DrawingBuffer::resize()/reset() already, why not just keep the eviction logic in
WebGLRenderingContext as well?

[bajones, 2013-04-23 21:48:30]

Layering issues have been addressed.

[Ken Russell (switch to Gerrit), 2013-04-24 01:30:56]

This looks good overall. I'd like to take one final look before commit.

https://codereview.chromium.org/14217005/diff/22001/Source/core/html/canvas/W...
File Source/core/html/canvas/WebGLRenderingContext.cpp (right):

https://codereview.chromium.org/14217005/diff/22001/Source/core/html/canvas/W...
Source/core/html/canvas/WebGLRenderingContext.cpp:575: m_drawingBuffer =
DrawingBuffer::create(m_context.get(), clampedCanvasSize(), preserve,
contextEvictionManager.get());
Use PassRefPtr in the callee (see the comment elsewhere) and then here use
release(). See http://www.webkit.org/coding/RefPtr.html .

https://codereview.chromium.org/14217005/diff/22001/Source/core/platform/grap...
File Source/core/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/14217005/diff/22001/Source/core/platform/grap...
Source/core/platform/graphics/gpu/DrawingBuffer.cpp:459:
m_contextEvictionManager.clear();
Shouldn't the eviction manager be used across multiple resize operations, since
it's set in the constructor?

https://codereview.chromium.org/14217005/diff/22001/Source/core/platform/grap...
Source/core/platform/graphics/gpu/DrawingBuffer.cpp:662: if(s_maxScaleAttempts >
-1 && scaleAttempts > s_maxScaleAttempts)
Is it strictly necessary to look for s_maxScaleAttempts being > -1? Wouldn't
this code just be removed if it's decided not to pay attention to it? Also,
please add a space between if and ( please (throughout this review, and in
general).

https://codereview.chromium.org/14217005/diff/22001/Source/core/platform/grap...
Source/core/platform/graphics/gpu/DrawingBuffer.cpp:678: return adjustedSize; //
Buffer fits without evicting a context
Please end comments in with periods here and throughout.

https://codereview.chromium.org/14217005/diff/22001/Source/core/platform/grap...
File Source/core/platform/graphics/gpu/DrawingBuffer.h (right):

https://codereview.chromium.org/14217005/diff/22001/Source/core/platform/grap...
Source/core/platform/graphics/gpu/DrawingBuffer.h:133: bool
packedDepthStencilExtensionSupported, PreserveDrawingBuffer,
ContextEvictionManager*);
This should be PassRefPtr<ContextEvictionManager>.

[bajones, 2013-04-24 03:37:08]

Thanks, I'll have these addressed first thing tomorrow.

https://codereview.chromium.org/14217005/diff/22001/Source/core/platform/grap...
File Source/core/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/14217005/diff/22001/Source/core/platform/grap...
Source/core/platform/graphics/gpu/DrawingBuffer.cpp:459:
m_contextEvictionManager.clear();
On 2013/04/24 01:30:57, kbr wrote:
> Shouldn't the eviction manager be used across multiple resize operations,
since
> it's set in the constructor?
It is. clear() is only called when the DrawingBuffer is being discarded. A
single eviction manager will be used for the lifetime of the DrawingBuffer.

https://codereview.chromium.org/14217005/diff/22001/Source/core/platform/grap...
Source/core/platform/graphics/gpu/DrawingBuffer.cpp:662: if(s_maxScaleAttempts >
-1 && scaleAttempts > s_maxScaleAttempts)
On 2013/04/24 01:30:57, kbr wrote:
> please add a space between if and ( please (throughout this review, and in
> general).
I swear I WILL remember this style rule eventually!

https://codereview.chromium.org/14217005/diff/22001/Source/core/platform/grap...
Source/core/platform/graphics/gpu/DrawingBuffer.cpp:678: return adjustedSize; //
Buffer fits without evicting a context
On 2013/04/24 01:30:57, kbr wrote:
> Please end comments in with periods here and throughout.
This one too. Years of habit working against me. *sigh*

[jamesr, 2013-04-24 03:43:16]

On 2013/04/23 04:05:56, jamesr wrote:
> Alternately, since WebGLRenderingContext has the calls to
> DrawingBuffer::resize()/reset() already, why not just keep the eviction logic
in
> WebGLRenderingContext as well?

Any answers to this question?

[bajones, 2013-04-24 03:47:43]

On 2013/04/24 03:43:16, jamesr wrote:
> On 2013/04/23 04:05:56, jamesr wrote:
> > Alternately, since WebGLRenderingContext has the calls to
> > DrawingBuffer::resize()/reset() already, why not just keep the eviction
logic
> in
> > WebGLRenderingContext as well?
> 
> Any answers to this question?

I misspoke when I originally said it was just forciblyLoseOldestContext that was
being called. We also need to get the oldest context size. I tried out both
passing the appropriate state and querying it via an abstract class and the
abstract class eventually won out for clarity and flexibility.

[bajones, 2013-04-24 18:40:23]

Note to self: Firefox currently displays a console message when their context
limit is exceeded, we should probably do the same. Also, Firefox has a limit of
16 contexts, and it probably wouldn't hurt to make our limit the same for the
time being for consistency.

[greggman, 2013-04-24 18:51:04]

On 2013/04/24 18:40:23, bajones wrote:
> Note to self: Firefox currently displays a console message when their context
> limit is exceeded, we should probably do the same.
> Also, Firefox has a limit of
> 16 contexts, and it probably wouldn't hurt to make our limit the same for the
> time being for consistency.

No, IMO. By that argument we should get rid of EXT_draw_buffers for consistency.
 We should also disable anti-aliasing everywhere they do and not everywhere they
don't etc...

[bajones, 2013-04-24 21:31:57]

Ken's feedback is addressed and console warnings have been added when a context
is forcibly lost.

[Ken Russell (switch to Gerrit), 2013-04-24 22:39:00]

Thanks for pressing forward on this.

LGTM. Per our offline discussion, it's clear that GPU process side resources
still aren't being cleaned up until the WebGLRenderingContext is GC'd and the
GraphicsContext3D / WebGraphicsContext3D are deleted. If it's possible to
address this in this patch that would be good, but if it's too much additional
code, let's land this as is and do that work in another patch.

[bajones, 2013-04-25 00:03:55]

On 2013/04/24 22:39:00, kbr wrote:
> Thanks for pressing forward on this.
> 
> LGTM. Per our offline discussion, it's clear that GPU process side resources
> still aren't being cleaned up until the WebGLRenderingContext is GC'd and the
> GraphicsContext3D / WebGraphicsContext3D are deleted. If it's possible to
> address this in this patch that would be good, but if it's too much additional
> code, let's land this as is and do that work in another patch.

The GC patch is turning out to be a fair amount of code (reasonably simple, just
a lot of it) and I worry that it would add too much noise to this particular
patch. We'll probably want to land them separately, though if you'd like to
delay landing this one till the other patch is ready I'm okay with that.

[Ken Russell (switch to Gerrit), 2013-04-25 00:08:54]

Let's land this as is. Are you able to do this at this point?

[commit-bot: I haz the power, 2013-04-25 01:27:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/14217005/35001

[commit-bot: I haz the power, 2013-04-25 02:30:01]

Retried try job too often on win_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[commit-bot: I haz the power, 2013-04-25 17:52:19]

Failed to apply the patch.

[commit-bot: I haz the power, 2013-04-25 19:43:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/14217005/51001

[commit-bot: I haz the power, 2013-04-25 20:16:55]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[bajones, 2013-04-25 22:02:31]

Committed patchset #9 manually as r149135 (presubmit successful).