[Mesa-dev] [PATCH] mesa/glthread: Avoid unnecessary batch reallocation

[Nicolai Hähnle]

On 03.04.2017 20:53, Bartosz Tomczyk wrote:
> Actually, I can just set only batch->used to 0, but it seems to error
> prone. When someone adds some fields to batch struct, it will be easy to
> miss that it should be initialized in glthread_unmarshal_batch.

Better to have it fail early and loudly with garbage data, rather than 
silently if 0 happens to be an accepted value. I think it should be 
changed (unless there's a good reason to rely on it on purpose, but I 
don't see one...).

Cheers,
Nicolai


>
> Anyway I can change it if you want to.
>
> On Mon, Apr 3, 2017 at 8:42 PM, Nicolai Hähnle <nhaehnle at gmail.com
> <mailto:nhaehnle at gmail.com>> wrote:
>
>     On 03.04.2017 20:38, Bartosz Tomczyk wrote:
>
>         ---
>          src/mesa/main/glthread.c | 15 +++++++++------
>          1 file changed, 9 insertions(+), 6 deletions(-)
>
>         diff --git a/src/mesa/main/glthread.c b/src/mesa/main/glthread.c
>         index 3f07c420d4..aa14292e59 100644
>         --- a/src/mesa/main/glthread.c
>         +++ b/src/mesa/main/glthread.c
>         @@ -53,7 +53,8 @@ glthread_allocate_batch(struct gl_context *ctx)
>          }
>
>          static void
>         -glthread_unmarshal_batch(struct gl_context *ctx, struct
>         glthread_batch *batch)
>         +glthread_unmarshal_batch(struct gl_context *ctx, struct
>         glthread_batch *batch,
>         +                         const bool release_batch)
>          {
>             size_t pos = 0;
>
>         @@ -64,7 +65,10 @@ glthread_unmarshal_batch(struct gl_context
>         *ctx, struct glthread_batch *batch)
>
>             assert(pos == batch->used);
>
>         -   free(batch);
>         +   if (release_batch)
>         +      free(batch);
>         +   else
>         +      memset(batch, 0, offsetof(struct glthread_batch, buffer));
>
>
>     Hmm. Why do we memset the batch? Seems like a trivial optimization
>     to just not do that...
>
>     Apart from that, the patch looks good to me.
>
>     Cheers,
>     Nicolai
>
>
>
>          }
>
>          static void *
>         @@ -103,7 +107,7 @@ glthread_worker(void *data)
>                glthread->busy = true;
>                pthread_mutex_unlock(&glthread->mutex);
>
>         -      glthread_unmarshal_batch(ctx, batch);
>         +      glthread_unmarshal_batch(ctx, batch, true);
>
>                pthread_mutex_lock(&glthread->mutex);
>                glthread->busy = false;
>         @@ -214,7 +218,7 @@ _mesa_glthread_flush_batch_locked(struct
>         gl_context *ctx)
>              * need to restore it when it returns.
>              */
>             if (false) {
>         -      glthread_unmarshal_batch(ctx, batch);
>         +      glthread_unmarshal_batch(ctx, batch, true);
>                _glapi_set_dispatch(ctx->CurrentClientDispatch);
>                return;
>             }
>         @@ -269,9 +273,8 @@ _mesa_glthread_finish(struct gl_context *ctx)
>             if (!(glthread->batch_queue || glthread->busy)) {
>                if (glthread->batch && glthread->batch->used) {
>                   struct _glapi_table *dispatch = _glapi_get_dispatch();
>         -         glthread_unmarshal_batch(ctx, glthread->batch);
>         +         glthread_unmarshal_batch(ctx, glthread->batch, false);
>                   _glapi_set_dispatch(dispatch);
>         -         glthread_allocate_batch(ctx);
>                }
>             } else {
>                _mesa_glthread_flush_batch_locked(ctx);
>
>
>
>     --
>     Lerne, wie die Welt wirklich ist,
>     Aber vergiss niemals, wie sie sein sollte.
>
>


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.