[Mesa-dev] [PATCH] mesa: detect inefficient buffer use and report through debug output

[Jose Fonseca]

On 08/12/15 01:42, Brian Paul wrote:
> When a buffer is created with GL_STATIC_DRAW, its contents should not
> be changed frequently.  But that's exactly what one application I'm
> debugging does.  This patch adds code to try to detect inefficient
> buffer use in a couple places.  The GL_ARB_debug_output mechanism is
> used to report the issue.
>
> NVIDIA's driver detects these sort of things too.
>
> Other types of inefficient buffer use could also be detected in the
> future.
> ---
>   src/mesa/main/bufferobj.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++
>   src/mesa/main/mtypes.h    |  4 ++++
>   2 files changed, 59 insertions(+)
>
> diff --git a/src/mesa/main/bufferobj.c b/src/mesa/main/bufferobj.c
> index f985982..6bc1b5e 100644
> --- a/src/mesa/main/bufferobj.c
> +++ b/src/mesa/main/bufferobj.c
> @@ -51,6 +51,34 @@
>
>
>   /**
> + * We count the number of buffer modification calls to check for
> + * inefficient buffer use.  This is the number of such calls before we
> + * issue a warning.
> + */
> +#define BUFFER_WARNING_CALL_COUNT 4
> +
> +
> +/**
> + * Helper to warn of possible performance issues, such as frequently
> + * updating a buffer created with GL_STATIC_DRAW.
> + */
> +static void
> +buffer_usage_warning(struct gl_context *ctx, const char *fmt, ...)
> +{
> +   va_list args;
> +   GLuint msg_id = 0;
> +
> +   va_start(args, fmt);
> +   _mesa_gl_vdebug(ctx, &msg_id,
> +                   MESA_DEBUG_SOURCE_API,
> +                   MESA_DEBUG_TYPE_PERFORMANCE,
> +                   MESA_DEBUG_SEVERITY_MEDIUM,
> +                   fmt, args);
> +   va_end(args);
> +}
> +
> +
> +/**
>    * Used as a placeholder for buffer objects between glGenBuffers() and
>    * glBindBuffer() so that glIsBuffer() can work correctly.
>    */
> @@ -1677,6 +1705,21 @@ _mesa_buffer_sub_data(struct gl_context *ctx, struct gl_buffer_object *bufObj,
>      if (size == 0)
>         return;
>
> +   bufObj->NumSubDataCalls++;
> +
> +   if ((bufObj->Usage == GL_STATIC_DRAW ||
> +        bufObj->Usage == GL_STATIC_COPY) &&
> +       bufObj->NumSubDataCalls >= BUFFER_WARNING_CALL_COUNT) {
> +      /* If the application declared the buffer as static draw/copy or stream
> +       * draw, it should not be frequently modified with glBufferSubData.
> +       */
> +      buffer_usage_warning(ctx,
> +                           "using %s(buffer %u, offset %u, size %u) to "
> +                           "update a %s buffer",
> +                           func, bufObj->Name, offset, size,
> +                           _mesa_enum_to_string(bufObj->Usage));
> +   }
> +
>      bufObj->Written = GL_TRUE;
>
>      assert(ctx->Driver.BufferSubData);
> @@ -2384,6 +2427,18 @@ _mesa_map_buffer_range(struct gl_context *ctx,
>         return NULL;
>      }
>
> +   if (access & GL_MAP_WRITE_BIT) {
> +      bufObj->NumMapBufferWriteCalls++;
> +      if ((bufObj->Usage == GL_STATIC_DRAW ||
> +           bufObj->Usage == GL_STATIC_COPY) &&
> +          bufObj->NumMapBufferWriteCalls >= BUFFER_WARNING_CALL_COUNT) {
> +         buffer_usage_warning(ctx,
> +                              "using %s(buffer %u, offset %u, length %u) to "
> +                              "update a %s buffer",
> +                              func, bufObj->Name, offset, length,
> +                              _mesa_enum_to_string(bufObj->Usage));
> +      }
> +   }
>
>      assert(ctx->Driver.MapBufferRange);
>      map = ctx->Driver.MapBufferRange(ctx, offset, length, access, bufObj,
> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> index 1eb1e21..de54169 100644
> --- a/src/mesa/main/mtypes.h
> +++ b/src/mesa/main/mtypes.h
> @@ -1275,6 +1275,10 @@ struct gl_buffer_object
>      GLboolean Immutable; /**< GL_ARB_buffer_storage */
>      gl_buffer_usage UsageHistory; /**< How has this buffer been used so far? */
>
> +   /** Counters used for buffer usage warnings */
> +   GLuint NumSubDataCalls;
> +   GLuint NumMapBufferWriteCalls;
> +
>      struct gl_buffer_mapping Mappings[MAP_COUNT];
>   };
>
>

LGTM.

Reviewed-by: Jose Fonseca <jfonseca at vmware.com>