BenchmarkRunner test measurement refactoring#28
BenchmarkRunner test measurement refactoring#28camillobruni wants to merge 24 commits intoWebKit:mainfrom
Conversation
camillobruni
changed the title
resources/benchmark-runner.mjs
Outdated
| submit(options = NATIVE_OPTIONS) | ||
| _dispatchSubmitEvent() | ||
| { | ||
| // FIXME FireFox doesn't like `new Event('submit') |
There was a problem hiding this comment.
Looks like this comment about Firefox is no longer applicable.
We should probably just use dispatchEvent instead.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
Could you rebase this against the current main? |
|
Ups, I thought I did that already yesterday. |
camillobruni
force-pushed
the
2022-11-22_async_metric
branch
from
4173f00 to
5d28f74
Compare
| this._recordTestResults(suite, test, syncTime, asyncTime, test_done_callback); | ||
| }, 0); | ||
| } | ||
|
|
There was a problem hiding this comment.
Do we have before/after numbers among browsers for this change?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
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. |
|
We do already see the throttling down in some cases when running over slower networks with the Speedometer 2.1. 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? |
resources/benchmark-runner.mjs
Outdated
| }; | ||
|
|
||
| class RafBracketedCallbacks { | ||
| constructor(first_callback, second_callback) { |
resources/benchmark-runner.mjs
Outdated
| ], | ||
| }; | ||
|
|
||
| class RafBracketedCallbacks { |
There was a problem hiding this comment.
rAF should probably capitalized as rAF, not Raf.
resources/benchmark-runner.mjs
Outdated
| }); | ||
| } | ||
| if (this._asyncMetricMode === "timeout") | ||
| setTimeout(this._measureSync.bind(this), 0); |
There was a problem hiding this comment.
Why don't we wrap this in callbacks class like TimerCallbacks which takes two arguments like RafBracketedCallbacks.
resources/benchmark-runner.mjs
Outdated
| if (this._asyncMetricMode === "timeout") | ||
| setTimeout(this._measureAsyncTimeoutCallback, 0); | ||
| else | ||
| this._rafMeasurement.run(); |
There was a problem hiding this comment.
I'm confused. Why do we need to call run again here?
| constructor(firstCallback, secondCallback) { | ||
| this._firstCallback = firstCallback; | ||
| this._secondCallback = secondCallback; | ||
| this._setTimeoutCallback = this._setTimeout.bind(this); |
There was a problem hiding this comment.
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);)
| this._asyncMeasurePromise = new Promise((resolve) => { | ||
| this._asyncDoneCallback = resolve; | ||
| }); | ||
| this._rAFMeasurement = new RAFBracketedCallbacks(this._measureRafStart.bind(this), this._measureRafEnd.bind(this)); |
There was a problem hiding this comment.
Something here is rather confusing. Using RAFBracketedCallbacks in two places with different callbacks.
Why do we need this kind of setup?
rniwa
added
major change
| }; | ||
|
|
||
| class TimerCallbacks { | ||
| constructor(firstCallback, secondCallback) { |
There was a problem hiding this comment.
Call these as first & second are rather confusing.
We should probably call them as syncCallback & asyncCallback.
|
This is getting all so complicated to review. I think we should split this into two pieces.
|
Alright, I've gone ahead and made a PR for (1): #164 |
|
Sorry, just got around to read this after my vacation. |
5 participants
Uh oh!
There was an error while loading. Please reload this page.