Created attachment 453463 [details]
Patch

<rdar://problem/89444976>

Comment on attachment 453463 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453463&action=review

> PerformanceTests/Speedometer/resources/benchmark-runner.js:149
> +        window.requestAnimationFrame(function () {

Is this part of the change really needed? This part would impact the score and it is therefore unfortunate. I would have hoped that the outer requestAnimationFrame would have been enough to avoid reaching the max nesting limit and cause throttling.

Comment on attachment 453463 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453463&action=review

>> PerformanceTests/Speedometer/resources/benchmark-runner.js:149
>> +        window.requestAnimationFrame(function () {
> 
> Is this part of the change really needed? This part would impact the score and it is therefore unfortunate. I would have hoped that the outer requestAnimationFrame would have been enough to avoid reaching the max nesting limit and cause throttling.

This is necessary to reset nesting level, and
1. This affects on the score, but just because we reset nesting level, and that's the intent of this change.
2. window.requestAnimationFrame itself is not included in the measurement since it is done outside of measurement (see sync-time duration and async-time duration. this call is not included).

(In reply to Yusuke Suzuki from comment #4)
> Comment on attachment 453463 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453463&action=review
> 
> >> PerformanceTests/Speedometer/resources/benchmark-runner.js:149
> >> +        window.requestAnimationFrame(function () {
> > 
> > Is this part of the change really needed? This part would impact the score and it is therefore unfortunate. I would have hoped that the outer requestAnimationFrame would have been enough to avoid reaching the max nesting limit and cause throttling.
> 
> This is necessary to reset nesting level, and
> 1. This affects on the score, but just because we reset nesting level, and
> that's the intent of this change.
> 2. window.requestAnimationFrame itself is not included in the measurement
> since it is done outside of measurement (see sync-time duration and
> async-time duration. this call is not included).

sync-time region and async-time region

Comment on attachment 453463 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453463&action=review

> PerformanceTests/Speedometer/resources/benchmark-runner.js:146
>          var endTime = now();

async test's measurement is finished at this point. So, window.requestAnimationFrame is unrelated to the measurement except for reseting the nesting level.

Comment on attachment 453463 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453463&action=review

r=me

I think Yusuke resolved Chris's comment; please correct me if not.

> PerformanceTests/Speedometer/resources/benchmark-runner.js:57
> +        if (element) {
> +            window.requestAnimationFrame(function () {
> +                return promise.resolve(element);
> +            });
> +            return;
> +        }

The change to _runTest (below) ensures that each test ends with zero timer nesting level.

I think the purpose of this change is to ensure that each test also begins with zero timer nesting level?

Seems like a reasonable solution. But I'm curious, to help with my own mental model: Did you discover a specific case where just one of these changes or the other alone was insufficient?

(In reply to Geoffrey Garen from comment #7)
> Comment on attachment 453463 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453463&action=review
> 
> r=me
> 
> I think Yusuke resolved Chris's comment; please correct me if not.

Yes. All good.

Comment on attachment 453463 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453463&action=review

Thanks!

>> PerformanceTests/Speedometer/resources/benchmark-runner.js:57
>> +        }
> 
> The change to _runTest (below) ensures that each test ends with zero timer nesting level.
> 
> I think the purpose of this change is to ensure that each test also begins with zero timer nesting level?
> 
> Seems like a reasonable solution. But I'm curious, to help with my own mental model: Did you discover a specific case where just one of these changes or the other alone was insufficient?

I think waitForElement can be used because it is not inside _runTest (this is helper function waitForElement), so even if it is not used (I think currently it is always used), this change can ensure that setTimeout of this function will not affect the following test suite's run.

Committed r290664 (247936@trunk): <https://commits.webkit.org/247936@trunk>