Make SKP bench JSON ouput better

tools/PictureResultsWriter.h

Index: tools/PictureResultsWriter.h
diff --git a/tools/PictureResultsWriter.h b/tools/PictureResultsWriter.h
index d4a15765f0325415e6bfebdcb075fa1b0a727a37..a1cc6e72f20dbfe80b8664ccad483f3b1f4d451e 100644
--- a/tools/PictureResultsWriter.h
+++ b/tools/PictureResultsWriter.h
@@ -10,6 +10,8 @@
 #ifndef SkPictureResultsWriter_DEFINED
 #define SkPictureResultsWriter_DEFINED
 
+
+#include "PictureRenderer.h"
 #include "BenchLogger.h"
 #include "ResultsWriter.h"
 #include "SkJSONCPP.h"
@@ -18,6 +20,8 @@
 #include "SkTArray.h"
 #include "TimerData.h"
 
+using namespace sk_tools;

[jcgregorio, 2014/06/23 19:02:44]

I'm leery of blanket using statements like this, does this need to be done?

[kelvinly, 2014/06/23 20:32:46]

Done.

+
 /**
  * Base class for writing picture bench results.
  */
@@ -29,7 +33,7 @@ public:
     virtual ~PictureResultsWriter() {}
 
     virtual void bench(const char name[], int32_t x, int32_t y) = 0;
-    virtual void tileConfig(SkString configName) = 0;
+    virtual void logRenderer(PictureRenderer *pr) = 0;
     virtual void tileMeta(int x, int y, int tx, int ty) = 0;
     virtual void addTileFlag(PictureResultsWriter::TileFlags flag) = 0;
     virtual void tileData(
@@ -59,9 +63,9 @@ public:
             fWriters[i]->bench(name, x, y);
         }
     }
-    virtual void tileConfig(SkString configName) {
+    virtual void logRenderer(PictureRenderer *pr) {
         for(int i=0; i<fWriters.count(); ++i) {
-            fWriters[i]->tileConfig(configName);
+            fWriters[i]->logRenderer(pr);
         }
     }
     virtual void tileMeta(int x, int y, int tx, int ty) {
@@ -112,8 +116,8 @@ public:
         result.printf("running bench [%i %i] %s ", x, y, name);
         this->logProgress(result.c_str());
     }
-    virtual void tileConfig(SkString configName) {
-        currentLine = configName;
+    virtual void logRenderer(PictureRenderer* renderer) {
+        currentLine = renderer->getConfigName();
     }
     virtual void tileMeta(int x, int y, int tx, int ty) {
         currentLine.appendf(": tile [%i,%i] out of [%i,%i]", x, y, tx, ty);
@@ -172,61 +176,139 @@ private:
 
 class PictureJSONResultsWriter : public PictureResultsWriter {
 public:
-    PictureJSONResultsWriter(const char filename[])
-        : fFilename(filename),
-          fRoot(),
-          fCurrentBench(NULL),
-          fCurrentTileSet(NULL),
-          fCurrentTile(NULL) {}
+    PictureJSONResultsWriter(const char filename[], 
+            const char builderName[],

[jcgregorio, 2014/06/23 19:02:44]

Align with first param decl.

[kelvinly, 2014/06/23 20:32:46]

Done.

+            int buildNumber,
+            int timestamp,
+            const char gitHash[],
+            int gitNumber)
+        : fStream(filename),

[jcgregorio, 2014/06/23 19:02:44]

I believe this should be:

: fStream(filename)
, fParams()
, fConfigString() {

[kelvinly, 2014/06/23 20:32:46]

Won't compile the other way

[jcgregorio, 2014/06/23 20:40:44]

Sorry, wasn't commenting on the order, just the formatting. See the Skia coding
style for "constructor initializers"

https://sites.google.com/site/skiadocs/developer-documentation/contributing-c...



On 2014/06/23 20:32:46, kelvinly wrote:

[kelvinly, 2014/06/23 20:49:27]

Ah, my bad, fixed

+          fParams(),
+          fConfigString() {
+        const char keys[6][32] = {

[jcgregorio, 2014/06/23 19:02:44]

Why not const char *keys[6] = {"role", ...

Also, since this is const you can move it out of the function.

[kelvinly, 2014/06/23 20:32:46]

Moved this to a function in ResultsWriter.cpp, to be shared across both result
writers

+            "role", "os", "model", "gpu", "arch", "configuration"};
+        const int numKeys = 6;

[jcgregorio, 2014/06/23 19:02:44]

Is 6 just the size of keys? I'm pretty sure we have a macro for that.

[kelvinly, 2014/06/23 20:32:46]

Um, would you happen to know it off the top of your head? Do you guys have docs
somewhere? I've been grepping through the source and relying on the doxygen
pages

[jcgregorio, 2014/06/23 20:40:44]

Good question, I usually rely on grepping the source and doxygen also. Let's see
what Brian or Robert say.

On 2014/06/23 20:32:46, kelvinly wrote:

+        fBuilderName = SkString(builderName);
+        fBuildNumber = buildNumber;
+        fTimestamp = timestamp;
+        fGitHash = SkString(gitHash);
+        fGitNumber = gitNumber;
+        fBuilderData = Json::Value();
 
-    virtual void bench(const char name[], int32_t x, int32_t y) {
-        SkString sk_name(name);
-        sk_name.append("_");
-        sk_name.appendS32(x);
-        sk_name.append("_");
-        sk_name.appendS32(y);
-        Json::Value* bench_node = SkFindNamedNode(&fRoot["benches"], sk_name.c_str());
-        fCurrentBench = &(*bench_node)["tileSets"];
-    }
-    virtual void tileConfig(SkString configName) {
-        SkASSERT(fCurrentBench != NULL);
-        fCurrentTileSet = SkFindNamedNode(fCurrentBench, configName.c_str());
-        fCurrentTile = &(*fCurrentTileSet)["tiles"][0];
+        if(fBuilderName.size() > 0) {
+            SkTArray<SkString> splitBuilder;
+            SkStrSplit(builderName, "-", &splitBuilder);
+            for(int i = 0; i < numKeys && i < splitBuilder.count(); i++) {
+                fBuilderData[keys[i]] = splitBuilder[i].c_str();
+            }
+            fBuilderData["builderName"] = builderName;
+            if(numKeys < splitBuilder.count()) {
+                SkString extras;
+                for(int i = numKeys; i < splitBuilder.count(); i++) {
+                    extras.append(splitBuilder[i]);
+                    if(i != splitBuilder.count() - 1) {
+                        extras.append("-");
+                    }
+                }
+                fBuilderData["badParams"] = extras.c_str();
+            }
+        }
     }
-    virtual void tileMeta(int x, int y, int tx, int ty) {
-        SkASSERT(fCurrentTileSet != NULL);
-        (*fCurrentTileSet)["tx"] = tx;
-        (*fCurrentTileSet)["ty"] = ty;
-        fCurrentTile = &(*fCurrentTileSet)["tiles"][x+tx*y];
+
+    virtual void bench(const char name[], int32_t x, int32_t y) {
+        fBenchName = SkString(name);
     }
-    virtual void addTileFlag(PictureResultsWriter::TileFlags flag) {
-        SkASSERT(fCurrentTile != NULL);
-        if(flag == PictureResultsWriter::kPurging) {
-            (*fCurrentTile)["flags"]["purging"] = true;
-        } else if(flag == PictureResultsWriter::kAvg) {
-            (*fCurrentTile)["flags"]["averaged"] = true;
-        }
+    virtual void logRenderer(PictureRenderer* pr) {
+        fParams = pr->getJSONConfig();
+        fConfigString = pr->getConfigName();
     }
+    // Apparently tiles aren't used, so tileMeta is empty
+    virtual void tileMeta(int x, int y, int tx, int ty) { ; }
+    // Same here

[jcgregorio, 2014/06/23 19:02:44]

As the code changes "Same here" may make less sense, please make this a comment
that can stand on its own.

[kelvinly, 2014/06/23 20:32:46]

Done.

+    virtual void addTileFlag(PictureResultsWriter::TileFlags flag) { ; }
     virtual void tileData(
             TimerData* data,
             const char format[],
             const TimerData::Result result,
             uint32_t timerTypes,
             int numInnerLoops = 1) {
-        SkASSERT(fCurrentTile != NULL);
-        (*fCurrentTile)["data"] = data->getJSON(timerTypes, result, numInnerLoops);
+        Json::Value newData = data->getJSON(timerTypes, result, numInnerLoops);
+        Json::Value combinedParams(fBuilderData);
+        for(Json::ValueIterator iter = fParams.begin(); iter != fParams.end();
+                iter++) {
+            combinedParams[iter.key().asString()]= *iter;
+        }
+        // For each set of timer data
+        for(Json::ValueIterator iter = newData.begin(); iter != newData.end();
+                iter++) {
+            Json::Value data;
+            data["buildNumber"] = fBuildNumber;
+            data["timestamp"] = fTimestamp;
+            data["gitHash"] = fGitHash.c_str();
+            data["gitNumber"] = fGitNumber;
+            data["isTrybot"] = fBuilderName.endsWith("Trybot");
+
+            data["params"] = combinedParams;
+            data["params"]["benchName"] = fBenchName.c_str();
+
+            // Not including skpSize because that's deprecated?
+            data["key"] = makeKey(iter.key().asString().c_str()).c_str();
+            // Get the data
+            SkTArray<double> times;
+            Json::Value val = *iter;
+            for(Json::ValueIterator vals = val.begin(); vals != val.end();
+                    vals++) {
+                times.push_back((*vals).asDouble());
+            }
+            // Because there's no sort I'll just remove the smallest 

[jcgregorio, 2014/06/23 19:02:44]

qsort?

[kelvinly, 2014/06/23 20:32:46]

Can you get the raw array out of an SkTArray?

[jcgregorio, 2014/06/23 20:40:44]

begin()?

On 2014/06/23 20:32:46, kelvinly wrote:

[kelvinly, 2014/06/23 20:49:27]

I don't think we have a begin():
https://code.google.com/p/skia/source/browse/trunk/include/core/SkTArray.h?sp...

[jcgregorio, 2014/06/23 20:52:59]

Don't reference code.google.com, that's not canonical anymore, use
skia.googlesource.com or github:

 https://github.com/google/skia/blob/master/include/core/SkTArray.h#L273

On 2014/06/23 20:49:27, kelvinly wrote:

[kelvinly, 2014/06/23 21:10:48]

Fixed

+            // 25% of values
+            int numValues = times.count();
+            double currentMin = 10000000.0f;
+            for(int i = 0; i < numValues; i++) {
+                if(times[i] < currentMin) {
+                    currentMin = times[i];
+                }
+            }
+            for(int i = 0; i < numValues*0.25f; i++) {
+                double localMin = currentMin;
+                for(int j = 0; j < numValues; j++) {
+                    if(times[i] > currentMin && times[i] < localMin) {
+                        localMin = times[i];
+                    }
+                }
+                currentMin = localMin;
+            }
+            data["value"] = currentMin;
+            data["params"]["measurementType"] = iter.key().asString();
+            fStream.writeText(Json::FastWriter().write(data).c_str());
+        }
     }
     virtual void end() {
-       SkFILEWStream stream(fFilename.c_str());
-       stream.writeText(Json::FastWriter().write(fRoot).c_str());
-       stream.flush();
+       fStream.flush();
     }
 private:
-    SkString fFilename;
-    Json::Value fRoot;
-    Json::Value *fCurrentBench;
-    Json::Value *fCurrentTileSet;
-    Json::Value *fCurrentTile;
+    SkString makeKey(const char measurementType[]) {
+        SkString tmp(fBuilderName);
+        tmp.append("_");
+        tmp.append(fBenchName);
+        tmp.append("_");
+        tmp.append(fConfigString);
+        tmp.append("_");
+        tmp.append(measurementType);
+        return tmp;
+    }
+
+    SkFILEWStream fStream;
+    Json::Value fBuilderData;
+    SkString fBenchName;
+    Json::Value fParams;
+
+    SkString fConfigString;
+    SkString fBuilderName;
+    int fBuildNumber;
+    int fTimestamp;
+    SkString fGitHash;
+    int fGitNumber;
 };
 
 #endif