Skip to content

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

Merged
merged 2 commits into from
Apr 23, 2020

Conversation

yjbanov
Copy link
Contributor

@yjbanov yjbanov commented Apr 21, 2020

Description

  • 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.

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.

- 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.
@yjbanov yjbanov requested a review from mdebbar April 21, 2020 20:52
@fluttergithubbot fluttergithubbot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Apr 21, 2020
@fluttergithubbot
Copy link
Contributor

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.

@yjbanov yjbanov requested a review from ferhatb April 21, 2020 20:52
@mdebbar
Copy link
Contributor

mdebbar commented Apr 21, 2020

Please add a link to the engine PR in the description. That way it's easy to follow the breadcrumbs.

@yjbanov
Copy link
Contributor Author

yjbanov commented Apr 21, 2020

Please add a link to the engine PR in the description. That way it's easy to follow the breadcrumbs.

Done.

Comment on lines 388 to 389
stopListeningToEngineBenchmarkValues(kProfilePrerollFrame);
stopListeningToEngineBenchmarkValues(kProfileApplyFrame);
Copy link
Contributor

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?

Copy link
Contributor Author

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;
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@yjbanov yjbanov merged commit 6f8945f into flutter:master Apr 23, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
c: contributor-productivity Team-specific productivity, code health, technical debt.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants