Improve performance testing #20802
Improve performance testing #20802
Conversation
sgomes
added
[Type] Performance
sgomes
requested review from
ajitbohra,
nerrad,
ntwb and
talldan
as code owners
|
Size Change: +139 B (0%) Total Size: 864 kB
ℹ️ View Unchanged
|
|
Run the tests a few times, the numbers seem consistent which is a good sign.
We may want to round that average number. |
Added a commit applying rounding to the performance reporter. |
| item.dur && | ||
| item.args && | ||
| item.args.data && | ||
| item.args.data.type === 'keypress'; |
There was a problem hiding this comment.
I'm not sure that's enough, I know previously keydown was also a problem.
You can see in the screenshot that there's aa keydown and then a keypress. Both of these are gathered in a "Task". and after that there are "follow up async tasks).
Ideally, if we can meansure the three segments it would be great, I believe the most important thing is the sync behavior (keydown + keypress). A format like that might be best keydown+keypress ( follow-ups: xx ) eg: 25ms ( follow-ups: 40ms )
What do you think?
There was a problem hiding this comment.
I don't think it will make much difference, since I expect the absolute values not to be as important as their evolution over time, and the keydown/keyup events don't seem to have a great deal of variability.
That said, for completeness sake, I added a commit where I calculate the sum of the three event durations for a total value for each virtual key press.
There was a problem hiding this comment.
I just noticed that you'd suggested separating out the values of the different event types in the output. Do you think that's necessary, or is the sum enough?
There was a problem hiding this comment.
That sum is enough for the first part (the synchronous work). I don't know though if it's possible somehow to compute the time spent in the asynchronous work. In the trace it's shown as separate from keydown, keyup, keypress event, it's shown as multiple small tasks.
I'd say, getting that value is less important but if we manage to get it, we shouldn't sum it with the rest.
does that make sense?
There was a problem hiding this comment.
I'd argue that whether or not the async value is important depends on what you're trying to measure. If you're trying to measure input latency as perceived by the user, then the async value isn't important, as that work happens after the screen has updated and probably does not limit interactivity in any way, as the tasks are very short.
In any case, I suspect it would be extremely difficult to get that value, as there wouldn't be a good way of correlating the async work with the key events that triggered it.
|
Thanks @sgomes This is great. |
Description
Performance testing for Gutenberg was being done using puppeteer, by instrumenting a browser instance, loading a page, and performing some typing. However, these times were being measured across the instrumented browser / host context switch; this meant that the numbers were potentially unreliable, since they were subject to unpredictable factors such as OS scheduling.
This PR performs the measurements within the instrumented browser instance, by relying on browser performance timings for the
load/DOMContentLoadednumbers, and by using in-browser tracing for the typing event duration numbers.The results won't be directly comparable to the previous way of testing, but they should be comparable between each other henceforth, assuming equivalent testing conditions (same machine, similar system load, etc.).
How has this been tested?
The intermediate trace files were loaded in Chrome's performance tab, and it was experimentally verified that the numbers in the generated
results.jsonmatched the duration of the events visible in the DevTools timeline.Types of changes
Test-only bugfixes.