这是indexloc提供的服务,不要输入任何密码
Skip to content

fix duration event of timeline summary #47742

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

Merged
merged 3 commits into from
Dec 27, 2019

Conversation

yuanhuihui
Copy link
Contributor

Need to modify duration event to beginEnd event, because of PR https://dart-review.googlesource.com/c/sdk/+/127921

@fluttergithubbot fluttergithubbot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. labels Dec 24, 2019
@dnfield dnfield requested a review from liyuqian December 24, 2019 23:57
@yuanhuihui
Copy link
Contributor Author

relation to #47771

Copy link
Contributor

@liyuqian liyuqian left a comment

Choose a reason for hiding this comment

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

The PR itself looks good. I'm more concerned if we can have a Dart change like https://dart-review.googlesource.com/c/sdk/+/127921 but none of our framework tests would fail to notify that such Dart change is in some way breaking the Flutter framework. I also didn't notice any significant change in https://flutter-dashboard.appspot.com/benchmarks.html . @goderbauer : do you know if there's any integration test that's supposed to catch such Dart changes?

@yuanhuihui
Copy link
Contributor Author

The PR itself looks good. I'm more concerned if we can have a Dart change like https://dart-review.googlesource.com/c/sdk/+/127921 but none of our framework tests would fail to notify that such Dart change is in some way breaking the Flutter framework. I also didn't notice any significant change in https://flutter-dashboard.appspot.com/benchmarks.html . @goderbauer : do you know if there's any integration test that's supposed to catch such Dart changes?

https://dart-review.googlesource.com/c/sdk/+/127921, it has merged in out own engine.
I run command of "flutter drive" to test performance that report build failed. My PR merged into dart warehouse, not synchronized into engine?

Copy link
Contributor

@liyuqian liyuqian left a comment

Choose a reason for hiding this comment

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

@yuanhuihui : ah, you're right! Flutter framework is using a 7-day old engine right before your commit has been merged into the Dart SDK Github repo:

LGTM to the updated PR.

CC @franciscojma86 : I think the recent post-submit failures of the engine roll that you reverted is related to this PR. The engine roll probably should be paused until the landing of this PR.

@franciscojma86
Copy link
Contributor

@liyuqian yeah I stopped it yesterday. I was trying to figure out how to fix this so happy to see this PR :)

@franciscojma86
Copy link
Contributor

The build is green again @yuanhuihui, is this ok to land?

@franciscojma86
Copy link
Contributor

I'll land to unblock the roll. Will revert if needed.

@franciscojma86 franciscojma86 merged commit e43fd1c into flutter:master Dec 27, 2019
@franciscojma86
Copy link
Contributor

This seems to break the build

2019-12-27T09:51:44.476526: stdout: [+9570 ms] 00:10 �[32m+0�[0m�[31m -1�[0m: scrolling performance test complex_layout_scroll_perf �[1m�[31m[E]�[0m�[0m
2019-12-27T09:51:44.477108: stdout: [        ]   Invalid argument(s): durations is empty!
2019-12-27T09:51:44.498305: stdout: [  +21 ms]   package:flutter_driver/src/driver/timeline_summary.dart 167:7   TimelineSummary._averageInMillis
stdout: [        ]   package:flutter_driver/src/driver/timeline_summary.dart 33:12   TimelineSummary.computeAverageFrameBuildTimeMillis
stdout: [        ]   package:flutter_driver/src/driver/timeline_summary.dart 89:42   TimelineSummary.summaryJson
stdout: [        ]   package:flutter_driver/src/driver/timeline_summary.dart 130:4

Will revert for now

franciscojma86 added a commit to franciscojma86/flutter that referenced this pull request Dec 27, 2019
franciscojma86 added a commit that referenced this pull request Dec 27, 2019
franciscojma86 added a commit to franciscojma86/flutter that referenced this pull request Dec 27, 2019
franciscojma86 added a commit that referenced this pull request Dec 27, 2019
@flar
Copy link
Contributor

flar commented Dec 27, 2019

This PR needs to include a new hash in bin/internal/engine.version to pull over the appropriately patched Engine version that includes the change to Dart. It looks like any "green" engine build after flutter/engine@5a730c6 would do that.

@flar
Copy link
Contributor

flar commented Dec 27, 2019

It looks like flutter/engine@5a730c6 rolled into Engine cleanly so using that specific hash should work to get a clean roll into Framework.

@flar
Copy link
Contributor

flar commented Dec 28, 2019

All remaining issues fixed with the following PRs:

#47896
#47899

@yuanhuihui
Copy link
Contributor Author

yuanhuihui commented Dec 28, 2019

All remaining issues fixed with the following PRs:

#47896
#47899

In my own engine, we have fix flutter_gallery. Because of using a 7-day old engine, this timeline_summary PR failed to merge before. So I plan to submit a fixing flutter_gallery PR after the timeline_summary PR have merged. It seems that you have discovered and solved this problem now.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants