Repository navigation
5x startup test repitition to reduce noise - #67147
Conversation
The current test is still too noisy for Skia perf's step-function-based alert algorithm to detect performance regressions. See https://flutter-flutter-perf.skia.org/e/?begin=1596666370&end=1601603433&queries=test%3Dcomplex_layout__start_up&requestType=0 as an example where we fail to detect a regression for commit 1510865. Currently, a startup test runs for about 1 minute. After the change, the test should run for about 5 minutes or less because some setup steps don't need to be repeated 5 times.
flutter-dashboard
Bot
added
the
c: contributor-productivity
|
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. |
liyuqian
added
perf: speed
| await flutter('packages', options: <String>['get']); | ||
|
|
||
| const int iterations = 3; | ||
| const int iterations = 15; |
There was a problem hiding this comment.
This test takes a while to execute, are you sure you want to run it 15 times? I think it probably takes at least 1-2 minutes.
There was a problem hiding this comment.
Yes, I checked the build dashboard and here's what I observed and expected (as documented in the PR/commit description)
Currently, a startup test runs for about 1 minute. After the change, the
test should run for about 5 minutes or less because some setup steps
don't need to be repeated 5 times.
|
CC @mehmetf . Hopefully this would help our customers in the future. |
|
This caused all startup tests to significantly regress in end to end time. For example, on the iOS gallery this test now takes 12 minutes. This is roughly equivalent to a 9 more tests for each startup test, probably about 30-40 more test equivalents. |
This reverts commit e45157f.
|
@jonahwilliams What do you mean, I don't understand, can reference what you are seeing with a skia perf link? Any change in the results is possibly not a regression but a more representative result. |
No, I mean execution time on the devicelab itself. We're already under-capacity and this change is really hurting iOS |
|
I made that point to @liyuqian but he was under the impression that the increase in execution time would be palatable. @liyuqian want to increase it somewhere in the middle, do you have any suggestions @jonahwilliams based on what you saw with 3 versus 15 iterations? |
|
#68516 should help somewhat, in that I've turned the iter count down to 5. If you need more then that to stabilize it would be worth thinking about different ways to benchmark this. |
6 participants
The current test is still too noisy for Skia perf's step-function-based
alert algorithm to detect performance regressions. See
https://flutter-flutter-perf.skia.org/e/?begin=1596666370&end=1601603433&queries=test%3Dcomplex_layout__start_up&requestType=0
for an example where we fail to detect a regression for commit 1510865.
Currently, a startup test runs for about 1 minute. After the change, the
test should run for about 5 minutes or less because some setup steps
don't need to be repeated 5 times.