commit 6d88e6ce51ff34c99a1135161f06f29322f58e5a
author mtklein <mtklein@chromium.org> Wed Jul 30 09:17:54 2014 -0700
committer Commit bot <commit-bot@chromium.org> Wed Jul 30 09:17:54 2014 -0700
tree 0744eb17309d6baf07c812022e3bbe7feca1e83b
parent 1723bfc98274c6d8d4159f139fc5eb59d8908987

Fix way-over-allocation in pipe.

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

src/pipe/SkGPipeWrite.cpp

diff --git a/src/pipe/SkGPipeWrite.cpp b/src/pipe/SkGPipeWrite.cpp
index 6711eb0..46e4260f 100644
--- a/src/pipe/SkGPipeWrite.cpp
+++ b/src/pipe/SkGPipeWrite.cpp
@@ -475,12 +475,14 @@
     }
 
     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.

tests/DeferredCanvasTest.cpp

diff --git a/tests/DeferredCanvasTest.cpp b/tests/DeferredCanvasTest.cpp
index b960fe3..b8b82b2 100644
--- a/tests/DeferredCanvasTest.cpp
+++ b/tests/DeferredCanvasTest.cpp
@@ -532,7 +532,7 @@
     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 @@
             }
             surface = SkSurface::NewRenderTarget(context, imageSpec);
             alternateSurface = SkSurface::NewRenderTarget(context, imageSpec);
-        } else 
+        } else
 #endif
         {
             surface = SkSurface::NewRaster(imageSpec);