Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[windows] use angle fast path on d3d11 backend #31830

Merged
merged 1 commit into from
Mar 7, 2022

Conversation

jonahwilliams
Copy link
Member

Experimentally this makes things render faster on my end.

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@jonahwilliams
Copy link
Member Author

ideally the test for something like this would be a benchmark

@dnfield
Copy link
Contributor

dnfield commented Mar 4, 2022

I agree on the benchmark. It'd be good to attach a timeline here of what we're working on to see what it looks like now.

This change LGTM - we don't have a good way to verify if or how this attribute gets used by angle short of benchmarking it. @zanderso WDYT?

Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zanderso
Copy link
Member

zanderso commented Mar 4, 2022

Would any of our existing benchmarks show a benefit from this change if we were running them on Windows in CI and sending the results to SkiaPerf?

@dnfield
Copy link
Contributor

dnfield commented Mar 4, 2022

Jonah's going to attach a trace, but raster times should get better on any Windows benchmark with this patch.

@jonahwilliams
Copy link
Member Author

Timeline: dart-timeline-2022-3-4 (1).zip

@jonahwilliams
Copy link
Member Author

I think it is still worthwhile to write a test that grabs the extensions to make sure this is enabled correctly - but I don't know if that would actually pass on a bot unless that bot supports D3D11.

@zanderso
Copy link
Member

zanderso commented Mar 4, 2022

@godofredoc do you know if the Windows bots have d3d11?

@jonahwilliams
Copy link
Member Author

I guess in that sense, its not really a unit test given that it is hardware dependent

@zanderso
Copy link
Member

zanderso commented Mar 4, 2022

It looks like there's no devicelab support for running benchmarks on Windows desktop https://github.com/flutter/flutter/blob/master/dev/devicelab/lib/framework/devices.dart#L55. I wonder if we have an issue for that? @cbracken

@dnfield
Copy link
Contributor

dnfield commented Mar 4, 2022

I don't think there's any device support for running on Desktop period.

@gspencergoog I think worked on adding an integration test for desktop at some point though, he may know more.

@godofredoc
Copy link
Contributor

@godofredoc do you know if the Windows bots have d3d11?

Not sure, we are re-using chrome images. We can create a test and giving it a try.

@godofredoc
Copy link
Contributor

It looks like there's no devicelab support for running benchmarks on Windows desktop https://github.com/flutter/flutter/blob/master/dev/devicelab/lib/framework/devices.dart#L55. I wonder if we have an issue for that? @cbracken

From the bots point of view the benchmark can use windows-android bot.

@jonahwilliams
Copy link
Member Author

Do those bots have visual studio and other windows dependencies installed?

@godofredoc
Copy link
Contributor

Do those bots have visual studio and other windows dependencies installed?

Visual studio is installed at runtime during the task execution, not sure what other dependencies are needed but once we identify them we can install them.

@jonahwilliams
Copy link
Member Author

Seeing if I can create a benchmark here: flutter/flutter#99564

@godofredoc
Copy link
Contributor

Seeing if I can create a benchmark here: flutter/flutter#99564

@keyonghan can you please help craft a led command and properties file to test flutter/flutter#99564 in a devicelab bot?

@keyonghan
Copy link
Contributor

Seeing if I can create a benchmark here: flutter/flutter#99564

@keyonghan can you please help craft a led command and properties file to test flutter/flutter#99564 in a devicelab bot?

Here is an LED run based on the above PR: https://luci-milo.appspot.com/raw/build/logs.chromium.org/flutter/led/keyonghan_google.com/a2959aaf43d320eeb3e5800d0f3dc1c63a4431d38439b1e8c9e630a017cba20f/+/build.proto, it failed from the very beginning when resolving depencies:

stdout: [  +88 ms] Running "flutter pub get" in flutter_gallery...
[windows_home_scroll_perf__timeline_summary] [STDOUT] stdout: [   +5 ms] executing: [C:\b\s\w\ir\x\w\recipe_cleanup\tmpaorsxrej\flutter sdk\dev\integration_tests\flutter_gallery/] C:\b\s\w\ir\x\w\recipe_cleanup\tmpaorsxrej\flutter sdk\bin\cache\dart-sdk\bin\dart __deprecated_pub --trace --verbose get --no-precompile
[windows_home_scroll_perf__timeline_summary] [STDOUT] stderr: [ +123 ms] FINE: Pub 2.17.0-169.0.dev
[windows_home_scroll_perf__timeline_summary] [STDOUT] stdout: [  +94 ms] MSG : Resolving dependencies...
[windows_home_scroll_perf__timeline_summary] [STDOUT] stderr: [  +30 ms] SLVR: fact: flutter_gallery is 0.0.0
[windows_home_scroll_perf__timeline_summary] [STDOUT] stderr: [   +7 ms] SLVR: derived: flutter_gallery
[windows_home_scroll_perf__timeline_summary] [STDOUT] stderr: [  +45 ms] SLVR: fact: flutter_gallery depends on flutter from sdk
[windows_home_scroll_perf__timeline_summary] [STDOUT] stderr: [        ] SLVR: fact: flutter_gallery depends on collection 1.15.0
[windows_home_scroll_perf__timeline_summary] [STDOUT] stderr: [        ] SLVR: fact: flutter_gallery depends on device_info 2.0.3
[windows_home_scroll_perf__timeline_summary] [STDOUT] stderr: [        ] SLVR: fact: flutter_gallery depends on intl 0.17.0
[windows_home_scroll_perf__timeline_summary] [STDOUT] stderr: [        ] SLVR: fact: flutter_gallery depends on connectivity 3.0.6

@jonahwilliams
Copy link
Member Author

Ahh, its trying to screenshot - I can fix that

@jonahwilliams
Copy link
Member Author

I updated the PR so it should be able to get further

@keyonghan
Copy link
Contributor

I updated the PR so it should be able to get further

Thanks for the quick fix. It passed through: https://luci-milo.appspot.com/raw/build/logs.chromium.org/flutter/led/keyonghan_google.com/fcd53b2134e182a9566e74c487a0ca45a3f580516a53a462fb78775d7ad25bc0/+/build.proto

@jonahwilliams
Copy link
Member Author

Nice, thank you for the help @keyonghan . I guess the next step is to merge that to get at least one benchmark running, then get a test exception to land this and see if it helps the numbers

@jonahwilliams
Copy link
Member Author

@christopherfujino
Copy link
Member

test-exempt: this will be tested by the benchmark in flutter/flutter#99564

@jonahwilliams jonahwilliams added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Mar 7, 2022
@fluttergithubbot fluttergithubbot merged commit 46dc713 into flutter:main Mar 7, 2022
@jonahwilliams jonahwilliams deleted the egl_swap_vertical branch March 7, 2022 18:17
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs tests platform-windows waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants