-
Notifications
You must be signed in to change notification settings - Fork 5.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
started polling the gpu usage #18752
Conversation
d71895b
to
c915b19
Compare
@@ -139,13 +139,18 @@ source_set("flutter_framework_source") { | |||
public_configs = [ "//flutter:config" ] | |||
|
|||
libs = [ |
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'm surprised that out GN format presubmit doesn't account for these.
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.
Same, that formatter is usually pretty strict!
@@ -0,0 +1,229 @@ | |||
// Copyright 2013 The Flutter Authors. All rights reserved. |
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.
Has this file been copied from somewhere? If so could you add a comment pointing to the source or reference? Reading it seems like it's an accumulation of a bunch of IOKit headers into one file.
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.
Yep, added documentation.
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.
How does copyright work when the header is an amalgamation of other (presumably copyrighted) headers?
return &object_; | ||
} | ||
T get() { return object_; } | ||
void swap(T new_value) { |
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.
this is more like reset
.
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 call, changed
@@ -28,9 +30,126 @@ | |||
} | |||
|
|||
namespace flutter { | |||
namespace { | |||
|
|||
#if FLUTTER_RUNTIME_MODE == FLUTTER_RUNTIME_MODE_DEBUG || \ |
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 how big this class is getting, consider moving this to gpu_profiler_metrics_ios.<h|mm>
.
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.
The file is 228 lines, shrug. If you feel strongly about it I can move it. It's an easy change, just takes time with our current system.
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
08003f0
to
293f8c6
Compare
converted to draft until the parent PRs are approved and land |
- added documentation - fixed method name
bf5f6fd
to
1000775
Compare
…he defines come from
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. |
Landing on red. The Drone CI tasks have stalled out and the engine is red because of a GOMA issue on the windows build machine. The last PR just changed the name of 2 ios test methods. |
It's now replaced by flutter/engine#18752
It's now replaced by flutter/engine#18752
Description
Started polling the GPU's usage percentage via IOKit in profile and debug builds.
Related Issues
flutter/flutter#58197
Tests
I added the following tests:
None, code relies on communicating with the kernel.
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
). This will ensure a smooth and quick review process.Breaking Change
Did any tests fail when you ran them? Please read handling breaking changes.