[Impeller] Rework Vulkan GPUTracker to decorate existing cmd buffers. #46963

jonahwilliams · 2023-10-16T19:51:39Z

Switches the GPU tracing implementation to work more like the metal version, where we decorate individual cmd buffers with start/end time queries and then min/max the final result. Hopefully this stabilizes the values on CI

flutter-dashboard · 2023-10-16T19:51:41Z

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 or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

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.

jonahwilliams · 2023-10-16T21:25:13Z

I even added ... well, its sort of a test.

dnfield · 2023-10-16T21:56:56Z

impeller/renderer/backend/vulkan/command_encoder_vk.cc

+
  auto status = command_buffer.end();
  if (status != vk::Result::eSuccess) {
+    gpu_tracer_->OnFenceComplete(end_frame);


Here and below, should we somehow signal to the tracer that it's ending really fast because of a failure?

I'm assuming here we'd get a zero duration, do we want something like a -1 duration?

Done. If we encounter an error we should not report for the frame since its probably an all bets are off situation.

dnfield · 2023-10-16T21:58:47Z

impeller/renderer/backend/vulkan/gpu_tracer_vk.cc

-#include "impeller/renderer/backend/vulkan/command_encoder_vk.h"
 #include "impeller/renderer/backend/vulkan/context_vk.h"
-#include "impeller/renderer/command_buffer.h"
+#include "vulkan/vulkan_enums.hpp"


Is this right? Should this just be vulkan/vulkan.hpp?

I get "included header is not used"

jonahwilliams · 2023-10-16T22:57:55Z

The previous implementation regressed raster time due to the addition of more cmd buffer submissions:

https://flutter-flutter-perf.skia.org/e/?keys=X9d806842a167e6fdac66ce7ec1922f27&requestType=0&selected=commit%3D37394%26name%3D%252Carch%253Dintel%252Cbranch%253Dmaster%252Cconfig%253Ddefault%252Cdevice_type%253DPixel_7_Pro%252Cdevice_version%253Dnone%252Chost_type%253Dlinux%252Csub_result%253Daverage_frame_rasterizer_time_millis%252Ctest%253Dnew_gallery_impeller__transition_perf%252C&xbaroffset=37394

We should make sure this fixes that.

matanlurey

Approved.

I'd file a follow-up issue for any comments/tests that are worth adding after this sticks.

matanlurey · 2023-10-17T18:07:35Z

impeller/renderer/backend/vulkan/gpu_tracer_vk.cc

 }

-void GPUTracerVK::RecordStartFrameTime() {
+bool GPUTracerVK::IsValid() const {


Just being pedantic, IsValid feels weird given it's totally valid for this instance to exist in an invalid state (i.e. either timestamp_period not being supported, or debug mode being disabled).

Consider renaming this IsEnabled() and consider making it private since there isn't (AFAICT) a reason for another class to check this property, it's internal-only state.

This is more or less just for testing. Update to IsEnabled SGTM

matanlurey · 2023-10-17T18:08:23Z

impeller/renderer/backend/vulkan/gpu_tracer_vk.cc

+}
+
+void GPUTracerVK::MarkFrameStart() {
+  in_frame_ = true;


Is it worth adding a FML_DCHECK that a frame is never started when in_frame_ already is true?

(Maybe that's fine given what you told me offline about multiple frames running concurrently - if so a 1-line comment documenting that would be nice to have)

Done, and added more comments to the in_frame_ field

matanlurey · 2023-10-17T18:08:54Z

impeller/renderer/backend/vulkan/gpu_tracer_vk.cc

+  current_state_ = (current_state_ + 1) % 16;
+
+  auto& state = trace_states_[current_state_];
+  FML_DCHECK(state.pending_buffers == 0u);


nit: Can you add a << "Reason why bad" sort of self-documenting why this should be 0?

It's confusing because the next line sets it to 0, why set it to 0 if it's already 0 :P

matanlurey · 2023-10-17T18:11:50Z

impeller/renderer/backend/vulkan/gpu_tracer_vk.cc

+  // We size the query pool to kPoolSize, but Flutter applications can create an
+  // unbounded amount of work per frame. If we encounter this, stop recording
+  // cmds.
+  if (state.current_index >= kPoolSize) {


Do you expect this to be common?

I wonder if it makes sense to at least log that recording did not happen.

No, I think this would be extremely rare but adapting the tracer to work with arbitrarily resizing query pools is much harder so punting for now.

matanlurey · 2023-10-17T18:12:17Z

impeller/renderer/backend/vulkan/gpu_tracer_vk.cc

-void GPUTracerVK::RecordEndFrameTime() {
-  if (!valid_ || !started_frame_) {
+void GPUTracerVK::RecordCmdBufferStart(const vk::CommandBuffer& buffer) {
+  if (!valid_ || !in_frame_) {


Is the !in_frame_ guard related to multiple concurrent frames?

If so can we add that as a 1-line comment. If not should it be DCHECK'd instead?

Added more explaination to what in_frame_ does

matanlurey · 2023-10-17T18:12:33Z

impeller/renderer/backend/vulkan/gpu_tracer_vk.cc

+}
+
+size_t GPUTracerVK::RecordCmdBufferEnd(const vk::CommandBuffer& buffer) {
+  if (!valid_ || !in_frame_) {


(Ditto above, though if no action is needed then disregard)

matanlurey · 2023-10-17T18:13:02Z

impeller/renderer/backend/vulkan/gpu_tracer_vk.cc

+  state.contains_failure = !success;
+  state.pending_buffers -= 1;
+
+  if (state.pending_buffers == 0 && !state.contains_failure) {


Explain in 1-line what you told me offline re: filtering out failures

Added more comment to contains_failure field on trace state.

matanlurey · 2023-10-17T18:14:09Z

impeller/renderer/backend/vulkan/test/mock_vulkan.cc

      static_cast<VkSampleCountFlags>(VK_SAMPLE_COUNT_1_BIT |
                                      VK_SAMPLE_COUNT_4_BIT);
  pProperties->limits.maxImageDimension2D = 4096;
+  pProperties->limits.timestampPeriod = 1;


(Just a comment)

We could make this configurable if you want to ensure timestampPeriod of 0 doesn't crash.

matanlurey · 2023-10-17T18:19:09Z

impeller/renderer/backend/vulkan/test/gpu_tracer_unittests.cc

As mentioned offline, I think given we're not sure this will stay landed (performance reasons, or otherwise), more tests aren't needed at this point as long as it "works on my machine". However I'd add a TODO for at least the following unit tests if this stays landed:

These are just suggestions, feel free to tweak wording

Success case where a frame is captured

Success case where MAX_FRAMES are captured

Success case where > MAX_FRAMES are captured and it wraps around (?)

Assert query pool is only initialized on the first frame

Failure case for unbounded amount of frames

Assert that contains_failure captures are dropped

Assert that your logic for longest frame WAI (i.e. multiple concurrent frames captures the right values)

Also should be asserting the final call to FML_TRACE_COUNTEr is actually recording the expected value.

jonahwilliams · 2023-10-17T21:08:17Z

Fixed an issue where we could partially record the mipmap cmd buffers. We now record the raster thread id and only permit cmd buffers recorded from the raster thread (in additon to the thread boundary checks).

flutter-dashboard · 2023-10-17T22:01:21Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #46963 at sha 8394706

…d buffers. (flutter/engine#46963)

…136774) flutter/engine@5df7af3...2eef9b4 2023-10-17 [email protected] Move imgui from buildroot to flutter third_party (flutter/engine#47031) 2023-10-17 [email protected] [Impeller] Rework Vulkan GPUTracker to decorate existing cmd buffers. (flutter/engine#46963) 2023-10-17 [email protected] [fml][embedder] Improve thread-check logging (flutter/engine#47020) 2023-10-17 [email protected] Roll Dart SDK from 99ce477503f8 to da48c75b73b1 (1 revision) (flutter/engine#47027) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…#46963) Switches the GPU tracing implementation to work more like the metal version, where we decorate individual cmd buffers with start/end time queries and then min/max the final result. Hopefully this stabilizes the values on CI

[Impeller] Make Vulkan tracing not crashy.

78e1e78

github-actions bot added the e: impeller label Oct 16, 2023

++

c16b21f

jonahwilliams changed the title ~~[Impeller] Make Vulkan tracing not crashy.~~ [Impeller] Rework Vulkan GPUTracker to decorate existing cmd buffers. Oct 16, 2023

jonahwilliams requested a review from dnfield October 16, 2023 20:37

its sort of a test.

df9b48a

disable test in release mode.

a479267

dnfield reviewed Oct 16, 2023

View reviewed changes

++

c7f4e49

jonahwilliams requested review from dnfield and matanlurey October 16, 2023 23:16

matanlurey approved these changes Oct 17, 2023

View reviewed changes

jonahwilliams added 2 commits October 17, 2023 13:03

++

e2ba52f

fix mipmap cmd buffers being partially recorded.

3f096e2

++

8394706

flutter-dashboard bot added the will affect goldens label Oct 17, 2023

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 17, 2023

auto-submit bot merged commit 910a7cb into flutter:main Oct 17, 2023

jonahwilliams deleted the make_vulkan_tracing_not_crashy branch October 17, 2023 22:54

engine-flutter-autoroll mentioned this pull request Oct 18, 2023

Roll Flutter Engine from 5df7af34a718 to 2eef9b43cfb4 (4 revisions) flutter/flutter#136774

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 18, 2023

910a7cb2a [Impeller] Rework Vulkan GPUTracker to decorate existing cm…

39450c9

…d buffers. (flutter/engine#46963)

[Impeller] Rework Vulkan GPUTracker to decorate existing cmd buffers. #46963

[Impeller] Rework Vulkan GPUTracker to decorate existing cmd buffers. #46963

Uh oh!

Conversation

jonahwilliams commented Oct 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Oct 16, 2023

Uh oh!

jonahwilliams commented Oct 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Oct 16, 2023

Uh oh!

matanlurey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Oct 17, 2023

Uh oh!

flutter-dashboard bot commented Oct 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jonahwilliams commented Oct 16, 2023 •

edited

Loading