-
Notifications
You must be signed in to change notification settings - Fork 28.9k
extract engine sub-metrics; change reported metrics #55331
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
Conversation
- Extract sub-metrics reported by the Web engine: "preroll_frame", "apply_frame". - Add a concept of unreported metrics: displayed on the benchmark UI, but not on the dashboard. - Make "sceneBuildDuration" and "windowRenderDuration" unreported, which are too fine-grained. They are included in "drawFrameDuration" already. - Report outlier ratio instead of outlier average. The ratio is more useful of the two.
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. |
Please add a link to the engine PR in the description. That way it's easy to follow the breadcrumbs. |
Done. |
stopListeningToEngineBenchmarkValues(kProfilePrerollFrame); | ||
stopListeningToEngineBenchmarkValues(kProfileApplyFrame); |
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.
Isn't it safer to put these two in the finally
block?
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.
Done.
js_util.setProperty(html.window, '_flutter_internal_on_benchmark', _dispatchEngineBenchmarkValue); | ||
} | ||
|
||
_engineBenchmarkListeners[name] = listener; |
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.
Given that this will override previous listeners for name
, should we throw if there's an existing listener for name
?
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.
Done.
Description
The new metrics will be added to the engine as part of flutter/engine#17852. However, this PR does not need to wait for the engine PR. If the metrics are missing, they are simply not reported.