[Mesa-dev] [PATCHv3 05/16] util: add a generic thread pool data structure

[Chia-I Wu]

On Thu, Aug 21, 2014 at 9:31 AM, Matt Turner <mattst88 at gmail.com> wrote:
> On Tue, Aug 19, 2014 at 11:40 PM, Chia-I Wu <olvaffe at gmail.com> wrote:
>> It can be used to implement, for example, threaded glCompileShader and
>> glLinkProgram.  Two basic tests are included to verify the basic functions,
>> and to give us some confidence about its thread-safety.
>>
>> v2: allow tasks to "complete" other tasks
>>
>> Signed-off-by: Chia-I Wu <olv at lunarg.com>
>> Reviewed-by: Brian Paul <brianp at vmware.com>
>>
>> v3: move to src/util/ and mention the tests
>> ---
>>  configure.ac                                  |   3 +-
>>  src/util/Makefile.am                          |   5 +-
>>  src/util/Makefile.sources                     |   3 +-
>>  src/util/tests/threadpool/Makefile.am         |  36 ++
>>  src/util/tests/threadpool/threadpool_test.cpp | 137 ++++++++
>>  src/util/threadpool.c                         | 476 ++++++++++++++++++++++++++
>>  src/util/threadpool.h                         |  67 ++++
>>  7 files changed, 724 insertions(+), 3 deletions(-)
>>  create mode 100644 src/util/tests/threadpool/Makefile.am
>>  create mode 100644 src/util/tests/threadpool/threadpool_test.cpp
>>  create mode 100644 src/util/threadpool.c
>>  create mode 100644 src/util/threadpool.h
>>
>> diff --git a/configure.ac b/configure.ac
>> index 57e9f7d..2f7268f 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -2261,7 +2261,8 @@ AC_CONFIG_FILES([Makefile
>>                 src/mesa/drivers/x11/Makefile
>>                 src/mesa/main/tests/Makefile
>>                 src/util/Makefile
>> -               src/util/tests/hash_table/Makefile])
>> +               src/util/tests/hash_table/Makefile
>> +               src/util/tests/threadpool/Makefile])
>>
>>  dnl Sort the dirs alphabetically
>>  GALLIUM_TARGET_DIRS=`echo $GALLIUM_TARGET_DIRS|tr " " "\n"|sort -u|tr "\n" " "`
>> diff --git a/src/util/Makefile.am b/src/util/Makefile.am
>> index 4733a1a..da6815e 100644
>> --- a/src/util/Makefile.am
>> +++ b/src/util/Makefile.am
>> @@ -19,7 +19,7 @@
>>  # FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
>>  # IN THE SOFTWARE.
>>
>> -SUBDIRS = . tests/hash_table
>> +SUBDIRS = . tests/hash_table tests/threadpool
>>
>>  include Makefile.sources
>>
>> @@ -34,6 +34,9 @@ libmesautil_la_SOURCES = \
>>         $(MESA_UTIL_FILES) \
>>         $(MESA_UTIL_GENERATED_FILES)
>>
>> +libmesautil_la_LIBADD = \
>> +       $(PTHREAD_LIBS)
>
> You want to link the test programs with pthreads, not the convenience
> library. Add PTHREAD_CFLAGS to the target's CFLAGS, and likewise for
> PTHREAD_LIBS (which it looks like you're doing below, so throw this
> hunk out?)
I think it can be thrown out.  I added $(PTHREAD_LIBS) to the
convenience library so that all targets would be automatically linked
with pthread.  But since this is a base utility library, and only
those use thread pool would need pthread, I can add pthread to all GL
targets (which should already be the case).

>
>> +
>>  BUILT_SOURCES = $(MESA_UTIL_GENERATED_FILES)
>>  CLEANFILES = $(BUILT_SOURCES)
>>
>> diff --git a/src/util/Makefile.sources b/src/util/Makefile.sources
>> index 86466dc..65f98f7 100644
>> --- a/src/util/Makefile.sources
>> +++ b/src/util/Makefile.sources
>> @@ -1,7 +1,8 @@
>>  MESA_UTIL_FILES :=     \
>>         hash_table.c    \
>>         ralloc.c \
>> -       strtod.cpp
>> +       strtod.cpp \
>> +       threadpool.c
>>
>>  MESA_UTIL_GENERATED_FILES = \
>>         format_srgb.c
>> diff --git a/src/util/tests/threadpool/Makefile.am b/src/util/tests/threadpool/Makefile.am
>> new file mode 100644
>> index 0000000..2aa010c
>> --- /dev/null
>> +++ b/src/util/tests/threadpool/Makefile.am
>> @@ -0,0 +1,36 @@
>> +# Copyright © 2009 Intel Corporation
>
> Seems wrong.
>
>> +#
>> +#  Permission is hereby granted, free of charge, to any person obtaining a
>> +#  copy of this software and associated documentation files (the "Software"),
>> +#  to deal in the Software without restriction, including without limitation
>> +#  on the rights to use, copy, modify, merge, publish, distribute, sub
>> +#  license, and/or sell copies of the Software, and to permit persons to whom
>> +#  the Software is furnished to do so, subject to the following conditions:
>> +#
>> +#  The above copyright notice and this permission notice (including the next
>> +#  paragraph) shall be included in all copies or substantial portions of the
>> +#  Software.
>> +#
>> +#  THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> +#  IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> +#  FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.  IN NO EVENT SHALL
>> +#  ADAM JACKSON BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER
>
> Adam Jackson?
That was copied from src/util/tests/hash_table/Makefile.am.  I will
update the copyright header.

>
> Otherwise, the build system stuff looks okay. Just fix this stuff
> before you commit.

-- 
olv at LunarG.com