[Impeller] Add GPU frame time to Vulkan backend using timestamp queries. #46796
Conversation
|
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. |
chinmaygarde
changed the title
|
I don't think this measures anything useful does it? Command buffers may complete in any order. Even if you depend on queue submission order, thats only when commands are consumed, not when they are completed. I'd rather not expose this information as it seems borderline meaningless. |
|
Fair enough, We can do this for real with timestamp queries. |
| GPUTracerVK::GPUTracerVK(const std::shared_ptr<ContextVK>& context) | ||
| : context_(context) { | ||
| timestamp_period_ = | ||
| context_->GetPhysicalDevice().getProperties().limits.timestampPeriod; |
There was a problem hiding this comment.
I think you need to check if this is actually supported right? Otherwise the value might be meaningless.
There was a problem hiding this comment.
the cmd itself is in 1.0, but I need to check that the queue supports timestamp queries. I think this will always be supported on graphics queues though, just might not have as much resolution as we'd like.
There was a problem hiding this comment.
some tests failed due to queue contruction issues so I added some logs to see what this value ends up as ...
There was a problem hiding this comment.
yup, this can be zero. Good call.
jonahwilliams
changed the title
jonahwilliams
changed the title
| private: | ||
| void ResetQueryPool(size_t pool); | ||
|
|
||
| const std::shared_ptr<ContextVK> context_; |
There was a problem hiding this comment.
I'm not a smart C++ guy. Is the ContextVK owning a shared_ptr to the GPUTracerVK and the GPUTracerVK also having a shared_ptr to the ContextVK going to cause problems? Feels a bit like a cycle. Could use weakptrs...
| // timeline event but that is a job for a future Jonah. | ||
| auto gpu_ms = (((bits[1] - bits[0]) * timestamp_period_) / 1000000); | ||
| FML_TRACE_COUNTER("flutter", "GPUTracer", | ||
| 1234, // Trace Counter ID |
There was a problem hiding this comment.
This should probably just be reinterpret_cast<int64_t>(this), or at least a named constant somewhere.
…/engine into add_start_end_trace_events
|
Trying this out on the old flutter gallery perf benchmark is triggering some crash in the driver. I'm going to investigate this a bit more :( |
|
This patch is triggering a crash, but only on a particular benchmark: the old gallery with the old zoom page transition enabled. There currently isn't a stack, so I'm rebuilding with validation layers to investigate. |
|
No validation warnings. |
|
Here is the crash. This is it. No symbols since it seems to be in the driver. |
|
This is pretty easy to repro. Run the old flutter gallery with --dart-define=flutter.benchmarks.force_disable_snapshot=true and navigate back and forth between pages that do the android zoom transition. |
|
The full command I'm using is: in |
|
Okay I fixed this by switching both queries to bottom of pipe. One day I'll learn what these do. |
jonahwilliams
added
the
autosubmit
…136566) flutter/engine@a4e6258...cd1a5ed 2023-10-13 skia-flutter-autoroll@skia.org Roll Skia from 4bc4b4d22866 to 9affbebb459a (2 revisions) (flutter/engine#46913) 2023-10-13 jonahwilliams@google.com [Impeller] Add GPU frame time to Vulkan backend using timestamp queries. (flutter/engine#46796) 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 jsimmons@google.com,rmistry@google.com,zra@google.com 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
These values will be 0 until flutter/engine#46846 and flutter/engine#46796 roll into the framework.
…es. (#46796) For flutter/flutter#136408 , Approximate the GPU frame time by measuring the difference between a timestamp query recorded at the beginning of the frame and a timestamp query recorded at the end of the frame.
For flutter/flutter#136408 ,
Approximate the GPU frame time by measuring the difference between a timestamp query recorded at the beginning of the frame and a timestamp query recorded at the end of the frame.