Summary: | Place an upper bound on canvas pixel counts | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||||||||||||||||||
Component: | Layout and Rendering | Assignee: | Brent Fulgham <bfulgham> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | bfulgham, cdumez, commit-queue, esprehn+autocc, ggaren, gyuyoung.kim, mkwst, ryanhaddad, simon.fraser, webkit-bug-importer | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=152009 https://bugs.webkit.org/show_bug.cgi?id=154713 |
||||||||||||||||||||||
Bug Depends on: | 152143 | ||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||
Attachments: |
|
Description
Brent Fulgham
2015-12-03 14:43:16 PST
Created attachment 266563 [details]
Patch
Comment on attachment 266563 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=266563&action=review > Source/WebCore/html/HTMLCanvasElement.cpp:109 > + unsigned pixelsReleased = width() * height(); Why not use the existing HTMLCanvasElement::memoryCost() ? I think this should also only do decrease the number if there is a m_imageBuffer. > Source/WebCore/html/HTMLCanvasElement.cpp:195 > + return ramSize() / (4 * 8); Why? > Source/WebCore/html/HTMLCanvasElement.cpp:219 > + size_t requestedPixels = width() * height(); > + if (numActivePixels + requestedPixels > maxActivePixels()) Use the same math as memoryCost(). Comment on attachment 266563 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=266563&action=review >> Source/WebCore/html/HTMLCanvasElement.cpp:109 >> + unsigned pixelsReleased = width() * height(); > > Why not use the existing HTMLCanvasElement::memoryCost() ? > > I think this should also only do decrease the number if there is a m_imageBuffer. memoryCost only returns a value once the image buffer is created. I want to know if I'm going to exceed the limit before then. >> Source/WebCore/html/HTMLCanvasElement.cpp:195 >> + return ramSize() / (4 * 8); > > Why? It's arbitrary, but based on the following thoughts: Blink limits the process to 1 GB. Assuming our typical-to-lower-end systems are using 8 GB, this calculation gives the number of 32 bit pixels that match that 1 GB limit. >> Source/WebCore/html/HTMLCanvasElement.cpp:219 >> + if (numActivePixels + requestedPixels > maxActivePixels()) > > Use the same math as memoryCost(). To use memoryCost we need to create the image buffer first. Comment on attachment 266563 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=266563&action=review >>> Source/WebCore/html/HTMLCanvasElement.cpp:219 >>> + if (numActivePixels + requestedPixels > maxActivePixels()) >> >> Use the same math as memoryCost(). > > To use memoryCost we need to create the image buffer first. ... but, I could use memoryCost once the canvas context is created (and memoryCost when the context is destroyed) to keep track of actual memory use. That way, I don't improperly mark memory in use for a canvas that didn't actually use memory (if such things ever happen). Comment on attachment 266563 [details]
Patch
I think we should record explicitly the number of pixels we added to numActivePixels so that we subtract that number back later. Relying on width and height seems risky, since they can change.
(In reply to comment #6) > Comment on attachment 266563 [details] > Patch > > I think we should record explicitly the number of pixels we added to > numActivePixels so that we subtract that number back later. Relying on width > and height seems risky, since they can change. size changes are handled via setSurfaceSize(), and this code needs to account for them. Created attachment 266580 [details]
Patch
Comment on attachment 266580 [details] Patch Attachment 266580 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/513362 Number of test failures exceeded the failure limit. Created attachment 266583 [details]
Archive of layout-test-results from ews114 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 266580 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=266580&action=review > Source/WebCore/html/HTMLCanvasElement.cpp:618 > + if (m_imageBuffer) > + removeFromActivePixelMemory(memoryCost()); You could tie the book-keeping more closely with the buffer allocation/deallocation by adding HTMLCanvasElement::setImageBuffer(), and doing the add/remove pixel count stuff inside it. Comment on attachment 266580 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=266580&action=review >> Source/WebCore/html/HTMLCanvasElement.cpp:618 >> + removeFromActivePixelMemory(memoryCost()); > > You could tie the book-keeping more closely with the buffer allocation/deallocation by adding HTMLCanvasElement::setImageBuffer(), and doing the add/remove pixel count stuff inside it. Sure! Note that the actual memory cost of canvas elements is difficult to assess, because there are code paths where CanvasRenderingContext2D creates and returns blank image buffers. So, the actual memory use is more than just the internal image buffer. Created attachment 266666 [details]
Patch
Comment on attachment 266666 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=266666&action=review > Source/WebCore/html/HTMLCanvasElement.cpp:123 > + // Do not manually delete the image buffer -- it will be cleaned up by the JSC collector. > + removeFromActivePixelMemory(memoryCost()); > + > m_context = nullptr; // Ensure this goes away before the ImageBuffer. > } Why not setImageBuffer(nullptr) at the end of the function, so you don't need to call removeFromActivePixelMemory() explicitly. Comment on attachment 266666 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=266666&action=review >> Source/WebCore/html/HTMLCanvasElement.cpp:123 >> } > > Why not setImageBuffer(nullptr) at the end of the function, so you don't need to call removeFromActivePixelMemory() explicitly. For the same reason as the comment in 'getContext': // FIXME: The code depends on the context not going away once created, to prevent JS from // seeing a dangling pointer. So for now we will disallow the context from being changed" If I call 'setImageBuffer(nullptr)' here, JSC gets very upset that the image buffer went away without it's control. I can add a comment to that effect. > Source/WebCore/html/HTMLCanvasElement.cpp:204 > + maxPixelMemory = std::max(ramSize() / 4, 1024 * MB); I started off with ramSize() / 8, but some of our tests want to allocate more than that. (ramSize() / 8) + 1 seems to work, but this might be a slightly less drastic change from our current "allocate whatever you want" approach. Some test machines are resource constrained, so I set a lower bound on the max memory. Comment on attachment 266666 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=266666&action=review >>> Source/WebCore/html/HTMLCanvasElement.cpp:123 >>> } >> >> Why not setImageBuffer(nullptr) at the end of the function, so you don't need to call removeFromActivePixelMemory() explicitly. > > For the same reason as the comment in 'getContext': > // FIXME: The code depends on the context not going away once created, to prevent JS from > // seeing a dangling pointer. So for now we will disallow the context from being changed" > > If I call 'setImageBuffer(nullptr)' here, JSC gets very upset that the image buffer went away without it's control. > > I can add a comment to that effect. Oh! Yes, your way works perfectly. Much better -- I'll change it. Created attachment 266674 [details]
Patch
Comment on attachment 266674 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=266674&action=review > Source/WebCore/html/HTMLCanvasElement.cpp:608 > + stringBuilder.appendLiteral("Total canvas memory use exceeds the maximum limit ( "); No space after the paren in the string. > Source/WebCore/html/HTMLCanvasElement.cpp:611 > + document().addConsoleMessage(MessageSource::JS, MessageLevel::Warning, stringBuilder.toString()); Does this string look OK in the console? Comment on attachment 266674 [details] Patch Attachment 266674 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/517210 Number of test failures exceeded the failure limit. Created attachment 266678 [details]
Archive of layout-test-results from ews115 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 266666 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=266666&action=review >>>> Source/WebCore/html/HTMLCanvasElement.cpp:123 >>>> } >>> >>> Why not setImageBuffer(nullptr) at the end of the function, so you don't need to call removeFromActivePixelMemory() explicitly. >> >> For the same reason as the comment in 'getContext': >> // FIXME: The code depends on the context not going away once created, to prevent JS from >> // seeing a dangling pointer. So for now we will disallow the context from being changed" >> >> If I call 'setImageBuffer(nullptr)' here, JSC gets very upset that the image buffer went away without it's control. >> >> I can add a comment to that effect. > > Oh! Yes, your way works perfectly. Much better -- I'll change it. Wait -- this is still failing on the debug test bots due to the GC being angry about the ImageBuffer going missing. I'll switch back to my previous version and retry the test. Created attachment 266681 [details]
Patch
Created attachment 266686 [details]
Patch
Comment on attachment 266686 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=266686&action=review > Source/WebCore/html/HTMLCanvasElement.cpp:121 > + releaseImageBufferAndContext(); Maybe name this 'releaseImageBufferAndContextStateSaver"? (In reply to comment #22) > > Oh! Yes, your way works perfectly. Much better -- I'll change it. > > Wait -- this is still failing on the debug test bots due to the GC being > angry about the ImageBuffer going missing. I'll switch back to my previous > version and retry the test. Okay -- as we discussed in person, this was due to the graphics context state saver not being cleared when the ImageBuffer was deallocated. This is now fixed, and with that everything works properly. Comment on attachment 266686 [details]
Patch
Actually rubber-stamped by Simon Fraser.
Resetting the r+ based on Simon's earlier review (plus in-person discussion of final fix) to allow webkit-patch to do its thing. Committed r193500: <http://trac.webkit.org/changeset/193500> This change seems to be causing the following layout test to fail on El Capitan WK2 Debug: fast/canvas/canvas-too-large-to-draw.html <https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r193649%20(1513)/results.html> Re-opened since this is blocked by bug 152143 The test bots that were failing are 8 GB Mac Minis. This means that we set the upper limit of memory use to 2 GB. These large canvas tests use ~268 MB canvas sizes. Even if we made two of them that should only reach about 0.5 GB of memory use. I think the issue is that the canvas elements from previous test runs remain in the JSC heap until a garbage collection runs. This test case gets hit in the midst of a number of canvas tests, which consume enough “pixel memory” that we can’t allocate the gargantuan canvas and we fail the test. Wait. I can't do math. The tests try to allocate: (1) a 16384 x 16384 pixel canvas, which consumes (at 4 bytes per pixel) = 1,073,741,824 bytes. (2) a 16385 x 16384 pixel canvas, which consumes (at 4 bytes per pixel) = 1,073,807,360 bytes. On an 8 GB machine, we allow 2 GB of RAM to be used: 2,147,483,648 bytes. (1) + (2) = 2,147,549,184 which exceeds the allowed memory. I'll adjust the allowed memory minimum to handle this case. Created attachment 267638 [details]
Patch (Revised with memory limit bumped to satisfy test criteria)
Comment on attachment 267638 [details]
Patch (Revised with memory limit bumped to satisfy test criteria)
This is a small update to satisfy 'fast/canvas/canvas-stroke-path.html'. I'm uploading it for "review" so that I can get EWS confirmation on a wider range of hardware before I re-land.
Tests are green. I'm relanding the change. Committed r194290: <http://trac.webkit.org/changeset/194290> Comment on attachment 267638 [details]
Patch (Revised with memory limit bumped to satisfy test criteria)
Clearing review flag now that this is landed.
|