Clean up the classes that handle run results, and correctly import JSON files with multiple runs #77

smfr · 2024-09-16T23:54:16Z

This series of commits clean up ResultsDashboard (now ScoreCalculator), and adds RunData.

It also adds unit tests for data import.

Fix a bug where we failed to pass the event to a the “benchmark-options” radio changed handler.

Add test-data-import.js which tests creating a `ResultsDashboard` with imported JSON, which is the JSON produced by running the test via the browser. The test validates that the results analysis produces output data in the correct format, but does not validate the score calculation. To make the unit test run faster, we make `bootstrapIterations` be configurable via an option. The unit test sets this to a small value. Fix a bug that prevented the JSON results dialog being shown by using arrow functions in `BenchmarkController` so that `this` is correctly bound.

`BenchmarkRunnerClient.results` becomes `BenchmarkRunnerClient.scoreCalculator`. Add a public getter for `targetFrameRate`.

This class encapsulates the tuple of options, version and run data. ScoreCalculator can then store this in `runData`, replacing the separate storage of `options`, `version` and the confusingly named `_iterationsSamplers`.

rniwa · 2024-09-16T23:58:01Z

MotionMark/resources/debug-runner/debug-runner.js

@@ -704,38 +691,38 @@ class DebugBenchmarkController extends BenchmarkController {
            this.addedKeyEvent = true;
        }

-        var dashboard = this.runnerClient.results;
-        if (dashboard.options["controller"] == "ramp")
+        var scoreCalculator = this.runnerClient.scoreCalculator;


const works.

rniwa · 2024-09-16T23:58:17Z

MotionMark/resources/debug-runner/debug-runner.js

-            "% / +" + ((dashboard.scoreUpperBound / score - 1) * 100).toFixed(2) + "%";
-        var fps = dashboard._systemFrameRate;
-        sectionsManager.setSectionVersion("results", dashboard.version);
+        var score = scoreCalculator.score;


why not const or let for these?

rniwa · 2024-09-17T00:01:53Z

MotionMark/resources/runner/results.js

+        RunData.#migrateImportedData(singleRunData);
+
+        if (!singleRunData.data instanceof Array) {
+            console.log('Imported singleRunData.data is not an array. Bailing');


Why not console.error?

rniwa · 2024-09-17T00:02:07Z

MotionMark/resources/runner/results.js

+    static resultsDataFromBenchmarkRunnerData(benchmarkData)
+    {
+        if (!benchmarkData instanceof Array) {
+            console.log('Imported benchmarkData is not an array. Bailing');


Ditto and ditto for the rest of console.log in this file.

rniwa · 2024-09-17T00:02:17Z

MotionMark/resources/runner/results.js

+        }
+
+        let runData = [];
+        for (let run of benchmarkData) {


Use const instead?

rniwa · 2024-09-17T00:02:36Z

MotionMark/resources/runner/results.js

+class ScoreCalculator {
+    constructor(runData)
+    {
+        this._runData = runData;


Why not use private member variables?

The whole codebase needs a switch to private variables. For now I was following existing code.

rniwa · 2024-09-17T00:03:54Z

MotionMark/resources/runner/results.js

+    constructor(element, headers)
+    {
+        this.element = element;
+        this._headers = headers;


Why not use a private member variable?

rniwa · 2024-09-17T00:04:06Z

MotionMark/resources/runner/results.js

+        this.element = element;
+        this._headers = headers;
+
+        this._flattenedHeaders = [];


rniwa · 2024-09-17T00:04:10Z

MotionMark/resources/runner/results.js

+        this._headers = headers;
+
+        this._flattenedHeaders = [];
+        this._headers.forEach(function(header) {


rniwa · 2024-09-17T00:06:11Z

MotionMark/resources/runner/results.js

+        function findRegression(series, profile) {
+            var minIndex = Math.round(.025 * series.length);
+            var maxIndex = Math.round(.975 * (series.length - 1));
+            var minComplexity = series.getFieldInDatum(minIndex, Strings.json.complexity);


Instead of keep accessing Strings.json.complexity over and over, can we just store in a local const variable and use that?

rniwa · 2024-09-17T00:06:38Z

MotionMark/resources/runner/results.js

+
+        // Convert these samples into SampleData objects if needed
+        [Strings.json.complexity, Strings.json.controller].forEach(function(seriesName) {
+            var series = samples[seriesName];


rniwa · 2024-09-17T00:06:53Z

MotionMark/resources/runner/results.js

+        const desiredFrameLength = 1000 / this._targetFrameRate;
+
+        function findRegression(series, profile) {
+            var minIndex = Math.round(.025 * series.length);


It seems like these could all be const. If not, let?

rniwa · 2025-02-03T23:47:35Z

MotionMark/resources/strings.js

@@ -60,6 +60,7 @@ var Strings = {
        scoreUpperBound: "scoreUpperBound",
        fps: "fps",
        bootstrap: "bootstrap",
+        bootstrapIterations: "bootstrapIterations",


Is this side table really useful? It seems like we can just hard-code these strings where it's used.

I don't really know why this strings table was used. I agree it's unwieldy.

rniwa · 2025-02-03T23:53:00Z

MotionMark/resources/runner/results.js

+
+    _addHeader()
+    {
+        var thead = Utilities.createElement("thead", {}, this.element);


Why not use const/let here as well?

rniwa · 2025-02-03T23:53:12Z

MotionMark/resources/runner/results.js

+
+    _addEmptyRow()
+    {
+        var row = Utilities.createElement("tr", {}, this.tbody);


Why not use const/let here as well?

rniwa · 2025-02-03T23:53:23Z

MotionMark/resources/runner/results.js

+
+    _addTest(testName, testResult, options)
+    {
+        var row = Utilities.createElement("tr", {}, this.tbody);


Why not use const/let here as well?

rniwa · 2025-02-03T23:53:34Z

MotionMark/resources/runner/results.js

+
+    _addIteration(iterationResult, iterationData, options)
+    {
+        var testsResults = iterationResult[Strings.json.results.tests];


Why not use const/let here as well?

rniwa · 2025-02-03T23:53:59Z

MotionMark/unit-tests/tests/test-data-import.js

@@ -0,0 +1,137 @@
+/*
+ * Copyright (C) 2024 Apple Inc. All rights reserved.


rniwa · 2025-02-03T23:55:20Z

MotionMark/unit-tests/tests/test-data-import.js

+        // Check that we have results, not their exact values.
+        const firstResults = this.results[0];
+
+        expect(typeof firstResults[Strings.json.score]).to.be('number');


Can we specify some small range for the actual value?

When importing a file which is the result of a benchmark run with multiple iterations (which contains a `debugOutput` key), store all the runs in `RunData`. Add static methods to `RunData` to handle both types of JSON, and move the data migration code there. The score computation already correctly computed the arithmetic mean of the runs. The code already correctly built the results table for multiple runs.

smfr added 4 commits September 16, 2024 10:28

Move ResultsDashboard and ResultTable to a new file.

4cbe356

Fix a bug where we failed to pass the event to a the “benchmark-options” radio changed handler.

Rename ResultsDashboard to ScoreCalculator

3a9e3f2

`BenchmarkRunnerClient.results` becomes `BenchmarkRunnerClient.scoreCalculator`. Add a public getter for `targetFrameRate`.

Make a RunData class

55a6bcc

This class encapsulates the tuple of options, version and run data. ScoreCalculator can then store this in `runData`, replacing the separate storage of `options`, `version` and the confusingly named `_iterationsSamplers`.

smfr requested review from shallawa and rniwa September 16, 2024 23:54

rniwa reviewed Sep 16, 2024

View reviewed changes

rniwa reviewed Sep 17, 2024

View reviewed changes

MotionMark/resources/runner/results.js

this.element = element;

this._headers = headers;

this._flattenedHeaders = [];

Copy link

Member

rniwa Sep 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

rniwa reviewed Sep 17, 2024

View reviewed changes

smfr force-pushed the handle-multiple-iteration-data branch from 13dce51 to dd5fe97 Compare February 3, 2025 22:35

rniwa reviewed Feb 3, 2025

View reviewed changes

MotionMark/unit-tests/tests/test-data-import.js

@@ -0,0 +1,137 @@

/*

* Copyright (C) 2024 Apple Inc. All rights reserved.

Copy link

Member

rniwa Feb 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2024-2025?

rniwa reviewed Feb 3, 2025

View reviewed changes

rniwa approved these changes Feb 3, 2025

View reviewed changes

smfr force-pushed the handle-multiple-iteration-data branch from dd5fe97 to 94f4fa7 Compare February 5, 2025 23:01

smfr merged commit fc28b62 into WebKit:main Feb 5, 2025

smfr deleted the handle-multiple-iteration-data branch February 5, 2025 23:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up the classes that handle run results, and correctly import JSON files with multiple runs #77

Clean up the classes that handle run results, and correctly import JSON files with multiple runs #77

smfr commented Sep 16, 2024

rniwa Sep 16, 2024

smfr Feb 3, 2025

rniwa Sep 16, 2024

rniwa Sep 17, 2024

rniwa Sep 17, 2024

rniwa Sep 17, 2024

rniwa Sep 17, 2024

smfr Sep 17, 2024

rniwa Sep 17, 2024

rniwa Sep 17, 2024

rniwa Sep 17, 2024

rniwa Sep 17, 2024

rniwa Sep 17, 2024

rniwa Sep 17, 2024

rniwa Feb 3, 2025 •

edited

Loading

smfr Feb 4, 2025

rniwa Feb 3, 2025

rniwa Feb 3, 2025

rniwa Feb 3, 2025

rniwa Feb 3, 2025

rniwa Feb 3, 2025

rniwa Feb 3, 2025

		@@ -0,0 +1,137 @@
		/*
		* Copyright (C) 2024 Apple Inc. All rights reserved.

Clean up the classes that handle run results, and correctly import JSON files with multiple runs #77

Clean up the classes that handle run results, and correctly import JSON files with multiple runs #77

Conversation

smfr commented Sep 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rniwa Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rniwa Feb 3, 2025 •

edited

Loading