[render-test] Add gfx probe for tracking gpu resource usage #15843

mpulkki-mapbox · 2019-10-22T12:12:22Z

No description provided.

pozdnyakov · 2019-10-23T11:25:30Z

include/mbgl/gfx/rendering_stats.hpp

+struct RenderingStats {
+    bool isZero() const;
+
+    int numDrawCalls = 0;


nit: RenderingStats() = default; ?

pozdnyakov · 2019-10-23T11:28:23Z

platform/default/src/mbgl/gfx/headless_frontend.cpp

    std::exception_ptr error;

    map.renderStill([&](std::exception_ptr e) {
        if (e) {
            error = e;
        } else {
-            result = backend->readStillImage();
+            result.image = backend->readStillImage();
+            result.stats = getBackend()->getContext().renderingStats();


backend->getContext()?

getBackend() is not a trivial getter function

pozdnyakov · 2019-10-23T11:29:26Z

platform/default/include/mbgl/gfx/headless_frontend.hpp

@@ -17,6 +18,11 @@ class TransformState;

 class HeadlessFrontend : public RendererFrontend {
 public:
+    struct RenderResult {


shall we add bool isValid() const here?

Yeah could be added. In current use std::exception is thrown if an error is encountered

pozdnyakov · 2019-10-23T11:32:49Z

include/mbgl/gfx/rendering_stats.hpp

+namespace gfx {
+
+struct RenderingStats {
+    bool isZero() const;


isEmpty() ?

I would say that being "empty" is not the right term in this context.

pozdnyakov · 2019-10-23T11:38:50Z

platform/default/include/mbgl/gfx/headless_frontend.hpp

@@ -48,7 +54,7 @@ class HeadlessFrontend : public RendererFrontend {
    LatLng latLngForPixel(const ScreenCoordinate&);

    PremultipliedImage readStillImage();


shall readStillImage() return RenderResult too? (Might be added later)

This is an interesting function as it has to be called from the rendering thread. It can be used together with the Map::renderStill(...) to fetch the still image in the callback.

I think this could be updated later.

pozdnyakov · 2019-10-23T11:42:02Z

render-test/metadata.hpp

@@ -82,13 +88,33 @@ struct NetworkProbe {
    size_t transferred;
 };

+struct GfxProbe {
+    struct Memory {
+        int allocated = 0;


Memory tracking between probeGFXStart and probeGFXEnd is relative, ie. these numbers could be negative in a tracking segment where more memory was released than allocated. I might need to rethink the logic :)

pozdnyakov · 2019-10-23T11:42:38Z

render-test/metadata.hpp

+    GfxProbe() = default;
+    GfxProbe(const mbgl::gfx::RenderingStats&);
+
+    int numDrawCalls = 0;


nit: no need = 0

This will lead to uninitialized memory, for example innocent looking initialization GfxProbe probe; will result in undefined behavior.

Of course GfxProbes could be initialized like GfxProbe probe {}; but that's really dangerous and error prone code imho.

GfxProbe() = default; shall solve it for you

I solved the issue for now by properly using aggregate initialization GfxProbe probe {} instead of more traditional GfxProbe probe; initialization

pozdnyakov · 2019-10-23T11:43:23Z

render-test/runner.cpp

@@ -34,6 +34,27 @@

 using namespace mbgl;

+GfxProbe::GfxProbe(const mbgl::gfx::RenderingStats& stats) {
+    numBuffers = stats.numBuffers;


nit: use init list

pozdnyakov · 2019-10-23T11:51:18Z

render-test/runner.cpp

+                         const char* msg, const std::string& name, const auto& expected, const auto& actual) -> bool {
+            if (expected != actual) {
+                char msgBuf[512];
+                snprintf(msgBuf, sizeof(msgBuf), msg, name.c_str(), actual, expected);


could std::stringstream be used instead?

I think the c-style printf formatting makes the code more readable (in this case) for the caller.

pozdnyakov · 2019-10-23T11:53:51Z

src/mbgl/gl/context.cpp

@@ -206,10 +207,12 @@ UniqueTexture Context::createUniqueTexture() {
    if (pooledTextures.empty()) {
        pooledTextures.resize(TextureMax);
        MBGL_CHECK_ERROR(glGenTextures(TextureMax, pooledTextures.data()));
+        stats.numCreatedTextures += TextureMax;


could this statistics population affect the performance? shall we collect it conditionally, e.g. for debug builds only? @alexshalamov @astojilj

Incrementing one integer value wont have any visible effect on performance :)

true, but I mean the whole stats collecting in general, e.g. std::chrono::steady_clock::now() might not be that lightweight

I think it's good idea to collect general rendering stats (in gl::Context). Frame time tracking in headless frontend could be collected conditionally, but impact on performance is neglectable nevertheless.

alexshalamov · 2019-10-23T11:54:07Z

render-test/runner.cpp

+        auto check = [&metadata](
+                         const char* msg, const std::string& name, const auto& expected, const auto& actual) -> bool {
+            if (expected != actual) {
+                char msgBuf[512];


maybe use string stream instead of hardcoded buffer?

Unfortunately snprintf requires a fixed buffer. C-style formatting is used here for more readable code.

alexshalamov · 2019-10-23T11:56:12Z

render-test/runner.cpp

+        const auto& expectedValue = expected.second;
+        const auto& actualValue = actual->second;
+
+        if (!check("Number of draw calls at probe \"%s\" is %i, expected is %i",


I think it would be better to check all probes and return 'false' if one of probe fails. Also, are all expected values (probes) mandatory?

Very good point!

pozdnyakov · 2019-10-23T19:37:28Z

src/mbgl/gl/context.cpp

@@ -647,6 +666,7 @@ void Context::performCleanup() {
            }
        }
        MBGL_CHECK_ERROR(glDeleteBuffers(int(abandonedBuffers.size()), abandonedBuffers.data()));
+        stats.numBuffers -= (int)abandonedBuffers.size();


nit: better int(..) or static_cast()

pozdnyakov · 2019-10-28T10:52:37Z

render-test/runner.cpp

+            return false;
+        }
+
+        auto check = [&metadata](


is this still needed?

Good catch, not anymore!

pozdnyakov · 2019-10-28T10:52:51Z

render-test/runner.cpp

+            std::stringstream ss;
+            ss << "Number of textures at probe \"" << probeName << "\" is " << actualValue.numTextures
+               << ", expected is " << expectedValue.numTextures;
+            if (metadata.errorMessage.length()) ss << "\n";


pozdnyakov

lgtm % comments

pozdnyakov · 2019-10-29T09:35:40Z

render-test/runner.cpp

+            std::stringstream ss;
+            ss << "Number of textures at probe \"" << probeName << "\" is " << actualValue.numTextures
+               << ", expected is " << expectedValue.numTextures;
+            if (metadata.errorMessage.length()) ss << std::endl;


should it be checked before adding the new error message?

All failed test cases of a probe are reported in a single message. Should this behavior be changed?

I mean, should std::endl be added first i.e. as a separator between the different probe reports?

if (!metadata.errorMessage.empty()) ss << std::endl; ss << "Number of textures at probe \"" << probeName << "\" is " << actualValue.numTextures << ", expected is " << expectedValue.numTextures;

True, metadata is shared between different operations.

pozdnyakov · 2019-10-29T09:36:19Z

render-test/runner.cpp

+            std::stringstream ss;
+            ss << "Number of textures at probe \"" << probeName << "\" is " << actualValue.numTextures
+               << ", expected is " << expectedValue.numTextures;
+            if (metadata.errorMessage.length()) ss << std::endl;


nit: !metadata.errorMessage.empty()

mpulkki-mapbox force-pushed the mpulkki-gfx-probe branch 4 times, most recently from 9df645f to ef1fca5 Compare October 23, 2019 11:08

mpulkki-mapbox marked this pull request as ready for review October 23, 2019 11:13

mpulkki-mapbox requested review from tmpsantos and pozdnyakov October 23, 2019 11:14

pozdnyakov assigned mpulkki-mapbox Oct 23, 2019

pozdnyakov added the tests label Oct 23, 2019

mpulkki-mapbox added the needs review label Oct 23, 2019

pozdnyakov reviewed Oct 23, 2019

View reviewed changes

alexshalamov reviewed Oct 23, 2019

View reviewed changes

mpulkki-mapbox force-pushed the mpulkki-gfx-probe branch 4 times, most recently from e4278e1 to de9bfb1 Compare October 24, 2019 14:10

pozdnyakov reviewed Oct 28, 2019

View reviewed changes

mpulkki-mapbox force-pushed the mpulkki-gfx-probe branch from de9bfb1 to d641af1 Compare October 29, 2019 09:27

pozdnyakov approved these changes Oct 29, 2019

View reviewed changes

mpulkki-mapbox force-pushed the mpulkki-gfx-probe branch 3 times, most recently from 3b8d19f to 192fcc5 Compare October 29, 2019 13:31

alexshalamov approved these changes Oct 30, 2019

View reviewed changes

mpulkki-mapbox added 3 commits October 30, 2019 13:23

[render-test] Add resource usage tracking to gfx context

06e5550

[render-test] Implement gfx probe for tracking gpu resources

08fd2ad

[render-test] Add gfx probe test cases

0cc9452

mpulkki-mapbox force-pushed the mpulkki-gfx-probe branch from 192fcc5 to 0cc9452 Compare October 30, 2019 11:35

mpulkki-mapbox merged commit 6900ac9 into master Oct 30, 2019

mpulkki-mapbox deleted the mpulkki-gfx-probe branch October 30, 2019 12:32

		@@ -48,7 +54,7 @@ class HeadlessFrontend : public RendererFrontend {
		LatLng latLngForPixel(const ScreenCoordinate&);

		PremultipliedImage readStillImage();

[render-test] Add gfx probe for tracking gpu resource usage #15843

[render-test] Add gfx probe for tracking gpu resource usage #15843

Conversation

mpulkki-mapbox commented Oct 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpulkki-mapbox Oct 23, 2019 • edited Loading

Choose a reason for hiding this comment

mpulkki-mapbox Oct 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexshalamov Oct 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pozdnyakov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpulkki-mapbox Oct 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpulkki-mapbox Oct 23, 2019 •

edited

Loading

mpulkki-mapbox Oct 23, 2019 •

edited

Loading

alexshalamov Oct 23, 2019 •

edited

Loading

mpulkki-mapbox Oct 29, 2019 •

edited

Loading