Bug 240605 - MotionMark's multiply suite reaches max complexity on faster devices
Summary: MotionMark's multiply suite reaches max complexity on faster devices
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-05-18 15:29 PDT by Scott Violet
Modified: 2022-06-23 18:01 PDT (History)
10 users (show)

See Also:


Attachments
Chrome running the multiply suite (1.20 MB, image/png)
2022-05-18 15:29 PDT, Scott Violet
no flags Details
Patch (1.21 KB, patch)
2022-05-18 16:01 PDT, Scott Violet
no flags Details | Formatted Diff | Diff
Patch (1.21 KB, patch)
2022-06-23 14:38 PDT, Scott Violet
no flags Details | Formatted Diff | Diff
Patch (1.46 KB, patch)
2022-06-23 15:51 PDT, Scott Violet
no flags Details | Formatted Diff | Diff
Patch (1.37 KB, patch)
2022-06-23 15:52 PDT, Scott Violet
no flags Details | Formatted Diff | Diff
Patch (1.36 KB, patch)
2022-06-23 15:52 PDT, Scott Violet
no flags Details | Formatted Diff | Diff
Patch (1.35 KB, patch)
2022-06-23 15:53 PDT, Scott Violet
no flags Details | Formatted Diff | Diff
Patch (1.39 KB, patch)
2022-06-23 15:54 PDT, Scott Violet
no flags Details | Formatted Diff | Diff
Patch (1.37 KB, patch)
2022-06-23 15:55 PDT, Scott Violet
no flags Details | Formatted Diff | Diff
Patch (1.37 KB, patch)
2022-06-23 15:55 PDT, Scott Violet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Scott Violet 2022-05-18 15:29:08 PDT
Created attachment 459557 [details]
Chrome running the multiply suite

On laptops the multiply suite has a maximum complexity of ~3100. STP 145 is pretty close to this as is Chrome. On recent MacBook Pros with ProMotion, Chrome receives an extremely low score (typically less than 10) even though it can generally handle the complexity (maintains a frame rate of 60hz). Chrome receives a low score because of two factors:

1. Chrome processes requestAnimationFrame at 120Hz.
2. The maximum complexity is not high enough.

These two result in Chrome getting a low score on multiply because the line fitting used to determine the score generally does not cross the 60hz line. In this case the scoring typically uses the first value seen, which is a complexity of around 1. I've attached a screenshot illustrating this. I'm happy to attach any other data that would be helpful (perhaps the JSON results).

All the other suites in MotionMark run long enough such that the line fitting uses a reasonable value.
Comment 1 Scott Violet 2022-05-18 16:01:34 PDT
Created attachment 459558 [details]
Patch
Comment 2 Scott Violet 2022-05-18 16:12:39 PDT
Change the description to better reflect the problem as there are numerous possible solutions.

I've attached a patch that increases the totalRows. totalRows directly influences the maximum complexity. In other words, this patch effectively increases the maximum complexity multiply runs to. This solution gives more headroom, and effectively fixes the issue. It may make sense to use a slightly higher number, possibly 60.

As the scoring generally assumes 60hz, another possibility is to use Math.max(1000/60, frame_rate) when determining frame-rate for scoring. This is a bit more invasive, but is another possibility. This has the result of smoothing out the frame rates, so it may have other side effects.

A final possibility is to run a number of initial frames to determine rAF, and then score based on how well that frame rate is maintained. This would mean you can not compare scores between Safari and Chrome or Firefox, which is not ideal.
Comment 3 Simon Fraser (smfr) 2022-05-19 12:02:22 PDT
Having MM respond to rAF frequency is something we plan, but out of scope for this particular bug.

The patch in the bug seems reasonable, though. To incorporate it into the public benchmark we'd need to rev the version number, in which case we might also want to consider other minor improvements to the benchmark.
Comment 4 Scott Violet 2022-05-19 12:18:48 PDT
Thanks for looking Simon. Can you suggest how we move forward and what the timeframe would be for reving the version?
Comment 5 Radar WebKit Bug Importer 2022-05-25 15:30:18 PDT
<rdar://problem/93931692>
Comment 6 EWS 2022-06-03 10:40:46 PDT
No reviewer information in commit message, blocking PR #None
Comment 7 Scott Violet 2022-06-21 10:30:52 PDT
Simon, any update as to how we can move forward here?
Comment 8 Simon Fraser (smfr) 2022-06-21 10:51:36 PDT
We can land this patch (the commit message just needs a "Reviewed By" line), but it will be some time before we update the public benchmark version.
Comment 9 Scott Violet 2022-06-23 14:38:00 PDT
Created attachment 460455 [details]
Patch
Comment 10 Scott Violet 2022-06-23 14:39:27 PDT
Sorry, I'm not familiar with the process. I uploaded a new patch (which should be exactly the same as the first). Please let me know if I need to do something.
Comment 11 Chris Dumez 2022-06-23 14:41:40 PDT
Comment on attachment 460455 [details]
Patch

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

> COMMIT_MESSAGE:8
> +https://bugs.webkit.org/show_bug.cgi?id=240605

I believe the format of your commit message is wrong. The bug URL should be on the line right under the bug title.
Also, you're missing the "Reviewed By Nobody" line and our commit hook should have added for you.
Comment 12 Scott Violet 2022-06-23 15:51:14 PDT
Created attachment 460459 [details]
Patch
Comment 13 Scott Violet 2022-06-23 15:52:00 PDT
Created attachment 460460 [details]
Patch
Comment 14 Scott Violet 2022-06-23 15:52:42 PDT
Created attachment 460462 [details]
Patch
Comment 15 Scott Violet 2022-06-23 15:53:40 PDT
Created attachment 460463 [details]
Patch
Comment 16 Scott Violet 2022-06-23 15:54:23 PDT
Created attachment 460464 [details]
Patch
Comment 17 Scott Violet 2022-06-23 15:55:09 PDT
Created attachment 460465 [details]
Patch
Comment 18 Scott Violet 2022-06-23 15:55:57 PDT
Created attachment 460466 [details]
Patch
Comment 19 Kenneth Russell 2022-06-23 15:57:41 PDT
We couldn't figure out how to get webkit-patch to reinitialize the commit message, so tried modifying the patch's commit message locally. Let me help Scott try to commit this.
Comment 20 Chris Dumez 2022-06-23 15:58:52 PDT
Comment on attachment 460466 [details]
Patch

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

> COMMIT_MESSAGE:2
> +

If it fails, it will be because of this blank line here.

> COMMIT_MESSAGE:3
> +

And this one.
Comment 21 Kenneth Russell 2022-06-23 16:10:52 PDT
Comment on attachment 460466 [details]
Patch

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

>> COMMIT_MESSAGE:3
>> +
> 
> And this one.

Yes - we had difficulty figuring out how to edit the patch locally (in mbox format) to produce a well-formed COMMIT_MESSAGE. Deleting the blank line between the subject and the bug URL completely eliminated the newline between the two in the COMMIT_MESSAGE produced by the review tool.

Hopefully this'll pass the CQ and then in future patches we'll use the Github PR workflow.
Comment 22 EWS 2022-06-23 17:10:02 PDT
Committed 251810@main (86daf4fcece9): <https://commits.webkit.org/251810@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 460466 [details].
Comment 23 Simon Fraser (smfr) 2022-06-23 18:01:37 PDT
Retitled to be more accurate (sorry I didn't do this before it landed).