Add measurePrepare param #440
Conversation
camillobruni
commented
Oct 23, 2024
camillobruni
commented
- Include the suite prepare time in total time if param.debugMetrics is set
- Add SuiteRunner._suiteResults local variable
|
What is the purpose of this change? Why do we ever want to include the prepare time in the total? |
|
This is useful for debugging and internal testing. We sometimes run the Speedometer workloads internally including the startup time with a live network. |
There was a problem hiding this comment.
Thanks for implementing my request!
To be honest I find the patch difficult to follow, because you've put a lot of things together in the same commit: how you deal with measuredValues, the checkbox UI, the prepare phase record. It would be good if you can split it in several meaningful commits. (no need to have separate PR).
Also I would like that we remove the first iteration when computing the average of the Prepare phase, because it's so different. What do you think?
resources/benchmark-runner.mjs
Outdated
| if (params.debugMetrics) | ||
| this._recordPrepareMetric(entry.duration); |
There was a problem hiding this comment.
I would prefer to always record it, but never add it to total. And then I'm not sure we need the parameter debugMetrics anymore?
resources/developer-mode.mjs
Outdated
|
|
||
| let label = document.createElement("label"); | ||
| label.append(check, " ", span("Use Warmup Suite")); | ||
| function createCheckboxUI(labelValue, initialValue) { |
There was a problem hiding this comment.
You could add the callback as a parameter and add it in this function. This would make the callers a lot leaner.
Also it would be good to use an option parameter { labelValue, initialValue, onChange } for a better readability at the caller site.
|
To simplify discussions on this PR, I'm splitting off the refactoring parts into separate PRs. |
…10-23_total_metric
| this.#suiteResults.tests[test.name] = { tests: { Sync: syncTime, Async: asyncTime }, total: total }; | ||
| let total = syncTime + asyncTime; | ||
| if (this.#params.measurePrepare) | ||
| total += this.#prepareTime; |
There was a problem hiding this comment.
I think if we did this, we should probably refuse to compute/show the final score. It's going to be very misleading otherwise. We don't want people to start reporting score with prepare time included as some kind of Speedometer score variant.
