LiveMetric: realtime statistics for debug UI. #405

hysw · 2024-01-16T00:21:49Z

This PR is a fork of #377 .

Adding real time statistics into the metric framework so it is easier to read.

mdagois · 2024-01-16T04:19:24Z

Could you please add a quick summary of what problem the PR solves?
I am not extremely familiar with the benchmark, and I have not touched BigWheels for a few months.

hysw · 2024-01-16T05:00:17Z

Could you please add a quick summary of what problem the PR solves?
I am not extremely familiar with the benchmark, and I have not touched BigWheels for a few months.

This gives a readable statistics for cpu submission time / gpu work duration / etc. i.e. the leading digit does not jump around every frame. This version is due to suggestion of @Keenuts 's suggestion on have it in the metric framework. The stand alone version is #377 .

Keenuts

Thanks for putting this together. A few comments 😊

src/ppx/metrics.cpp

include/ppx/metrics.h

Keenuts · 2024-01-16T12:41:41Z

include/ppx/metrics.h

@@ -299,6 +415,12 @@ class Manager final
    // Get Gauge Basic Statistics, only works for type GAUGE
    GaugeBasicStatistics GetGaugeBasicStatistics(MetricID id) const;

+    // Get Live Statistics for a LiveMetric, only works for type GAUGE
+    LiveStatistics GetLiveStatistics(MetricID id) const;


Shall this be renamed to GetGaugeLiveStatistics to match the function above?

It does not report MetricGauge's live statistics, so I'll leave Gauge out of the name.

benchmarks/graphics_pipeline/GraphicsBenchmarkApp.cpp

mdagois · 2024-01-17T08:33:38Z

Could you please add a quick summary of what problem the PR solves?
I am not extremely familiar with the benchmark, and I have not touched BigWheels for a few months.

This gives a readable statistics for cpu submission time / gpu work duration / etc. i.e. the leading digit does not jump around every frame. This version is due to suggestion of @Keenuts 's suggestion on have it in the metric framework. The stand alone version is #377 .

It is a kind of tracker of statistics for an underlying gauge metrics, and you reset it (by clearing history) whenever you want. Is that correct?

Keenuts · 2024-01-17T13:08:38Z

Could you please add a quick summary of what problem the PR solves?
I am not extremely familiar with the benchmark, and I have not touched BigWheels for a few months.

This gives a readable statistics for cpu submission time / gpu work duration / etc. i.e. the leading digit does not jump around every frame. This version is due to suggestion of @Keenuts 's suggestion on have it in the metric framework. The stand alone version is #377 .

It is a kind of tracker of statistics for an underlying gauge metrics, and you reset it (by clearing history) whenever you want. Is that correct?

The initial idea is to have a layer on top of the raw metrics that averages it over X samples.
Precision is not important since it's only used for UI display. What's more important is to be able to eyeball the metrics quickly (hence the window average).
Because this is for quick eyeballing investigation, it's a "nice to have" to be able to disable recording.

That's for the goal/idea I have in mind.

Keenuts

Thanks for the changes! Added comments, but LGTM for the direction. Addind second owner.

include/ppx/application.h

src/test/metrics_test.cpp

hysw · 2024-01-17T19:55:54Z

It is a kind of tracker of statistics for an underlying gauge metrics, and you reset it (by clearing history) whenever you want. Is that correct?

Addition to Nathan's comment. The use case is to tracks a set of gauge metrics that we want to see on screen, not necessary the ones we want to record to a profiling run (e.g. cpu time spent on input processing). The samples are $2^{-t_{elapsed}}$ weighted to make sure we are showing recent information (also this formulation allows constant time update). Please note this is not the last X samples since interval between sample can vary from $<0.1ms$ to $100^+ ms$ . The clearing of history is somewhat unnecessary, since old data's weight will decay to almost zero in a few seconds, it's mainly done to since if we know parameter has changed, immediate update is better than waiting a few seconds.

Statistics include latest/mean/variance/min/max.

hysw · 2024-01-17T20:46:31Z

Adding Angela & Ran since they have reviewed #377 .

mdagois · 2024-01-18T01:55:02Z

Could you please add a quick summary of what problem the PR solves?
I am not extremely familiar with the benchmark, and I have not touched BigWheels for a few months.

This gives a readable statistics for cpu submission time / gpu work duration / etc. i.e. the leading digit does not jump around every frame. This version is due to suggestion of @Keenuts 's suggestion on have it in the metric framework. The stand alone version is #377 .

It is a kind of tracker of statistics for an underlying gauge metrics, and you reset it (by clearing history) whenever you want. Is that correct?

The initial idea is to have a layer on top of the raw metrics that averages it over X samples. Precision is not important since it's only used for UI display. What's more important is to be able to eyeball the metrics quickly (hence the window average). Because this is for quick eyeballing investigation, it's a "nice to have" to be able to disable recording.

That's for the goal/idea I have in mind.

Ok, thx. Makes sense.

mdagois · 2024-01-18T02:08:12Z

LGTM from a metrics perspective. I'll defer approval to people that own the project.

apazylbe

Sorry for the late review, only left a couple of comments cause I need to take another look fully.
Personally though, I find the metrics framework more confusing with these changes, and not sure if the complexity is justified. But if I am in minority, I am also fine with this.
Also I would like for Application to contain less code that doesn't need to be there, I understand we have some wrappers there to add additional metrics, but do the others functions need to be there?

apazylbe · 2024-01-18T03:40:26Z

include/ppx/application.h

+    bool BindMetric(metrics::MetricID metricID, const metrics::MetricMetadata& metadata);
+
+    // Bind a live metric, return true on success.
+    bool BindLiveMetric(metrics::MetricID metricID);


Could you elaborate here? What does "binding" a metric mean? Why do we need to call that? What happens if we don't?
I would ideally like to know these things without reading the code.

apazylbe · 2024-01-18T03:41:13Z

include/ppx/application.h

@@ -523,13 +524,28 @@ class Application
    // See StartMetricsRun for why this wrapper is necessary.
    virtual bool HasActiveMetricsRun() const;

+    // Allocate a metric id to be used for a combined live/recorded metric.


Do we need to have all of these metric related functions in Application?

hysw · 2024-01-18T18:20:37Z

Personally though, I find the metrics framework more confusing with these changes, and not sure if the complexity is justified.

My preference is keeping it simple and separate, however Nathan have some good point, so I also made this version to see if integration makes sense.

hysw requested review from Keenuts and mdagois January 16, 2024 00:21

hysw force-pushed the tianc/metric-live branch 2 times, most recently from c875639 to 55f4a42 Compare January 16, 2024 00:28

hysw force-pushed the tianc/metric-live branch from 55f4a42 to 557479c Compare January 16, 2024 04:58

Keenuts requested changes Jan 16, 2024

View reviewed changes

Keenuts reviewed Jan 16, 2024

View reviewed changes

benchmarks/graphics_pipeline/GraphicsBenchmarkApp.cpp Outdated Show resolved Hide resolved

hysw force-pushed the tianc/metric-live branch from cbb395f to 8488b7d Compare January 16, 2024 18:57

Keenuts approved these changes Jan 17, 2024

View reviewed changes

include/ppx/application.h Outdated Show resolved Hide resolved

src/test/metrics_test.cpp Outdated Show resolved Hide resolved

Keenuts requested a review from apazylbe January 17, 2024 13:34

hysw added 3 commits January 17, 2024 15:33

LiveMetric: realtime statistics for debug UI.

8b98a0d

Statistics include latest/mean/variance/min/max.

Address reviewer comment and add test

e987c9a

Fix typo, add more comments and tests

6bab0a5

hysw force-pushed the tianc/metric-live branch from e97381b to 6bab0a5 Compare January 17, 2024 20:33

hysw requested review from angela28chen and wangra-google January 17, 2024 20:45

apazylbe reviewed Jan 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LiveMetric: realtime statistics for debug UI. #405

LiveMetric: realtime statistics for debug UI. #405

hysw commented Jan 16, 2024

mdagois commented Jan 16, 2024

hysw commented Jan 16, 2024

Keenuts left a comment

Keenuts Jan 16, 2024

hysw Jan 17, 2024

mdagois commented Jan 17, 2024

Keenuts commented Jan 17, 2024

Keenuts left a comment

hysw commented Jan 17, 2024 •

edited

Loading

hysw commented Jan 17, 2024

mdagois commented Jan 18, 2024

mdagois commented Jan 18, 2024

apazylbe left a comment

apazylbe Jan 18, 2024

apazylbe Jan 18, 2024

hysw commented Jan 18, 2024

LiveMetric: realtime statistics for debug UI. #405

Are you sure you want to change the base?

LiveMetric: realtime statistics for debug UI. #405

Conversation

hysw commented Jan 16, 2024

mdagois commented Jan 16, 2024

hysw commented Jan 16, 2024

Keenuts left a comment

Choose a reason for hiding this comment

Keenuts Jan 16, 2024

Choose a reason for hiding this comment

hysw Jan 17, 2024

Choose a reason for hiding this comment

mdagois commented Jan 17, 2024

Keenuts commented Jan 17, 2024

Keenuts left a comment

Choose a reason for hiding this comment

hysw commented Jan 17, 2024 • edited Loading

hysw commented Jan 17, 2024

mdagois commented Jan 18, 2024

mdagois commented Jan 18, 2024

apazylbe left a comment

Choose a reason for hiding this comment

apazylbe Jan 18, 2024

Choose a reason for hiding this comment

apazylbe Jan 18, 2024

Choose a reason for hiding this comment

hysw commented Jan 18, 2024

hysw commented Jan 17, 2024 •

edited

Loading