Add comments to clarify per-frame profiler by vauduong · Pull Request #1039 · facebookresearch/habitat-sim

vauduong · 2021-01-19T23:42:14Z

Motivation and Context

We added a per-frame profiler in #1015 to display frame duration, cpu duration, and gpu duration at runtime to be aware of bottlenecks in data processing when running our viewer. This PR adds comments to clarify how to interpret values.

How Has This Been Tested

Build and run

Types of changes

[ x] Docs change / refactoring / dependency upgrade
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

[x ] My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
[ x] I have read the CONTRIBUTING document.
[x ] I have completed my CLA (see CONTRIBUTING)
I have added tests to cover my changes.
All new and existing tests passed.

dhruvbatra · 2021-01-20T21:06:04Z

+      Mn::DebugTools::GLFrameProfiler::Value::FrameTime |  // Time to render per
+                                                           // frame frame
+      Mn::DebugTools::GLFrameProfiler::Value::
+          CpuDuration |  // Time to process action (eg. physics, key presses)


Is physics/simulation and UI processing the only two broad categories? Is there anything else?

The profiler currently measures just one scope, that's its limitation, except if more than one profiler instance would be used for more scopes, as i suggested on #1015. That's a possible path for extending this functionality.

(I might be expanding the Magnum functionality eventually as well, depending on what will be interesting to measure for the Vulkan backend.)

dhruvbatra · 2021-01-20T21:06:25Z

+          CpuDuration |  // Time to process action (eg. physics, key presses)
+                         // data per frame
+      Mn::DebugTools::GLFrameProfiler::Value::
+          GpuDuration;  // Time to process graphics data per frame


"Process graphics data" = rendering? Or do we mean something else?

@dhruvbatra : "GpuDruation" is using asynchronous query such as ARB_timer_query, to record the amount of time that GPU takes to fully complete a set of scoped GL commands.

I think this could be an easy enough interpretation of the numbers:

FrameTime is an inverse of FPS and should be 16.6 ms or less for smooth interactivity. In my opinion it's better than FPS because you can better measure improvements. "FPS increased by 10" means a totally different thing if it was from 10 to 20 or from 900 to 910, while "frame time decreased by 10 ms" always has a clear meaning.

CpuDuration measures how much CPU time was spent doing all work in a frame -- event processing, physics, but also traversing the scenegraph, submitting work for the GPU or the driver overhead. It should be less than GPU duration, if it's not then the GPU is sitting there bored.

GpuDuration measured how much time did it take for the GPU to process all work submitted by the CPU. If it's less than the CPU time then if you reduce the CPU / driver overhead you can do / draw more in a frame (or draw frames faster), if it's significantly more than the CPU time then you're GPU-bound -- rendering too dense meshes, having too high texture resolution, or maybe just rendering a lot of objects that end up occluded or out of the view or having inefficient layout of the mesh data. Generally, since the aim here is to render as fast as possible (and not power efficiency for example), the CPU and GPU time should be roughly equal.

@vauduong in case you haven't stumbled upon it yet, there's an article about the geometry pipeline in Magnum, with more info about how to interpret the values and what makes meshes slow or fast to render: https://blog.magnum.graphics/announcements/new-geometry-pipeline/ It's scary long but written hopefully in a general enough way that might give you useful info even if you don't end up using Magnum further in your career ;)

Thanks for the helpful comments @mosras! I did stumble upon that article and it was also super informative :)

bigbike · 2021-01-21T18:01:48Z

@mosra : Saw you are in the reviewer list.
Any comments would be highly appreciated. Thanks!

vauduong · 2021-01-21T18:30:33Z

@mosra : Yes, would appreciate any feedback on writing more precise comments to help users understand what the GLProfiler is doing!

mosra · 2021-01-21T18:59:10Z

Replied above :)

I'm realizing some of this info could go straight into Magnum docs as well, because the explanation is currently a bit underwhelming.

bigbike · 2021-01-22T00:39:43Z

+   * Uses asynchronous querying to measure the amount of time
+   * to fully complete a set of GL commands without stalling rendering, 3 frame
+   * delay
+   * Asynchronous querying extensions: ARB_timer_query (OpenGL 3.3),


I will recommend removing such details (L567 - L570)

bigbike · 2021-01-22T00:40:07Z

+   * CpuDuration: (Units::Nanoseconds) CPU time spent processing events,
+   * physics, traversing SceneGraph, and submitting data to GPU/drivers per
+   * frame
+   * Measured using std::chrono::high_resolution_clock, 1 frame delay


remove details such as L560.

bigbike · 2021-01-22T00:41:27Z

+   * frame
+   * Measured using std::chrono::high_resolution_clock, 1 frame delay
+   *
+   * GpuDuration: (Units::Nanoseconds) GPU time spent rendering data submitted


measured how much time it takes for the GPU to process all work submitted by the CPU.

bigbike · 2021-01-22T00:42:18Z

+   * GpuDuration: (Units::Nanoseconds) GPU time spent rendering data submitted
+   * by CPU per frame
+   * Uses asynchronous querying to measure the amount of time
+   * to fully complete a set of GL commands without stalling rendering, 3 frame


remove 3 frame delay.

bigbike · 2021-01-22T00:43:36Z

      Mn::DebugTools::GLFrameProfiler::Value::GpuDuration;

-// VertexFetchRatio and PrimitiveClipRatio only supported for GL 4.6
+/**


You do not need this. Undo the change.

Add comments to clarify per frame profiler

fd0242e

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Jan 19, 2021

Lint

af8a693

vauduong requested review from bigbike and dhruvbatra January 20, 2021 20:53

dhruvbatra reviewed Jan 20, 2021

View reviewed changes

vauduong requested a review from mosra January 21, 2021 17:59

bigbike requested review from aclegg3 and eundersander January 21, 2021 18:00

Update with master, improve comments

cbc8a3f

bigbike reviewed Jan 22, 2021

View reviewed changes

Update with feedback

c19042b

vauduong merged commit 15ab57f into facebookresearch:master Jan 22, 2021

vauduong deleted the profiler-comments branch January 22, 2021 18:27

Conversation

vauduong commented Jan 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

How Has This Been Tested

Types of changes

Checklist

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!

bigbike commented Jan 21, 2021

Uh oh!

vauduong commented Jan 21, 2021

Uh oh!

mosra commented Jan 21, 2021

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vauduong commented Jan 19, 2021 •

edited

Loading