[Impeller] disable GLES tracing unless opted in. #165887
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
github-actions
bot
added
engine
| @@ -129,7 +129,7 @@ ProcTableGLES::ProcTableGLES( // NOLINT(google-readability-function-size) | |||
|
|
|||
| #undef IMPELLER_PROC | |||
|
|
|||
| if (!description_->HasDebugExtension()) { | |||
| if (!description_->HasDebugExtension() || !ENABLE_GLES_LABELING) { | |||
There was a problem hiding this comment.
| if (!description_->HasDebugExtension() || !ENABLE_GLES_LABELING) { | |
| if (!ENABLE_GLES_LABELING || !description_->HasDebugExtension()) { |
You want to hit the macro first.
| @@ -14,6 +14,9 @@ | |||
| #include "impeller/renderer/backend/gles/description_gles.h" | |||
| #include "impeller/renderer/backend/gles/gles.h" | |||
|
|
|||
| /// Enable to allow GLES to push/pop labels for usage in GPU traces | |||
| #define ENABLE_GLES_LABELING false | |||
jonahwilliams
added
the
autosubmit
auto-submit
bot
removed
the
autosubmit
|
autosubmit label was removed for flutter/flutter/165887, because - The status or check suite Linux_android_emu android_engine_vulkan_tests has failed. Please fix the issues identified (or deflake) before re-applying this label. |
jonahwilliams
added
the
autosubmit
| @@ -96,33 +96,6 @@ TEST(ReactorGLES, UntrackedHandle) { | |||
| EXPECT_TRUE(reactor->React()); | |||
| } | |||
|
|
|||
| TEST(ReactorGLES, NameUntrackedHandle) { | |||
There was a problem hiding this comment.
Instead of deleting this, why not just put it in the preprocessor guard. This test seems to point to a better way to disable this: SupportsDebugLabels(). If we turn that off this test will be skipped automatically.
There was a problem hiding this comment.
Yeah i shouldn't remove this.
There was a problem hiding this comment.
The gtest skip still triggers the gmock assertions though, is there a way to disable them?
There was a problem hiding this comment.
Can you just move the expect calls to after the skip?
There was a problem hiding this comment.
Its after we've moved the mock gles :(
There was a problem hiding this comment.
Are you sure the GTEST_SKIP doesn't just handle cancelling the expects?
You could keep a raw pointer around to do the expect_calls after the skips. It's not pretty but it's a test and we know it will still be in memory.
There was a problem hiding this comment.
I'll try the raw_ptr
flutter-dashboard
bot
removed
the
autosubmit
GLES tracing may add substantial overhead on some devices. Since this is only useful when using frame capture tools, and its unlikely anyone that isn't an engine developer (or who knows how to compile the engine) will use them - we can turn off with a define. If there are other usecases we need to cover later this can be made to use the new flag system.
GLES tracing may add substantial overhead on some devices. Since this is only useful when using frame capture tools, and its unlikely anyone that isn't an engine developer (or who knows how to compile the engine) will use them - we can turn off with a define.
If there are other usecases we need to cover later this can be made to use the new flag system.