[Chromium] Implementing a global limit on memory consumed by deferred 2D canvases
Created attachment 159415 [details] Patch
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.
Created attachment 159737 [details] Patch
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.
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).
Created attachment 160208 [details] Patch
(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.
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.
Created attachment 160470 [details] Patch
(In reply to comment #9) > Created an attachment (id=160470) [details] > Patch Implemented suggestion from comment #8
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.
Created attachment 160479 [details] Patch
(In reply to comment #12) > Created an attachment (id=160479) [details] > Patch Resolve patch conflict
Comment on attachment 160479 [details] Patch r=me (bots willing).
Comment on attachment 160479 [details] Patch Clearing flags on attachment: 160479 Committed r126620: <http://trac.webkit.org/changeset/126620>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by 94976
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
D'oh! The failing ASSERT should been '>= 0' rather than '> 0'.
Created attachment 160707 [details] Patch
Comment on attachment 160707 [details] Patch OK. r=me
Comment on attachment 160707 [details] Patch Clearing flags on attachment: 160707 Committed r126760: <http://trac.webkit.org/changeset/126760>
This kills webkit_unit_tests in debug - isInList() in one of the ASSERT()s is doing infinite recursion.
https://code.google.com/p/chromium/issues/detail?id=231365