[Mesa-dev] [PATCH 00/13] mesa: Locking improvements and optimizations

[Timothy Arceri]

On Thu, 2015-08-06 at 17:10 -0700, Matt Turner wrote:
> Patches 1-11 improve performance of SynMark OglBatch7 by 6.29586% +/- 
> 0.277734%
> (n=337) and OglMultithread by 1.12564% +/- 0.424038% (n=209). I haven't
> benchmarked individual patches because I'd like to not waste all that time 
> if I
> get review feedback that requires me to change things. :)
> 
> Patches 12-13 were supposed to improve performance, but seem to make an
> existing thread-safety problem worse, so I'm not proposing them for 
> inclusion.
> 
> 
> [01/13] c11/threads: Assert that mtx is non-NULL and check
> [02/13] mesa: Remove debugging code from _mesa_reference_*.
> 

Small comment on 1 but doesn't really matter.

These two are Reviewed-by: Timothy Arceri <t_arceri at yahoo.com.au>


> [03/13] mesa: Add locking to sampler objects.
> [04/13] mesa: Add locking to programs.
> 
>    These two add missing locks to sampler and program objects, which
>    I believe are supposed to be thread-safe.

These two look correct but shouldn't you just squash these with 6 and 7?

If you really want to show the code evolution in git history then: 
Reviewed-by: Timothy Arceri <t_arceri at yahoo.com.au>

> 
> [05/13] mesa: Replace buffer object locks with atomic inc/dec.
> [06/13] mesa: Replace sampler object locks with atomic inc/dec.
> [07/13] mesa: Replace program locks with atomic inc/dec.
> [08/13] mesa: Replace renderbuffer object locks with atomic inc/dec.
> [09/13] mesa: Replace texture buffer object locks with atomic inc/dec.
> 
>    These five replace locks around RefCount++/-- with atomic increment
>    and decrement.

Nice I didn't know about this.

Reviewed-by: Timothy Arceri <t_arceri at yahoo.com.au>

> 
> [10/13] hash: Add _mesa_HashRemoveLocked() function.
> [11/13] mesa: Replace uses of Shared->Mutex with hash-table mutexes
> 
>    These two replace uses of ctx->Shared->Mutex with the mutexes in the
>    hash tables.

Nice. I was looking in this area early in the year when I noticed some common
Phoronix benchmark games spent a lot of time locking in these areas so maybe
some real work gains here.

Some minor comments on these.

Reviewed-by: Timothy Arceri <t_arceri at yahoo.com.au>

> 
> [12/13] mesa: Remove unnecessary locking from container
> [13/13] mesa: Remove deleteFlag pattern.
> 
>    *I am not proposing these for inclusion*
> 
>    These two remove some "unnecessary" locking from so called "container
>    objects" that are not shared between threads by the GL. While I expected
>    them to improve performance, they actually cause double-free errors in
>    SynMark OglMultithread. Valgrind's helgrind tool shows that there are 
> many
>    thread-safety issues in the texture code, and removing these locks seems > to
>    exacerbate the problem.
> 
>    Specifically, multiple threads are reading and writing to 
> gl_texture_objects
>    without any synchronization from places like 
> intel_finalize_mipmap_tree(),
>    gen7_update_texture_surface(), brw_populate_sampler_prog_key_data(),
>    update_sampler_state(), and _mesa_BindTexture().
> 
>    Suggestions for solving this (apparently quite longstanding) problem are
>    welcome.
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev