Bug 94386 – [Chromium] Implementing a global limit on memory consumed by deferred 2D canvases

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Justin Novosad]

[Chromium] Implementing a global limit on memory consumed by deferred 2D canvases

[Justin Novosad]

Created attachment 159415 [details]
Patch

[Kenneth Russell]

Comment on attachment 159415 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159415&action=review

This looks pretty good to me overall but I defer to senorblanco and jamesr for a more thorough review. One license-related issue though.

> Source/WebCore/platform/graphics/chromium/DeferredLayer.cpp:16
> + * this software without specific prior written permission.

Wrong license. Here and throughout, please use the two-clause license in Source/WebKit/LICENSE.

[Justin Novosad]

Created attachment 159737 [details]
Patch

[Justin Novosad]

New patch fixes:
- license issue from comment 2
- DeferredLayerManager::isInList was not working properly because next/prev linked list pointers were not reset to 0 when removing an element from the list
- Improved unit test coverage for isInList.

[Stephen White]

Comment on attachment 159737 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159737&action=review

I'm still not fond of this solution.  It's not really "global" since it's only per-render process.  But you've managed to convince me that there are use cases for which this level of complexity is necessary.  If possible, let's try to hang the manager off the GraphicsContext3D at some level, rather than using a singleton.

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.cpp:191
> +        didDraw();

This is a shame; I was hoping we could get rid of contextAcquired() altogether once we removed the non-deferred path.  Oh well.

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.h:47
> +class Canvas2DLayerBridge : public WebKit::WebExternalTextureLayerClient, public SkDeferredCanvas::NotificationClient, public DeferredLayer {

Please avoid multiple implementation inheritance, if possible.  If you must derive, please derive from the DoublyLinkedListNode<> directly, and just maintain a list of those in the manager.  (I think DeferredLayer could then go away).

> Source/WebCore/platform/graphics/chromium/DeferredLayer.cpp:35
> +    , m_manager(&DeferredLayerManager::get())

Since it's a singleton, might as well just call DeferredLayerManager::get() on demand, and save the memory here.

> Source/WebCore/platform/graphics/chromium/DeferredLayerManager.cpp:71
> +            m_layerList.push(layer); // Set as MRU

I'm not too fond of adding the complexity of MRU cache, but you've convinced me that there is a use case apart from navigation for which it's necessary (the game with separate "contexts" and multiple canvases).

[Justin Novosad]

Created attachment 160208 [details]
Patch

[Justin Novosad]

(In reply to comment #6)
> Created an attachment (id=160208) [details]
> Patch

New patch addresses feedback from comment #5.
After discussion, opted to not not hang the layer manager off of GraphicsContext3D and keep it as a singleton.

[Stephen White]

Comment on attachment 160208 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160208&action=review

Thanks for the fixes.  Remaining comments are optional.  r=me

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.cpp:114
> +    Canvas2DLayerManager::get().layerAllocatedStorageChanged(this);

If you wanted to, I think you could remove m_newBytesAllocated in each layer by passing the old and new sizes (or the delta) directly to the manager when they're changed on the layer.  Then you wouldn't need to pick them up later.

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.cpp:127
> +    m_newBytesAllocated = deferredCanvas()->storageAllocatedForRecording();

Same as above.

[Justin Novosad]

Created attachment 160470 [details]
Patch

[Justin Novosad]

(In reply to comment #9)
> Created an attachment (id=160470) [details]
> Patch

Implemented suggestion from comment #8

[Stephen White]

Comment on attachment 160470 [details]
Patch

Looks good.  However, the EWS bots are having trouble; I think you might have to update.  r=me in any case.

[Justin Novosad]

Created attachment 160479 [details]
Patch

[Justin Novosad]

(In reply to comment #12)
> Created an attachment (id=160479) [details]
> Patch

Resolve patch conflict

[Stephen White]

Comment on attachment 160479 [details]
Patch

r=me (bots willing).

[WebKit Review Bot]

Comment on attachment 160479 [details]
Patch

Clearing flags on attachment: 160479

Committed r126620: <http://trac.webkit.org/changeset/126620>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 94976

[Julien Chaffraix]

One ASSERT is triggering on the bots:

crash log for DumpRenderTree (pid 19068):
STDOUT: <empty>
STDERR: ASSERTION FAILED: (intptr_t)m_bytesAllocated + deltaBytes > 0
STDERR: third_party/WebKit/Source/WebCore/platform/graphics/chromium/Canvas2DLayerManager.cpp(88) : void WebCore::Canvas2DLayerManager::layerAllocatedStorageChanged(WebCore::Canvas2DLayerBridge*, intptr_t)
STDERR: 1   0x7fc4feee5408
STDERR: 2   0x7fc4feee468c
STDERR: 3   0x7fc4feee46d2
STDERR: 4   0x7fc4fa75aa28
STDERR: 5   0x7fc4fa75aa5c
STDERR: 6   0x7fc4fa7aeff8 SkCanvas::flush()
STDERR: 7   0x7fc4feee49e0
STDERR: 8   0x7fc4fe3bffec
STDERR: 9   0x7fc4ffaf02bc
STDERR: 10  0x7fc4ffb26a5e
STDERR: 11  0x7fc4ffb26210
STDERR: 12  0x7fc4ffb25ff1
STDERR: 13  0x7fc4ffb776ee
STDERR: 14  0x7fc4ffb76730
STDERR: 15  0x7fc4ffb2563e
STDERR: 16  0x7fc4fe3c324b WebKit::WebLayerTreeView::compositeAndReadback(void*, WebKit::WebRect const&)
STDERR: 17  0x7fc4fdd76d4c
STDERR: 18  0x7fc4fdd770ac
STDERR: 19  0x4cd696
STDERR: 20  0x4cd91c
STDERR: 21  0x4bb957
STDERR: 22  0x4ba238
STDERR: 23  0x492440
STDERR: 24  0x4939a7
STDERR: 25  0x4ccac4
STDERR: 26  0x4c9766
STDERR: 27  0x7fc4fdcb5589
STDERR: 28  0x7fc4ff5863bc
STDERR: 29  0x7fc4ff586fc6
STDERR: 30  0x7fc4ff561d50
STDERR: 31  0x7fc4ff59bfe7
STDERR: [19068:19068:13306426671345:ERROR:process_util_posix.cc(143)] Received signal 11
STDERR: 	base::debug::StackTrace::StackTrace() [0x7fc4fb805ea6]
STDERR: 	base::(anonymous namespace)::StackDumpSignalHandler() [0x7fc4fb86ceed]
STDERR: 	0x7fc4f52ccaf0
STDERR: 	WebCore::Canvas2DLayerManager::layerAllocatedStorageChanged() [0x7fc4feee5412]
STDERR: 	WebCore::Canvas2DLayerBridge::storageAllocatedForRecordingChanged() [0x7fc4feee468c]
STDERR: 	WebCore::Canvas2DLayerBridge::flushedDrawCommands() [0x7fc4feee46d2]
STDERR: 	DeferredDevice::flushPending() [0x7fc4fa75aa28]
STDERR: 	DeferredDevice::flush() [0x7fc4fa75aa5c]
STDERR: 	SkCanvas::flush() [0x7fc4fa7aeff8]
STDERR: 	WebCore::Canvas2DLayerBridge::prepareTexture() [0x7fc4feee49e0]
STDERR: 	WebKit::WebExternalTextureLayerImpl::prepareTexture() [0x7fc4fe3bffec]
STDERR: 	WebCore::TextureLayerChromium::update() [0x7fc4ffaf02bc]
STDERR: 	WebCore::CCLayerTreeHost::paintLayerContents() [0x7fc4ffb26a5e]
STDERR: 	WebCore::CCLayerTreeHost::updateLayers() [0x7fc4ffb26210]
STDERR: 	WebCore::CCLayerTreeHost::updateLayers() [0x7fc4ffb25ff1]
STDERR: 	WebCore::CCSingleThreadProxy::commitAndComposite() [0x7fc4ffb776ee]
STDERR: 	WebCore::CCSingleThreadProxy::compositeAndReadback() [0x7fc4ffb76730]
STDERR: 	WebCore::CCLayerTreeHost::compositeAndReadback() [0x7fc4ffb2563e]
STDERR: 	WebKit::WebLayerTreeView::compositeAndReadback() [0x7fc4fe3c324b]
STDERR: 	WebKit::WebViewImpl::doPixelReadbackToCanvas() [0x7fc4fdd76d4c]
STDERR: 	WebKit::WebViewImpl::paint() [0x7fc4fdd770ac]
STDERR: 	WebViewHost::paintRect() [0x4cd696]
STDERR: 	WebViewHost::paintInvalidatedRegion() [0x4cd91c]
STDERR: 	TestShell::dump() [0x4bb957]
STDERR: 	TestShell::testFinished() [0x4ba238]
STDERR: 	DRTTestRunner::WorkQueue::processWorkSoon() [0x492440]
STDERR: 	DRTTestRunner::locationChangeDone() [0x4939a7]
STDERR: 	WebViewHost::locationChangeDone() [0x4ccac4]
STDERR: 	WebViewHost::didFinishLoad() [0x4c9766]
STDERR: 	WebKit::FrameLoaderClientImpl::dispatchDidFinishLoad() [0x7fc4fdcb5589]
STDERR: 	WebCore::FrameLoader::checkLoadCompleteForThisFrame() [0x7fc4ff5863bc]
STDERR: 	WebCore::FrameLoader::checkLoadComplete() [0x7fc4ff586fc6]
STDERR: 	WebCore::DocumentLoader::finishedLoading() [0x7fc4ff561d50]
STDERR: 	WebCore::MainResourceLoader::didFinishLoading() [0x7fc4ff59bfe7]
STDERR: 	WebCore::ResourceLoader::didFinishLoading() [0x7fc4ff5b04f9]
STDERR: 	WebCore::ResourceHandleInternal::didFinishLoading() [0x7fc4fef8135a]
STDERR: 	webkit_glue::WebURLLoaderImpl::Context::OnCompletedRequest() [0x7fc4f9dab154]
STDERR: 	(anonymous namespace)::RequestProxy::NotifyCompletedRequest() [0x60288f]
STDERR: 	base::internal::RunnableAdapter<>::Run() [0x608c84]
STDERR: 	base::internal::InvokeHelper<>::MakeItSo() [0x60862e]
STDERR: 	base::internal::Invoker<>::Run() [0x607f40]
STDERR: 	base::Callback<>::Run() [0x7fc4fb7fe049]
STDERR: 	MessageLoop::RunTask() [0x7fc4fb83fb2e]
STDERR: 	MessageLoop::DeferOrRunPendingTask() [0x7fc4fb83fc46]
STDERR: 	MessageLoop::DoWork() [0x7fc4fb840461]
STDERR: 	base::MessagePumpGlib::HandleDispatch() [0x7fc4fb7e4271]
STDERR: 	(anonymous namespace)::WorkSourceDispatch() [0x7fc4fb7e398b]
STDERR: 	0x7fc4f60258c2
STDERR: 	0x7fc4f6029748
STDERR: 	0x7fc4f60298fc
STDERR: 	base::MessagePumpGlib::RunWithDispatcher() [0x7fc4fb7e3f20]
STDERR: 	base::MessagePumpGlib::Run() [0x7fc4fb7e434e]
STDERR: 	MessageLoop::RunInternal() [0x7fc4fb83f7f3]
STDERR: 	MessageLoop::RunHandler() [0x7fc4fb83f6aa]
STDERR: 	base::RunLoop::Run() [0x7fc4fb872ba6]
STDERR: 	MessageLoop::Run() [0x7fc4fb83efd8]
STDERR: 	webkit_support::RunMessageLoop() [0x52c119]
STDERR: 	TestShell::waitTestFinished() [0x4c0be2]
STDERR: 	TestShell::runFileTest() [0x4b9ca4]
STDERR: 	runTest() [0x485a82]
STDERR: 	main [0x486443]
STDERR: 	0x7fc4f52b7c4d

List of the tests triggering it:

compositing/layer-creation/spanOverlapsCanvas.html
compositing/masks/layer-mask-placement.html
compositing/overflow/image-load-overflow-scrollbars.html
compositing/overflow/overflow-hidden-canvas-layer.html
platform/chromium/compositing/scissor-out-of-viewport.html

[Justin Novosad]

D'oh! The failing ASSERT should been '>= 0' rather than '> 0'.

[Justin Novosad]

Created attachment 160707 [details]
Patch

[Stephen White]

Comment on attachment 160707 [details]
Patch

OK.  r=me

[WebKit Review Bot]

Comment on attachment 160707 [details]
Patch

Clearing flags on attachment: 160707

Committed r126760: <http://trac.webkit.org/changeset/126760>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

This kills webkit_unit_tests in debug - isInList() in one of the ASSERT()s is doing infinite recursion.

[Stephen Chenney]

https://code.google.com/p/chromium/issues/detail?id=231365