AuroraMiddleware/skia2
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Fix way-over-allocation in pipe.

Browse Source 
https://codereview.chromium.org/283093002 fixed some bugs in pipe memory
allocation, but also introduced one of its own: nearly every block requested
from needOpBytes() got its own 16K allocation.

The correct logic is to take the requested size, add four more bytes for a
DrawOp, make sure that's 4-byte aligned, then check to see if there's enough
space for that in the current block.  If there's not, allocate at least
MIN_BLOCK_SIZE bytes to fit the request.

The bug is that I moved that round-up-to-MIN_BLOCK_SIZE step before checking
for space in the current block.  This means most (all?) blocks will be 16K but
never seem to have room to fit another allocation.  You need 8 bytes?  You get
16K.  You need 8 more bytes?  Nope, can't fit that.  Here's a new 16K...

This reverts the change to the test I made then, which really should have
tipped me off.  It was testing exactly this bug.

BUG=372671
R=tomhudson@chromium.org, tomhudson@google.com

Author: mtklein@chromium.org

Review URL: https://codereview.chromium.org/433463003
This commit is contained in:
mtklein 2014-07-30 09:17:54 -07:00 committed by Commit bot
parent 1723bfc982
commit 6d88e6ce51
2 changed files with 7 additions and 5 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
src/pipe/SkGPipeWrite.cpp
View File

@ -475,12 +475,14 @@ bool SkGPipeCanvas::needOpBytes(size_t needed) {
}
needed += 4; // size of DrawOp atom
needed = SkTMax<size_t>(MIN_BLOCK_SIZE, needed);
needed = SkAlign4(needed);
if (fWriter.bytesWritten() + needed > fBlockSize) {
// Before we wipe out any data that has already been written, read it
// out.
// Before we wipe out any data that has already been written, read it out.
this->doNotify();
// If we're going to allocate a new block, allocate enough to make it worthwhile.
needed = SkTMax<size_t>(MIN_BLOCK_SIZE, needed);
void* block = fController->requestBlock(needed, &fBlockSize);
if (NULL == block) {
// Do not notify the readers, which would call this function again.

4
tests/DeferredCanvasTest.cpp
View File

@ -532,7 +532,7 @@ static void TestDeferredCanvasBitmapCaching(skiatest::Reporter* reporter) {
canvas->drawBitmap(sourceImages[0], 0, 0, NULL);
REPORTER_ASSERT(reporter, 2 == notificationCounter.fStorageAllocatedChangedCount);
canvas->drawBitmap(sourceImages[0], 0, 0, NULL);
REPORTER_ASSERT(reporter, 3 == notificationCounter.fStorageAllocatedChangedCount);
REPORTER_ASSERT(reporter, 2 == notificationCounter.fStorageAllocatedChangedCount);
REPORTER_ASSERT(reporter, 1 == notificationCounter.fFlushedDrawCommandsCount);
REPORTER_ASSERT(reporter, canvas->storageAllocatedForRecording() < 2 * bitmapSize);
@ -790,7 +790,7 @@ static void TestDeferredCanvasSetSurface(skiatest::Reporter* reporter, GrContext
}
surface = SkSurface::NewRenderTarget(context, imageSpec);
alternateSurface = SkSurface::NewRenderTarget(context, imageSpec);
} else
} else
#endif
{
surface = SkSurface::NewRaster(imageSpec);