-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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
Perf test with SkSL warm-up #56638
Perf test with SkSL warm-up #56638
Conversation
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. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
@jonahwilliams this test is now ready for review! It exercises all the tools that we've built so far and measures a ~2x speedup on the worst frame rasterization time. I also look forward to the iOS tool so we can achieve similar speedup in our ios32 benchmarks! |
'-t', testTarget, | ||
'--use-application-binary', '$testDirectory/build/app/outputs/flutter-apk/app-profile.apk', | ||
// Getting Observatory URI from --vmservice-out-file is flaky as we're | ||
// not sure when the file is written. Hence we |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this missing the rest of the sentence
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed and replaced with --vmservice-out-file
_forwardStream(broadcastOut, 'run stdout'); | ||
_forwardStream(_runProcess.stderr, 'run stderr'); | ||
|
||
// Getting Observatory URI from --vmservice-out-file is flaky as we're |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should delete the file before the test starts and then poll for the existence, that avoid scraping stdout which I 100% guarantee is less reliable. See https://github.com/flutter/flutter/blob/master/dev/devicelab/lib/tasks/track_widget_creation_enabled_task.dart#L118
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall approach looks good, but you really need to use the vmservice-out-file or this test will not be reliable/will cause problems if we rephrase any of the messages
Separately, I need to expose a vm service method for pulling the sksl. That would remove the need for all stdout scraping. I can clean that up later though
}); | ||
|
||
await attachReady.future; | ||
process.stdin.write('M'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a vm service request for this, so the stdin is unnecessary: https://github.com/flutter/flutter/blob/master/packages/flutter_tools/test/integration.shard/vmservice_integration_test.dart#L85
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried the vmservice but got flutterGetSkSL: (-32601) Method not found
or s0.flutterGetSkSL: (-32601) Method not found
... Can you please check my latest commit and see what I did wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now the method is only hooked up in debug mode, are you running in profile/release to generate the sksl? or are you sending the request during drive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if its easier I could add logic to write on exit for drive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work: #58743 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jonahwilliams, it's now using |
Yay! I'll try and get iOS hooked up with the sksl bundle soon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This test verifies that SkSL warmup can speedup cubic_bezier_perf's worst frame rasterization time by 2x (from ~90ms to ~45ms).
Fixes #35142