BenchmarkRunner test measurement refactoring by camillobruni · Pull Request #28 · WebKit/Speedometer

camillobruni · 2022-11-22T22:00:41Z

Added separate MeasureTask for measuring
Added params.asyncMetric to experiment with the existing "timeout"-based and new "raf"-based approach

rniwa · 2022-11-23T08:19:18Z

-    submit(options = NATIVE_OPTIONS)
+    _dispatchSubmitEvent()
    {
-        // FIXME FireFox doesn't like `new Event('submit')


Looks like this comment about Firefox is no longer applicable.
We should probably just use dispatchEvent instead.

Which version are you using? FF 107.0 (64-bit) still get's stuck on AngularJS-TodoMVC if I don't use the custom event.

That is surprising when the c++ code for new Event literally calls initEvent
https://searchfox.org/mozilla-central/rev/670e2e0999f04dc7734c8c12b2c3d420a1e31f12/dom/events/Event.cpp#365-367

That is surprising when the c++ code for new Event literally calls initEvent https://searchfox.org/mozilla-central/rev/670e2e0999f04dc7734c8c12b2c3d420a1e31f12/dom/events/Event.cpp#365-367

I agree it's surprising, though I observe the same behavior on Nightly.

rniwa · 2022-12-01T22:08:26Z

Could you rebase this against the current main?

camillobruni · 2022-12-01T22:40:26Z

Ups, I thought I did that already yesterday.

rniwa · 2022-12-01T23:56:59Z

+            this._recordTestResults(suite, test, syncTime, asyncTime, test_done_callback);
        }, 0);
    }



Do we have before/after numbers among browsers for this change?

I've finally had time to run this change against the previous version. I do see a 1% to 2% slowdown. I will have to investigate and see whether I can repro this a bit better to pinpoint the main culprit.

https://docs.google.com/spreadsheets/d/1siPnknDnBh8mx9KJEL_MQrA5OufXUO5cOOX3eDRGBzE/edit?usp=sharing&resourcekey=0-tyHRwrlR0h5Ga0p9xjjYxw

Have we gotten around to do any analysis on this?

- Pre-allocate all mark-label strings to avoid potential gc's during sensitive measurements - Move code to new separate `_recordTestResults` method in preparation for pr #28

rniwa · 2023-04-11T21:55:33Z

We had some internal discussion about this, and we're concerned that the proposed change will make CPU throttle back the frequency too much. In practice, this will result in a paradoxical effect of the slower a software is, the faster CPU gets (because longer running task will tend to keep CPUs busier for a longer period of time and therefore keeps it at a higher frequency). We could work around this problem by warming up CPU with some workload but then that's pretty artificial as well and may have other adverse side effects like triggering thermal throttling of CPU.

The current measurement methodology of Speedometer has an advantage that it lets faster browser keep running at a high CPU frequency after finishing preceding workloads. In effect, it lets us measure the throughput of the browser engine, and not the latency for CPU to ramp up.

camillobruni · 2023-04-19T16:55:16Z

We do already see the throttling down in some cases when running over slower networks with the Speedometer 2.1.
So this might be an issue worth addressing.. I'm not too fond of warming up CPUs, maybe we do need some minor cool-down period after loading the resources?

Given that we do need some way to measure async performance, would you be up for landing just the preparatory refactoring without the addditional RAF-based metric for now?

rniwa · 2023-05-16T23:06:40Z

 };

+class RafBracketedCallbacks {
+    constructor(first_callback, second_callback) {


Please use camelCase.

rniwa · 2023-05-16T23:07:41Z

    ],
 };

+class RafBracketedCallbacks {


rAF should probably capitalized as rAF, not Raf.

rniwa · 2023-05-16T23:11:12Z

+            });
        }
+        if (this._asyncMetricMode === "timeout")
+            setTimeout(this._measureSync.bind(this), 0);


Why don't we wrap this in callbacks class like TimerCallbacks which takes two arguments like RafBracketedCallbacks.

rniwa · 2023-05-16T23:11:26Z

        if (this._asyncMetricMode === "timeout")
            setTimeout(this._measureAsyncTimeoutCallback, 0);
+        else
+            this._rafMeasurement.run();


I'm confused. Why do we need to call run again here?

bgrins · 2023-05-17T13:07:05Z

+    constructor(firstCallback, secondCallback) {
+        this._firstCallback = firstCallback;
+        this._secondCallback = secondCallback;
+        this._setTimeoutCallback = this._setTimeout.bind(this);


Nit: I'd find it easier to follow these classes if property name was always _setTimeoutCallback both in the class declaration and in this statement reassigning to the bound function (this._setTimeoutCallback = this._setTimeoutCallback.bind(this);)

smaug---- · 2023-05-17T13:12:35Z

+        this._asyncMeasurePromise = new Promise((resolve) => {
+            this._asyncDoneCallback = resolve;
+        });
+        this._rAFMeasurement = new RAFBracketedCallbacks(this._measureRafStart.bind(this), this._measureRafEnd.bind(this));


Something here is rather confusing. Using RAFBracketedCallbacks in two places with different callbacks.
Why do we need this kind of setup?

rniwa · 2023-05-19T01:47:20Z

 };

+class TimerCallbacks {
+    constructor(firstCallback, secondCallback) {


Call these as first & second are rather confusing.
We should probably call them as syncCallback & asyncCallback.

rniwa · 2023-05-19T01:49:24Z

This is getting all so complicated to review. I think we should split this into two pieces.

Refactoring of callbacks
Addition of rAF-based measurement

rniwa · 2023-05-19T06:46:18Z

This is getting all so complicated to review. I think we should split this into two pieces.

Refactoring of callbacks

Addition of rAF-based measurement

Alright, I've gone ahead and made a PR for (1): #164

camillobruni · 2023-05-23T15:11:29Z

Sorry, just got around to read this after my vacation.
Happy to split this into separate parts.

rniwa · 2023-05-25T02:00:57Z

We've made the equivalent changes in #173 and #164

camillobruni commented Nov 22, 2022

View reviewed changes