Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(35)

Issue 285943002: Revert of 4x allocation in PipeController is probably overkill. (Closed)

Created:
6 years, 7 months ago by mtklein
Modified:
6 years, 7 months ago
CC:
skia-review_googlegroups.com
Base URL:
https://skia.googlesource.com/skia.git@master
Visibility:
Public.

Description

Revert of 4x allocation in PipeController is probably overkill. (https://codereview.chromium.org/267863002/) Reason for revert: ~29% perf regression in Chrome's Animation_balls-canvas bench on Macs. Going to cut up and reland in pieces. https://code.google.com/p/chromium/issues/detail?id=372671 Original issue's description: > 4x allocation in PipeController is probably overkill. > > When verylargebitmap GM runs in cross-process pipe mode, we're > requestBlock()ing ~200M to carry the bitmaps. The current > implementation ends up allocating ~800M, which is a bit wasteful. > > SkGPipeWrite already rounds up to 16K, so just rely on that. > > This change exposed several bugs in pipe: > - we don't reserve enough space in drawVertices > - we don't reserve enough space for factory names in cross-process mode > - we don't quite have the right check in needOpBytes to see if we needed to send off the current block and allocate a new one > > SETUP_NOTIFY and generally calling doNotify() more often than necessary made things hard to debug and understand. Now the pipe always waits to send off its current block until it needs more space than that block can provide, or it's the final block. We can put these back if we need the proactive flushing, but it seems not necessary? > > Removed an assert in DeferredCanvasTest, which is somtimes 2 (Debug), sometimes 3 (Release). It seemed like the other asserts were more essential, and this one was more of a white-box assertion. Still sound if we remove it? > > BUG=skia:2478 > > Committed: http://code.google.com/p/skia/source/detail?r=14613 TBR=scroggo@google.com,reed@google.com,junov@chromium.org,mtklein@chromium.org NOTREECHECKS=true NOTRY=true BUG=skia:2478

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+79 lines, -58 lines) Patch
M src/pipe/SkGPipeWrite.cpp View 39 chunks +64 lines, -46 lines 0 comments Download
M src/pipe/utils/SamplePipeControllers.h View 1 chunk +3 lines, -3 lines 0 comments Download
M src/pipe/utils/SamplePipeControllers.cpp View 1 chunk +12 lines, -5 lines 0 comments Download
M tests/DeferredCanvasTest.cpp View 1 chunk +0 lines, -4 lines 0 comments Download

Messages

Total messages: 4 (0 generated)
mtklein
Created Revert of 4x allocation in PipeController is probably overkill.
6 years, 7 months ago (2014-05-14 13:33:01 UTC) #1
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/mtklein@google.com/285943002/1
6 years, 7 months ago (2014-05-14 13:33:09 UTC) #2
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 7 months ago (2014-05-14 13:33:12 UTC) #3
commit-bot: I haz the power
6 years, 7 months ago (2014-05-14 13:33:12 UTC) #4
Failed to apply patch for src/pipe/SkGPipeWrite.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file src/pipe/SkGPipeWrite.cpp
  Hunk #1 FAILED at 28.
  Hunk #39 FAILED at 1257.
  2 out of 39 hunks FAILED -- saving rejects to file
src/pipe/SkGPipeWrite.cpp.rej

Patch:       src/pipe/SkGPipeWrite.cpp
Index: src/pipe/SkGPipeWrite.cpp
diff --git a/src/pipe/SkGPipeWrite.cpp b/src/pipe/SkGPipeWrite.cpp
index
14caecd850798694232d0458f9c4d46e59c823f3..82848f8a6ff1af1aa7b81b6147f791c658bcb4b3
100644
--- a/src/pipe/SkGPipeWrite.cpp
+++ b/src/pipe/SkGPipeWrite.cpp
@@ -28,14 +28,6 @@
 #include "SkTSearch.h"
 #include "SkTypeface.h"
 #include "SkWriter32.h"
-
-#if SK_DEBUG
-// When debugging, allocate snuggly.
-static const size_t kMinBlockSize = 0;
-#else
-// For peformance, make large allocations.
-static const size_t kMinBlockSize = 16 * 1024;
-#endif
 
 enum {
     kSizeOfFlatRRect = sizeof(SkRect) + 4 * sizeof(SkVector)
@@ -358,6 +350,15 @@
     SkPaint fPaint;
     void writePaint(const SkPaint&);
 
+    class AutoPipeNotify {
+    public:
+        AutoPipeNotify(SkGPipeCanvas* canvas) : fCanvas(canvas) {}
+        ~AutoPipeNotify() { fCanvas->doNotify(); }
+    private:
+        SkGPipeCanvas* fCanvas;
+    };
+    friend class AutoPipeNotify;
+
     typedef SkCanvas INHERITED;
 };
 
@@ -365,7 +366,7 @@
     const char* name;
     while ((name = fFactorySet->getNextAddedFactoryName()) != NULL) {
         size_t len = strlen(name);
-        if (this->needOpBytes(SkWriter32::WriteStringSize(name, len))) {
+        if (this->needOpBytes(len)) {
             this->writeOp(kDef_Factory_DrawOp);
             fWriter.writeString(name, len);
         }
@@ -420,6 +421,7 @@
 
 ///////////////////////////////////////////////////////////////////////////////
 
+#define MIN_BLOCK_SIZE  (16 * 1024)
 #define BITMAPS_TO_KEEP 5
 #define FLATTENABLES_TO_KEEP 10
 
@@ -457,6 +459,7 @@
         }
     }
     fFlattenableHeap.setBitmapStorage(fBitmapHeap);
+    this->doNotify();
 }
 
 SkGPipeCanvas::~SkGPipeCanvas() {
@@ -471,13 +474,12 @@
     }
 
     needed += 4;  // size of DrawOp atom
-    needed = SkTMax(kMinBlockSize, needed);
-    needed = SkAlign4(needed);
     if (fWriter.bytesWritten() + needed > fBlockSize) {
-        // We're making a new block.  First push out the current block.
+        // Before we wipe out any data that has already been written, read it
+        // out.
         this->doNotify();
-
-        void* block = fController->requestBlock(needed, &fBlockSize);
+        size_t blockSize = SkTMax<size_t>(MIN_BLOCK_SIZE, needed);
+        void* block = fController->requestBlock(blockSize, &fBlockSize);
         if (NULL == block) {
             // Do not notify the readers, which would call this function again.
             this->finish(false);
@@ -508,7 +510,11 @@
 
 ///////////////////////////////////////////////////////////////////////////////
 
+#define NOTIFY_SETUP(canvas)    \
+    AutoPipeNotify apn(canvas)
+
 void SkGPipeCanvas::willSave(SaveFlags flags) {
+    NOTIFY_SETUP(this);
     if (this->needOpBytes()) {
         this->writeOp(kSave_DrawOp, 0, flags);
     }
@@ -518,6 +524,7 @@
 
 SkCanvas::SaveLayerStrategy SkGPipeCanvas::willSaveLayer(const SkRect* bounds,
const SkPaint* paint,
                                                          SaveFlags saveFlags) {
+    NOTIFY_SETUP(this);
     size_t size = 0;
     unsigned opFlags = 0;
 
@@ -547,6 +554,7 @@
 }
 
 void SkGPipeCanvas::willRestore() {
+    NOTIFY_SETUP(this);
     if (this->needOpBytes()) {
         this->writeOp(kRestore_DrawOp);
     }
@@ -587,6 +595,7 @@
 
 void SkGPipeCanvas::didConcat(const SkMatrix& matrix) {
     if (!matrix.isIdentity()) {
+        NOTIFY_SETUP(this);
         switch (matrix.getType()) {
             case SkMatrix::kTranslate_Mask:
                 this->recordTranslate(matrix);
@@ -604,6 +613,7 @@
 }
 
 void SkGPipeCanvas::didSetMatrix(const SkMatrix& matrix) {
+    NOTIFY_SETUP(this);
     if (this->needOpBytes(matrix.writeToMemory(NULL))) {
         this->writeOp(kSetMatrix_DrawOp);
         fWriter.writeMatrix(matrix);
@@ -613,6 +623,7 @@
 
 void SkGPipeCanvas::onClipRect(const SkRect& rect, SkRegion::Op rgnOp,
                                ClipEdgeStyle edgeStyle) {
+    NOTIFY_SETUP(this);
     if (this->needOpBytes(sizeof(SkRect))) {
         unsigned flags = 0;
         if (kSoft_ClipEdgeStyle == edgeStyle) {
@@ -626,6 +637,7 @@
 
 void SkGPipeCanvas::onClipRRect(const SkRRect& rrect, SkRegion::Op rgnOp,
                                 ClipEdgeStyle edgeStyle) {
+    NOTIFY_SETUP(this);
     if (this->needOpBytes(kSizeOfFlatRRect)) {
         unsigned flags = 0;
         if (kSoft_ClipEdgeStyle == edgeStyle) {
@@ -639,6 +651,7 @@
 
 void SkGPipeCanvas::onClipPath(const SkPath& path, SkRegion::Op rgnOp,
                                ClipEdgeStyle edgeStyle) {
+    NOTIFY_SETUP(this);
     if (this->needOpBytes(path.writeToMemory(NULL))) {
         unsigned flags = 0;
         if (kSoft_ClipEdgeStyle == edgeStyle) {
@@ -652,6 +665,7 @@
 }
 
 void SkGPipeCanvas::onClipRegion(const SkRegion& region, SkRegion::Op rgnOp) {
+    NOTIFY_SETUP(this);
     if (this->needOpBytes(region.writeToMemory(NULL))) {
         this->writeOp(kClipRegion_DrawOp, 0, rgnOp);
         fWriter.writeRegion(region);
@@ -662,6 +676,7 @@
 ///////////////////////////////////////////////////////////////////////////////
 
 void SkGPipeCanvas::clear(SkColor color) {
+    NOTIFY_SETUP(this);
     unsigned flags = 0;
     if (color) {
         flags |= kClear_HasColor_DrawOpFlag;
@@ -675,6 +690,7 @@
 }
 
 void SkGPipeCanvas::drawPaint(const SkPaint& paint) {
+    NOTIFY_SETUP(this);
     this->writePaint(paint);
     if (this->needOpBytes()) {
         this->writeOp(kDrawPaint_DrawOp);
@@ -684,6 +700,7 @@
 void SkGPipeCanvas::drawPoints(PointMode mode, size_t count,
                                const SkPoint pts[], const SkPaint& paint) {
     if (count) {
+        NOTIFY_SETUP(this);
         this->writePaint(paint);
         if (this->needOpBytes(4 + count * sizeof(SkPoint))) {
             this->writeOp(kDrawPoints_DrawOp, mode, 0);
@@ -694,6 +711,7 @@
 }
 
 void SkGPipeCanvas::drawOval(const SkRect& rect, const SkPaint& paint) {
+    NOTIFY_SETUP(this);
     this->writePaint(paint);
     if (this->needOpBytes(sizeof(SkRect))) {
         this->writeOp(kDrawOval_DrawOp);
@@ -702,6 +720,7 @@
 }
 
 void SkGPipeCanvas::drawRect(const SkRect& rect, const SkPaint& paint) {
+    NOTIFY_SETUP(this);
     this->writePaint(paint);
     if (this->needOpBytes(sizeof(SkRect))) {
         this->writeOp(kDrawRect_DrawOp);
@@ -710,6 +729,7 @@
 }
 
 void SkGPipeCanvas::drawRRect(const SkRRect& rrect, const SkPaint& paint) {
+    NOTIFY_SETUP(this);
     this->writePaint(paint);
     if (this->needOpBytes(kSizeOfFlatRRect)) {
         this->writeOp(kDrawRRect_DrawOp);
@@ -719,6 +739,7 @@
 
 void SkGPipeCanvas::onDrawDRRect(const SkRRect& outer, const SkRRect& inner,
                                  const SkPaint& paint) {
+    NOTIFY_SETUP(this);
     this->writePaint(paint);
     if (this->needOpBytes(kSizeOfFlatRRect * 2)) {
         this->writeOp(kDrawDRRect_DrawOp);
@@ -728,6 +749,7 @@
 }
 
 void SkGPipeCanvas::drawPath(const SkPath& path, const SkPaint& paint) {
+    NOTIFY_SETUP(this);
     this->writePaint(paint);
     if (this->needOpBytes(path.writeToMemory(NULL))) {
         this->writeOp(kDrawPath_DrawOp);
@@ -739,24 +761,16 @@
                                      unsigned flags,
                                      size_t opBytesNeeded,
                                      const SkPaint* paint) {
-    if (fDone) {
-        return false;
-    }
-
     if (paint != NULL) {
         flags |= kDrawBitmap_HasPaint_DrawOpFlag;
         this->writePaint(*paint);
     }
-
-    // This needs to run first so its calls to needOpBytes() and writes
-    // don't interlace with the needOpBytes() and writes below.
-    SkASSERT(fBitmapHeap != NULL);
-    int32_t bitmapIndex = fBitmapHeap->insert(bm);
-    if (SkBitmapHeap::INVALID_SLOT == bitmapIndex) {
-        return false;
-    }
-
     if (this->needOpBytes(opBytesNeeded)) {
+        SkASSERT(fBitmapHeap != NULL);
+        int32_t bitmapIndex = fBitmapHeap->insert(bm);
+        if (SkBitmapHeap::INVALID_SLOT == bitmapIndex) {
+            return false;
+        }
         this->writeOp(op, flags, bitmapIndex);
         return true;
     }
@@ -765,6 +779,7 @@
 
 void SkGPipeCanvas::drawBitmap(const SkBitmap& bm, SkScalar left, SkScalar top,
                                const SkPaint* paint) {
+    NOTIFY_SETUP(this);
     size_t opBytesNeeded = sizeof(SkScalar) * 2;
 
     if (this->commonDrawBitmap(bm, kDrawBitmap_DrawOp, 0, opBytesNeeded,
paint)) {
@@ -776,6 +791,7 @@
 void SkGPipeCanvas::drawBitmapRectToRect(const SkBitmap& bm, const SkRect* src,
                                          const SkRect& dst, const SkPaint*
paint,
                                          DrawBitmapRectFlags dbmrFlags) {
+    NOTIFY_SETUP(this);
     size_t opBytesNeeded = sizeof(SkRect);
     bool hasSrc = src != NULL;
     unsigned flags;
@@ -799,6 +815,7 @@
 
 void SkGPipeCanvas::drawBitmapMatrix(const SkBitmap& bm, const SkMatrix&
matrix,
                                      const SkPaint* paint) {
+    NOTIFY_SETUP(this);
     size_t opBytesNeeded = matrix.writeToMemory(NULL);
 
     if (this->commonDrawBitmap(bm, kDrawBitmapMatrix_DrawOp, 0, opBytesNeeded,
paint)) {
@@ -808,6 +825,7 @@
 
 void SkGPipeCanvas::drawBitmapNine(const SkBitmap& bm, const SkIRect& center,
                                    const SkRect& dst, const SkPaint* paint) {
+    NOTIFY_SETUP(this);
     size_t opBytesNeeded = sizeof(int32_t) * 4 + sizeof(SkRect);
 
     if (this->commonDrawBitmap(bm, kDrawBitmapNine_DrawOp, 0, opBytesNeeded,
paint)) {
@@ -821,6 +839,7 @@
 
 void SkGPipeCanvas::drawSprite(const SkBitmap& bm, int left, int top,
                                  …
(message too large)

Powered by Google App Engine
This is Rietveld 408576698