Revert of Add MultiPictureDraw to nanobench (patchset google#7 id:120…

…001 of https://codereview.chromium.org/731973005/) Reason for revert: Needs more work Original issue's description: > Add MultiPictureDraw to nanobench > > I would like some guard against performance regressions on our side before turning layer hoisting on in Chromium. > > [email protected] > > Committed: https://skia.googlesource.com/skia/+/0ddad31012dabfc1267effc8071d37f7d606efbe [email protected],[email protected] NOTREECHECKS=true NOTRY=true Review URL: https://codereview.chromium.org/750583002
HistoryFork · Nov 21, 2014 · e77dadd · e77dadd
1 parent 0ddad31
commit e77dadd
Show file tree

Hide file tree

Showing 8 changed files with 42 additions and 175 deletions.
diff --git a/bench/Benchmark.cpp b/bench/Benchmark.cpp
@@ -37,14 +37,6 @@ void Benchmark::preDraw() {
     this->onPreDraw();
 }
 
-void Benchmark::perCanvasPreDraw(SkCanvas* canvas) {
-    this->onPerCanvasPreDraw(canvas);
-}
-
-void Benchmark::perCanvasPostDraw(SkCanvas* canvas) {
-    this->onPerCanvasPostDraw(canvas);
-}
-
 void Benchmark::draw(const int loops, SkCanvas* canvas) {
     SkAutoCanvasRestore ar(canvas, true/*save now*/);
     this->onDraw(loops, canvas);

diff --git a/bench/Benchmark.h b/bench/Benchmark.h
@@ -70,11 +70,6 @@ class Benchmark : public SkRefCnt {
     // before the initial draw.
     void preDraw();
 
-    // Called once before and after a series of draw calls to a single canvas.
-    // The setup/break down in these calls is not timed.
-    void perCanvasPreDraw(SkCanvas*);
-    void perCanvasPostDraw(SkCanvas*);
-
     // Bench framework can tune loops to be large enough for stable timing.
     void draw(const int loops, SkCanvas*);
 
@@ -106,8 +101,6 @@ class Benchmark : public SkRefCnt {
     virtual const char* onGetName() = 0;
     virtual const char* onGetUniqueName() { return this->onGetName(); }
     virtual void onPreDraw() {}
-    virtual void onPerCanvasPreDraw(SkCanvas*) {}
-    virtual void onPerCanvasPostDraw(SkCanvas*) {}
     // Each bench should do its main work in a loop like this:
     //   for (int i = 0; i < loops; i++) { <work here> }
     virtual void onDraw(const int loops, SkCanvas*) = 0;

diff --git a/bench/SKPBench.cpp b/bench/SKPBench.cpp
@@ -7,28 +7,15 @@
 
 #include "SKPBench.h"
 #include "SkCommandLineFlags.h"
-#include "SkMultiPictureDraw.h"
-#include "SkSurface.h"
 
 DEFINE_int32(benchTile, 256, "Tile dimension used for SKP playback.");
 
-SKPBench::SKPBench(const char* name, const SkPicture* pic, const SkIRect& clip, SkScalar scale,
-                   bool useMultiPictureDraw)
+SKPBench::SKPBench(const char* name, const SkPicture* pic, const SkIRect& clip, SkScalar scale)
     : fPic(SkRef(pic))
     , fClip(clip)
     , fScale(scale)
-    , fName(name)
-    , fUseMultiPictureDraw(useMultiPictureDraw) {
+    , fName(name) {
     fUniqueName.printf("%s_%.2g", name, scale);  // Scale makes this unqiue for skiaperf.com traces.
-    if (useMultiPictureDraw) {
-        fUniqueName.append("_mpd");
-    }
-}
-
-SKPBench::~SKPBench() {
-    for (int i = 0; i < fSurfaces.count(); ++i) {
-        fSurfaces[i]->unref();
-    }
 }
 
 const char* SKPBench::onGetName() {
@@ -39,49 +26,6 @@ const char* SKPBench::onGetUniqueName() {
     return fUniqueName.c_str();
 }
 
-void SKPBench::onPerCanvasPreDraw(SkCanvas* canvas) {
-    if (!fUseMultiPictureDraw) {
-        return;
-    }
-
-    SkIRect bounds;
-    SkAssertResult(canvas->getClipDeviceBounds(&bounds));
-
-    int xTiles = SkScalarCeilToInt(bounds.width()  / SkIntToScalar(FLAGS_benchTile));
-    int yTiles = SkScalarCeilToInt(bounds.height() / SkIntToScalar(FLAGS_benchTile));
-
-    fSurfaces.setReserve(xTiles * yTiles);
-    fTileRects.setReserve(xTiles * yTiles);
-
-    SkImageInfo ii = canvas->imageInfo().makeWH(FLAGS_benchTile, FLAGS_benchTile);
-
-    for (int y = bounds.fTop; y < bounds.fBottom; y += FLAGS_benchTile) {
-        for (int x = bounds.fLeft; x < bounds.fRight; x += FLAGS_benchTile) {
-            *fTileRects.append() = SkIRect::MakeXYWH(x, y, FLAGS_benchTile, FLAGS_benchTile);
-            *fSurfaces.push() = canvas->newSurface(ii);
-            fSurfaces.top()->getCanvas()->setMatrix(canvas->getTotalMatrix());
-            fSurfaces.top()->getCanvas()->scale(fScale, fScale);
-        }
-    }
-}
-
-void SKPBench::onPerCanvasPostDraw(SkCanvas* canvas) {
-    if (!fUseMultiPictureDraw) {
-        return;
-    }
-
-    // Draw the last set of tiles into the master canvas in case we're
-    // saving the images
-    for (int i = 0; i < fTileRects.count(); ++i) {
-        canvas->drawImage(fSurfaces[i]->newImageSnapshot(),
-                          SkIntToScalar(fTileRects[i].fLeft), SkIntToScalar(fTileRects[i].fTop));
-        SkSafeSetNull(fSurfaces[i]);
-    }
-
-    fSurfaces.rewind();
-    fTileRects.rewind();
-}
-
 bool SKPBench::isSuitableFor(Backend backend) {
     return backend != kNonRendering_Backend;
 }
@@ -91,41 +35,21 @@ SkIPoint SKPBench::onGetSize() {
 }
 
 void SKPBench::onDraw(const int loops, SkCanvas* canvas) {
-    if (fUseMultiPictureDraw) {
-        for (int i = 0; i < loops; i++) {
-            SkMultiPictureDraw mpd;
-
-            for (int i = 0; i < fTileRects.count(); ++i) {
-                SkMatrix trans;
-                trans.setTranslate(-fTileRects[i].fLeft/fScale,
-                                   -fTileRects[i].fTop/fScale);
-                mpd.add(fSurfaces[i]->getCanvas(), fPic, &trans);
-            }
-
-            mpd.draw();
-
-            for (int i = 0; i < fTileRects.count(); ++i) {
-                fSurfaces[i]->getCanvas()->flush();
-            }
-        }
-    } else {
-        SkIRect bounds;
-        SkAssertResult(canvas->getClipDeviceBounds(&bounds));
+    SkIRect bounds;
+    SkAssertResult(canvas->getClipDeviceBounds(&bounds));
 
-        SkAutoCanvasRestore overall(canvas, true/*save now*/);
-        canvas->scale(fScale, fScale);
+    SkAutoCanvasRestore overall(canvas, true/*save now*/);
+    canvas->scale(fScale, fScale);
 
-        for (int i = 0; i < loops; i++) {
-            for (int y = bounds.fTop; y < bounds.fBottom; y += FLAGS_benchTile) {
-                for (int x = bounds.fLeft; x < bounds.fRight; x += FLAGS_benchTile) {
-                    SkAutoCanvasRestore perTile(canvas, true/*save now*/);
-                    canvas->clipRect(SkRect::Make(
-                                SkIRect::MakeXYWH(x, y, FLAGS_benchTile, FLAGS_benchTile)));
-                    fPic->playback(canvas);
-                }
+    for (int i = 0; i < loops; i++) {
+        for (int y = bounds.fTop; y < bounds.fBottom; y += FLAGS_benchTile) {
+            for (int x = bounds.fLeft; x < bounds.fRight; x += FLAGS_benchTile) {
+                SkAutoCanvasRestore perTile(canvas, true/*save now*/);
+                canvas->clipRect(SkRect::Make(
+                            SkIRect::MakeXYWH(x, y, FLAGS_benchTile, FLAGS_benchTile)));
+                fPic->playback(canvas);
             }
-
-            canvas->flush();
         }
+        canvas->flush();
     }
 }
diff --git a/bench/SKPBench.h b/bench/SKPBench.h
@@ -17,18 +17,14 @@
  */
 class SKPBench : public Benchmark {
 public:
-    SKPBench(const char* name, const SkPicture*, const SkIRect& devClip, SkScalar scale,
-             bool useMultiPictureDraw);
-    ~SKPBench() SK_OVERRIDE;
+    SKPBench(const char* name, const SkPicture*, const SkIRect& devClip, SkScalar scale);
 
 protected:
-    const char* onGetName() SK_OVERRIDE;
-    const char* onGetUniqueName() SK_OVERRIDE;
-    void onPerCanvasPreDraw(SkCanvas*) SK_OVERRIDE;
-    void onPerCanvasPostDraw(SkCanvas*) SK_OVERRIDE;
-    bool isSuitableFor(Backend backend) SK_OVERRIDE;
-    void onDraw(const int loops, SkCanvas* canvas) SK_OVERRIDE;
-    SkIPoint onGetSize() SK_OVERRIDE;
+    virtual const char* onGetName() SK_OVERRIDE;
+    virtual const char* onGetUniqueName() SK_OVERRIDE;
+    virtual bool isSuitableFor(Backend backend) SK_OVERRIDE;
+    virtual void onDraw(const int loops, SkCanvas* canvas) SK_OVERRIDE;
+    virtual SkIPoint onGetSize() SK_OVERRIDE;
 
 private:
     SkAutoTUnref<const SkPicture> fPic;
@@ -37,10 +33,6 @@ class SKPBench : public Benchmark {
     SkString fName;
     SkString fUniqueName;
 
-    const bool fUseMultiPictureDraw;
-    SkTDArray<SkSurface*> fSurfaces;   // for MultiPictureDraw
-    SkTDArray<SkIRect> fTileRects;     // for MultiPictureDraw
-
     typedef Benchmark INHERITED;
 };
 

diff --git a/bench/nanobench.cpp b/bench/nanobench.cpp
@@ -26,7 +26,6 @@
 #include "SkPictureRecorder.h"
 #include "SkString.h"
 #include "SkSurface.h"
-#include "SkTaskGroup.h"
 
 #if SK_SUPPORT_GPU
     #include "gl/GrGLDefines.h"
@@ -71,7 +70,6 @@ DEFINE_int32(maxLoops, 1000000, "Never run a bench more times than this.");
 DEFINE_string(clip, "0,0,1000,1000", "Clip for SKPs.");
 DEFINE_string(scales, "1.0", "Space-separated scales for SKPs.");
 DEFINE_bool(bbh, true, "Build a BBH for SKPs?");
-DEFINE_bool(mpd, true, "Use MultiPictureDraw for the SKPs?");
 DEFINE_int32(flushEvery, 10, "Flush --outResultsFile every Nth run.");
 
 static SkString humanize(double ms) {
@@ -217,16 +215,14 @@ static int cpu_bench(const double overhead, Benchmark* bench, SkCanvas* canvas,
 }
 
 #if SK_SUPPORT_GPU
-static void setup_gl(SkGLContext* gl) {
-    gl->makeCurrent();
-    // Make sure we're done with whatever came before.
-    SK_GL(*gl, Finish());
-}
-
 static int gpu_bench(SkGLContext* gl,
                      Benchmark* bench,
                      SkCanvas* canvas,
                      double* samples) {
+    gl->makeCurrent();
+    // Make sure we're done with whatever came before.
+    SK_GL(*gl, Finish());
+
     // First, figure out how many loops it'll take to get a frame up to FLAGS_gpuMs.
     int loops = FLAGS_loops;
     if (kAutoTuneLoops == loops) {
@@ -441,8 +437,7 @@ class BenchmarkStream {
                       , fGMs(skiagm::GMRegistry::Head())
                       , fCurrentRecording(0)
                       , fCurrentScale(0)
-                      , fCurrentSKP(0)
-                      , fCurrentUseMPD(0) {
+                      , fCurrentSKP(0) {
         for (int i = 0; i < FLAGS_skps.count(); i++) {
             if (SkStrEndsWith(FLAGS_skps[i], ".skp")) {
                 fSKPs.push_back() = FLAGS_skps[i];
@@ -467,11 +462,6 @@ class BenchmarkStream {
                 exit(1);
             }
         }
-
-        fUseMPDs.push_back() = false;
-        if (FLAGS_mpd) {
-            fUseMPDs.push_back() = true;
-        }
     }
 
     static bool ReadPicture(const char* path, SkAutoTUnref<SkPicture>* pic) {
@@ -530,33 +520,25 @@ class BenchmarkStream {
         // Then once each for each scale as SKPBenches (playback).
         while (fCurrentScale < fScales.count()) {
             while (fCurrentSKP < fSKPs.count()) {
-                const SkString& path = fSKPs[fCurrentSKP];
+                const SkString& path = fSKPs[fCurrentSKP++];
                 SkAutoTUnref<SkPicture> pic;
                 if (!ReadPicture(path.c_str(), &pic)) {
-                    fCurrentSKP++;
                     continue;
                 }
-
-                while (fCurrentUseMPD < fUseMPDs.count()) {
-                    if (FLAGS_bbh) {
-                        // The SKP we read off disk doesn't have a BBH.  Re-record so it grows one.
-                        SkRTreeFactory factory;
-                        SkPictureRecorder recorder;
-                        static const int kFlags = SkPictureRecorder::kComputeSaveLayerInfo_RecordFlag;
-                        pic->playback(recorder.beginRecording(pic->cullRect().width(),
-                                                              pic->cullRect().height(),
-                                                              &factory, kFlags));
-                        pic.reset(recorder.endRecording());
-                    }
-                    SkString name = SkOSPath::Basename(path.c_str());
-                    fSourceType = "skp";
-                    fBenchType = "playback";
-                    return SkNEW_ARGS(SKPBench,
-                            (name.c_str(), pic.get(), fClip,
-                             fScales[fCurrentScale], fUseMPDs[fCurrentUseMPD++]));
+                if (FLAGS_bbh) {
+                    // The SKP we read off disk doesn't have a BBH.  Re-record so it grows one.
+                    SkRTreeFactory factory;
+                    SkPictureRecorder recorder;
+                    pic->playback(recorder.beginRecording(pic->cullRect().width(),
+                                                          pic->cullRect().height(),
+                                                          &factory));
+                    pic.reset(recorder.endRecording());
                 }
-                fCurrentUseMPD = 0;
-                fCurrentSKP++;
+                SkString name = SkOSPath::Basename(path.c_str());
+                fSourceType = "skp";
+                fBenchType  = "playback";
+                return SkNEW_ARGS(SKPBench,
+                        (name.c_str(), pic.get(), fClip, fScales[fCurrentScale]));
             }
             fCurrentSKP = 0;
             fCurrentScale++;
@@ -573,10 +555,6 @@ class BenchmarkStream {
                     SkStringPrintf("%d %d %d %d", fClip.fLeft, fClip.fTop,
                                                   fClip.fRight, fClip.fBottom).c_str());
             log->configOption("scale", SkStringPrintf("%.2g", fScales[fCurrentScale]).c_str());
-            if (fCurrentUseMPD > 0) {
-                SkASSERT(1 == fCurrentUseMPD || 2 == fCurrentUseMPD);
-                log->configOption("multi_picture_draw", fUseMPDs[fCurrentUseMPD-1] ? "true" : "false");
-            }
         }
     }
 
@@ -586,21 +564,18 @@ class BenchmarkStream {
     SkIRect            fClip;
     SkTArray<SkScalar> fScales;
     SkTArray<SkString> fSKPs;
-    SkTArray<bool>     fUseMPDs;
 
     const char* fSourceType;  // What we're benching: bench, GM, SKP, ...
     const char* fBenchType;   // How we bench it: micro, recording, playback, ...
     int fCurrentRecording;
     int fCurrentScale;
     int fCurrentSKP;
-    int fCurrentUseMPD;
 };
 
 int nanobench_main();
 int nanobench_main() {
     SetupCrashHandler();
     SkAutoGraphics ag;
-    SkTaskGroup::Enabler enabled;
 
 #if SK_SUPPORT_GPU
     GrContext::Options grContextOpts;
@@ -684,14 +659,6 @@ int nanobench_main() {
             SkCanvas* canvas = targets[j]->surface.get() ? targets[j]->surface->getCanvas() : NULL;
             const char* config = targets[j]->config.name;
 
-#if SK_SUPPORT_GPU
-            if (Benchmark::kGPU_Backend == targets[j]->config.backend) {
-                setup_gl(targets[j]->gl);
-            }
-#endif
-
-            bench->perCanvasPreDraw(canvas);
-
             const int loops =
 #if SK_SUPPORT_GPU
                 Benchmark::kGPU_Backend == targets[j]->config.backend
@@ -700,8 +667,6 @@ int nanobench_main() {
 #endif
                  cpu_bench(       overhead, bench.get(), canvas, samples.get());
 
-            bench->perCanvasPostDraw(canvas);
-
             if (canvas && !FLAGS_writePath.isEmpty() && FLAGS_writePath[0]) {
                 SkString pngFilename = SkOSPath::Join(FLAGS_writePath[0], config);
                 pngFilename = SkOSPath::Join(pngFilename.c_str(), bench->getUniqueName());

diff --git a/include/core/SkColorTable.h b/include/core/SkColorTable.h
@@ -65,7 +65,7 @@ class SK_API SkColorTable : public SkRefCnt {
     */
     void unlock16BitCache() {
         SkASSERT(f16BitCacheLockCount > 0);
-        SkDEBUGCODE(sk_atomic_dec(&f16BitCacheLockCount);)
+        SkDEBUGCODE(f16BitCacheLockCount -= 1);
     }
 
     explicit SkColorTable(SkReadBuffer&);

diff --git a/src/core/SkBitmap.cpp b/src/core/SkBitmap.cpp
@@ -1323,6 +1323,7 @@ void SkBitmap::validate() const {
 
     if (fPixels) {
         SkASSERT(fPixelRef);
+        SkASSERT(fPixelLockCount > 0);
         SkASSERT(fPixelRef->isLocked());
         SkASSERT(fPixelRef->rowBytes() == fRowBytes);
         SkASSERT(fPixelRefOrigin.fX >= 0);

diff --git a/src/core/SkColorTable.cpp b/src/core/SkColorTable.cpp
@@ -70,7 +70,7 @@ const uint16_t* SkColorTable::lock16BitCache() {
         build_16bitcache(f16BitCache, fColors, fCount);
     }
 
-    SkDEBUGCODE(sk_atomic_inc(&f16BitCacheLockCount));
+    SkDEBUGCODE(f16BitCacheLockCount += 1);
     return f16BitCache;
 }