Cleanup timing state machine

tools/VisualBench/TimingStateMachine.cpp

 /*
  * Copyright 2015 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "TimingStateMachine.h"
 
 #include "SkCanvas.h"
 #include "SkCommandLineFlags.h"
 
 DEFINE_int32(gpuFrameLag, 5, "Overestimate of maximum number of frames GPU is allowed to lag.");
 DEFINE_int32(frames, 5, "Number of frames of each skp to render per sample.");
 DEFINE_double(loopMs, 5, "Each benchmark will be tuned until it takes loopsMs millseconds.");
 

[robertphillips, 2015/10/05 18:37:37]

Why isn't preWarmBetweenSamples a member variable ?

 TimingStateMachine::TimingStateMachine()
     : fCurrentFrame(0)
     , fLoops(1)
     , fLastMeasurement(0.)
-    , fState(kPreWarmLoopsPerCanvasPreDraw_State) {
+    , fState(kTuning_State)
+    , fInnerState(kPreWarm_InnerState) {
 }
 
-TimingStateMachine::ParentEvents TimingStateMachine::nextFrame(SkCanvas* canvas,
-                                                               Benchmark* benchmark) {
-    switch (fState) {
-        case kPreWarmLoopsPerCanvasPreDraw_State:
-            return this->perCanvasPreDraw(canvas, benchmark, kPreWarmLoops_State);
-        case kPreWarmLoops_State:
-            return this->preWarm(kTuneLoops_State);
-        case kTuneLoops_State:
-            return this->tuneLoops();
-        case kPreWarmTimingPerCanvasPreDraw_State:
-            return this->perCanvasPreDraw(canvas, benchmark, kPreWarmTiming_State);
-        case kPreWarmTiming_State:
-            return this->preWarm(kTiming_State);
-        case kTiming_State:
-            return this->timing(canvas, benchmark);
+TimingStateMachine::ParentEvents TimingStateMachine::nextFrame(bool preWarmBetweenSamples) {
+    ParentEvents parentEvent = kTiming_ParentEvents;
+    switch (fInnerState) {
+        case kPreWarm_InnerState: {
+            if (fCurrentFrame >= FLAGS_gpuFrameLag) {
+                fCurrentFrame = 0;
+                fTimer.start();

[robertphillips, 2015/10/05 18:37:37]

Why not just set fInnerState to kTiming_InnerState here ?

+                this->nextState(preWarmBetweenSamples);
+            } else {
+                fCurrentFrame++;
+            }
+            break;
+        }
+        case kTiming_InnerState: {
+            switch (fState) {
+                case kTuning_State: {
+                    if (1 << 30 == fLoops) {
+                        // We're about to wrap.  Something's wrong with the bench.
+                        SkDebugf("InnerLoops wrapped\n");
+                        fLoops = 1;
+                    } else {
+                        double elapsedMs = this->elapsed();
+                        if (elapsedMs < FLAGS_loopMs) {
+                            fLoops *= 2;
+                        } else {
+                            this->nextState(preWarmBetweenSamples);
+                        }
+                        this->resetTimingState();
+                        parentEvent = kReset_ParentEvents;
+                    }
+                }
+                case kTiming_State: {
+                    if (fCurrentFrame >= FLAGS_frames) {
+                        this->recordMeasurement();
+                        this->resetTimingState();
+                        parentEvent = kTimingFinished_ParentEvents;
+                        this->nextState(preWarmBetweenSamples);
+                    } else {
+                        fCurrentFrame++;
+                    }
+                }
+            }
+        }
     }
-    SkFAIL("Incomplete switch\n");
-    return kTiming_ParentEvents;
+    return parentEvent;
 }
 

[robertphillips, 2015/10/05 18:37:37]

Would it be cleaner to fold this into nextFrame (not sure - really asking) ?

-inline void TimingStateMachine::nextState(State nextState) {
-    fState = nextState;
-}
-
-TimingStateMachine::ParentEvents TimingStateMachine::perCanvasPreDraw(SkCanvas* canvas,
-                                                                      Benchmark* benchmark,
-                                                                      State nextState) {
-    benchmark->perCanvasPreDraw(canvas);
-    benchmark->preDraw(canvas);
-    fCurrentFrame = 0;
-    this->nextState(nextState);
-    return kTiming_ParentEvents;
-}
-
-TimingStateMachine::ParentEvents TimingStateMachine::preWarm(State nextState) {
-    if (fCurrentFrame >= FLAGS_gpuFrameLag) {
-        // we currently time across all frames to make sure we capture all GPU work
-        this->nextState(nextState);
-        fCurrentFrame = 0;
-        fTimer.start();
-    } else {
-        fCurrentFrame++;
+inline void TimingStateMachine::nextState(bool preWarmBetweenSamples) {
+    switch (fInnerState) {
+        case kPreWarm_InnerState: {
+            fInnerState = kTiming_InnerState;
+            break;
+        }
+        case kTiming_InnerState: {
+            switch (fState) {
+                case kTuning_State: {
+                    fState = kTiming_State;
+                    fInnerState = kPreWarm_InnerState;
+                    break;
+                }
+                case kTiming_State: {
+                    if (preWarmBetweenSamples) {
+                        fInnerState = kPreWarm_InnerState;
+                    } else {
+                        fTimer.start(); // start timing again, don't change state
+                    }
+                }
+            }
+        }
     }
-    return kTiming_ParentEvents;
 }
 
 inline double TimingStateMachine::elapsed() {
     fTimer.end();
     return fTimer.fWall;
 }
 
 void TimingStateMachine::resetTimingState() {
     fCurrentFrame = 0;
     fTimer = WallTimer();
 }
 
-inline TimingStateMachine::ParentEvents TimingStateMachine::tuneLoops() {
-    if (1 << 30 == fLoops) {
-        // We're about to wrap.  Something's wrong with the bench.
-        SkDebugf("InnerLoops wrapped\n");
-        fLoops = 1;
-        return kTiming_ParentEvents;
-    } else {
-        double elapsedMs = this->elapsed();
-        if (elapsedMs > FLAGS_loopMs) {
-            this->nextState(kPreWarmTimingPerCanvasPreDraw_State);
-        } else {
-            fLoops *= 2;
-            this->nextState(kPreWarmLoops_State);
-        }
-        this->resetTimingState();
-        return kReset_ParentEvents;
-    }
-}
-
 void TimingStateMachine::recordMeasurement() {
     fLastMeasurement = this->elapsed() / (FLAGS_frames * fLoops);
 }
 
-inline TimingStateMachine::ParentEvents TimingStateMachine::timing(SkCanvas* canvas,
-                                                                   Benchmark* benchmark) {
-    if (fCurrentFrame >= FLAGS_frames) {
-        this->recordMeasurement();
-        this->resetTimingState();
-        return kTimingFinished_ParentEvents;
-    } else {
-        fCurrentFrame++;
-        return kTiming_ParentEvents;
-    }
-}
-
 void TimingStateMachine::nextBenchmark(SkCanvas* canvas, Benchmark* benchmark) {
     benchmark->postDraw(canvas);
     benchmark->perCanvasPostDraw(canvas);
     fLoops = 1;
-    this->nextState(kPreWarmLoopsPerCanvasPreDraw_State);
+    fState = kTuning_State;
+    fInnerState = kPreWarm_InnerState;
 }
-
-void TimingStateMachine::nextSampleWithPrewarm() {
-    this->nextState(kPreWarmTimingPerCanvasPreDraw_State);
-}
-
-void TimingStateMachine::nextSample() {
-    fTimer.start();
-}