Reland "Reland: [macOS] Use CVDisplayLink to drive repaint" #51210
Conversation
|
|
||
| // Warm up frame should be presented as soon as possible. | ||
| EXPECT_TRUE(fabs(entries[0].timestamp - now) < 0.005); | ||
| EXPECT_TRUE(fabs(entries[0].targetTimestamp - now) < 0.005); |
There was a problem hiding this comment.
Potentially worth extracting a named constant, but with only two cases, it's likely not really worth it. We probably have precedent for naming such a thing, ideally with a better name than kTimingWiggleRoom.
There was a problem hiding this comment.
Hold off for a little bit on the reland while we see if we can verify frame timings on the bot manually. We're getting conflicting info on the refresh rate for their bot pool, and need to go through Chrome Ops to verify the machines in the pool the affected test runs on. Will reply as soon as I've got confirmation.
We could maybe add conditional logging (i.e. enabled through Info.plist) to see what ticks we're getting from CVDisplayLink. |
|
Or even just a "test" that always passes but collects ~10 frame intervals and logs min, average, max? |
I like that. Added FlutterDisplayLinkTest.CVDisplayLinkInterval. |
knopp
force-pushed
the
reland_displaylink_2
branch
from
de74281
to
6ef81fe
Compare
knopp
changed the title
|
Setting as WIP until I figure out what's going on with |
knopp
changed the title
|
Update: This is currently blocked on Infra. I've provided a test application which should help us determine whether the regression is caused by the PR or runner. |
There was a problem hiding this comment.
From what I've heard from @cbracken , this makes a big difference locally, and modulo issues with the Dart/Flutter clock I think we can land it. The benchmarks we have in the devicelab I have less faith in. Given that they are mac minis with no screen, we could have all sorts of issues there. And locking to 60fps is terrible
So LGTM
|
Yep, as mentioned on Discord, I think we should re-land this. I don't trust our benchmarking on this one at all and looking into it is hugely time-consuming given the back and forth between us, our infra team, and Chrome's (who owns the bots). I'm fine with us disabling the benchmark (or possibly re-baselining it though not sure how useful that even is). |
|
I think I'd just let it regress from now, ask folks to try it out and see. Possibly the apps are getting treated as "backgrounded" by the OS and so they are correctly running at 30fps. Or maybe its just cursed. |
|
The clock issue is fixed in this PR. It properly converts |
knopp
changed the title
…lutter#51126)" (flutter#51192)" This reverts commit 78cecee.
Using __block with non copyable objects should not have compiled, but yet it did for unique PTR for some reason. But just in case use shared_ptr instead.
knopp
force-pushed
the
reland_displaylink_2
branch
from
98ce68b
to
22e117f
Compare
knopp
added
the
waiting for tree to go green
…145108) flutter/engine@9551b49...c889344 2024-03-13 skia-flutter-autoroll@skia.org Roll Skia from 1ad0d0fabe7a to a315e4572f4e (7 revisions) (flutter/engine#51377) 2024-03-13 matej.knopp@gmail.com Reland "Reland: [macOS] Use CVDisplayLink to drive repaint" (flutter/engine#51210) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC bdero@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
The macOS devicelab machines are not physically configured for benchmarking at a 60 Hz refresh rate. With Flutter vsync wired up via CVDisplayLink (rather than a hardcoded 60 Hz rate as previously), the bot shows 0 monitors configured and syncs at 30 Hz. This is not a meaningful target to measure against but consumes devicelab CPU cycles. Given that there is currently no plan in place to acquire and configure a consistent pool of physical benchmarking desktop machines, disabling this test to reduce load on the bots. See flutter/engine#51210 for a discussion of the decision to move forward with wiring up vsync from macOS and ignore this benchmark.
The macOS devicelab machines are not physically configured for benchmarking at a 60 Hz refresh rate. With Flutter vsync wired up via CVDisplayLink (rather than a hardcoded 60 Hz rate as previously), the bot shows 0 monitors configured and syncs at 30 Hz. This is not a meaningful target to measure against but consumes devicelab CPU cycles. Given that there is currently no plan in place to acquire and configure a consistent pool of physical benchmarking desktop machines, disabling this test to reduce load on the bots. See flutter/engine#51210 for a discussion of the decision to move forward with wiring up vsync from macOS and ignore this benchmark. For future archaeologists, I don't think it's worth removing all the plumbing in the Dart code (in [dev/devicelab/lib/tasks/perf_tests.dart](https://github.com/flutter/flutter/blob/9d47055640a90879cfb45b2da55283504bedc354/dev/devicelab/lib/tasks/perf_tests.dart#L963)) that supports running this benchmark on macOS. If we ever acquire hardware suitable for benchmarking with the appropriate HDMI dongle to simulate a 60Hz 4k display, for example, it would be reasonable to re-enable this benchmark.
Original reland was reverted because of skiaperf regressions. The regressions are caused by runner machines running display at 30hz.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.