[Mesa-dev] [PATCH] glthread: get rid of unmarshal dispatch enum/table
Grigori Goronzy
greg at chown.ath.cx
Fri Jun 30 17:14:55 UTC 2017
On 2017-06-30 15:27, Nicolai Hähnle wrote:
> On 30.06.2017 02:29, Grigori Goronzy wrote:
>> Use function pointers to identify the unmarshalling function, which
>> is simpler and gets rid of a lot generated code.
>>
>> This removes an indirection and possibly results in a slight speedup
>> as well.
>
> The fact that it blows up cmd_base from 4 bytes to 16 bytes might
> result in a slowdown. Marek's recent changes clearly indicated that
> looking at memory behavior matters quite a bit for glthread. So I'm
> inclined to say No on this unless you can demonstrate a consistent
> speedup.
>
That's indeed a notable difference. I suspect it isn't so much the byte
size of the marshalled commands that affects throughput, but the number
of commands per batch and their associated costs when unmarshalling, so
the larger size of cmd_base might not matter much (perhaps with adjusted
max batch size). In any case, I'll try get hold of some numbers.
Best regards
Grigori
> Cheers,
> Nicolai
>
>
>> ---
>> src/mapi/glapi/gen/Makefile.am | 4 --
>> src/mapi/glapi/gen/gl_marshal.py | 36 ++--------------
>> src/mapi/glapi/gen/gl_marshal_h.py | 86
>> --------------------------------------
>> src/mesa/Android.gen.mk | 7 ----
>> src/mesa/Makefile.sources | 1 -
>> src/mesa/SConscript | 8 ----
>> src/mesa/main/.gitignore | 1 -
>> src/mesa/main/glthread.c | 9 +++-
>> src/mesa/main/glthread.h | 2 -
>> src/mesa/main/marshal.c | 19 ++++-----
>> src/mesa/main/marshal.h | 14 +++----
>> 11 files changed, 26 insertions(+), 161 deletions(-)
>> delete mode 100644 src/mapi/glapi/gen/gl_marshal_h.py
>>
>> diff --git a/src/mapi/glapi/gen/Makefile.am
>> b/src/mapi/glapi/gen/Makefile.am
>> index bd04519..62007a4 100644
>> --- a/src/mapi/glapi/gen/Makefile.am
>> +++ b/src/mapi/glapi/gen/Makefile.am
>> @@ -76,7 +76,6 @@ EXTRA_DIST= \
>> gl_genexec.py \
>> gl_gentable.py \
>> gl_marshal.py \
>> - gl_marshal_h.py \
>> gl_procs.py \
>> gl_SPARC_asm.py \
>> gl_table.py \
>> @@ -297,9 +296,6 @@ $(MESA_DIR)/main/api_exec.c: gl_genexec.py
>> apiexec.py $(COMMON)
>> $(MESA_DIR)/main/marshal_generated.c: gl_marshal.py marshal_XML.py
>> $(COMMON)
>> $(PYTHON_GEN) $(srcdir)/gl_marshal.py -f
>> $(srcdir)/gl_and_es_API.xml > $@
>> -$(MESA_DIR)/main/marshal_generated.h: gl_marshal_h.py
>> marshal_XML.py $(COMMON)
>> - $(PYTHON_GEN) $(srcdir)/gl_marshal_h.py -f
>> $(srcdir)/gl_and_es_API.xml > $@
>> -
>> $(MESA_DIR)/main/dispatch.h: gl_table.py $(COMMON)
>> $(PYTHON_GEN) $(srcdir)/gl_table.py -f $(srcdir)/gl_and_es_API.xml
>> -m remap_table > $@
>> diff --git a/src/mapi/glapi/gen/gl_marshal.py
>> b/src/mapi/glapi/gen/gl_marshal.py
>> index efa4d9e..e71ede3 100644
>> --- a/src/mapi/glapi/gen/gl_marshal.py
>> +++ b/src/mapi/glapi/gen/gl_marshal.py
>> @@ -34,7 +34,6 @@ header = """
>> #include "dispatch.h"
>> #include "glthread.h"
>> #include "marshal.h"
>> -#include "marshal_generated.h"
>> """
>> @@ -106,7 +105,7 @@ class PrintCode(gl_XML.gl_print_base):
>> def print_async_dispatch(self, func):
>> out('cmd = _mesa_glthread_allocate_command(ctx, '
>> - 'DISPATCH_CMD_{0}, cmd_size);'.format(func.name))
>> + '(unmarshal_func)_mesa_unmarshal_{0},
>> cmd_size);'.format(func.name))
>> for p in func.fixed_params:
>> if p.count:
>> out('memcpy(cmd->{0}, {0}, {1});'.format(
>> @@ -166,7 +165,7 @@ class PrintCode(gl_XML.gl_print_base):
>> out('};')
>> def print_async_unmarshal(self, func):
>> - out('static inline void')
>> + out('static void')
>> out(('_mesa_unmarshal_{0}(struct gl_context *ctx, '
>> 'const struct marshal_cmd_{0}
>> *cmd)').format(func.name))
>> out('{')
>> @@ -205,6 +204,7 @@ class PrintCode(gl_XML.gl_print_base):
>> else:
>> out('variable_data +=
>> {0};'.format(p.size_string(False)))
>> + out('debug_print_unmarshal("{0}");'.format(func.name))
>> self.print_sync_call(func)
>> out('}')
>> @@ -276,35 +276,6 @@ class PrintCode(gl_XML.gl_print_base):
>> out('')
>> out('')
>> - def print_unmarshal_dispatch_cmd(self, api):
>> - out('size_t')
>> - out('_mesa_unmarshal_dispatch_cmd(struct gl_context *ctx, '
>> - 'const void *cmd)')
>> - out('{')
>> - with indent():
>> - out('const struct marshal_cmd_base *cmd_base = cmd;')
>> - out('switch (cmd_base->cmd_id) {')
>> - for func in api.functionIterateAll():
>> - flavor = func.marshal_flavor()
>> - if flavor in ('skip', 'sync'):
>> - continue
>> - out('case DISPATCH_CMD_{0}:'.format(func.name))
>> - with indent():
>> -
>> out('debug_print_unmarshal("{0}");'.format(func.name))
>> - out(('_mesa_unmarshal_{0}(ctx, (const struct
>> marshal_cmd_{0} *)'
>> - ' cmd);').format(func.name))
>> - out('break;')
>> - out('default:')
>> - with indent():
>> - out('assert(!"Unrecognized command ID");')
>> - out('break;')
>> - out('}')
>> - out('')
>> - out('return cmd_base->cmd_size;')
>> - out('}')
>> - out('')
>> - out('')
>> -
>> def print_create_marshal_table(self, api):
>> out('struct _glapi_table *')
>> out('_mesa_create_marshal_table(const struct gl_context
>> *ctx)')
>> @@ -338,7 +309,6 @@ class PrintCode(gl_XML.gl_print_base):
>> async_funcs.append(func)
>> elif flavor == 'sync':
>> self.print_sync_body(func)
>> - self.print_unmarshal_dispatch_cmd(api)
>> self.print_create_marshal_table(api)
>> diff --git a/src/mapi/glapi/gen/gl_marshal_h.py
>> b/src/mapi/glapi/gen/gl_marshal_h.py
>> deleted file mode 100644
>> index 6e39148..0000000
>> --- a/src/mapi/glapi/gen/gl_marshal_h.py
>> +++ /dev/null
>> @@ -1,86 +0,0 @@
>> -#!/usr/bin/env python
>> -
>> -# Copyright (C) 2012 Intel Corporation
>> -#
>> -# 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
>> -# the rights to use, copy, modify, merge, publish, distribute,
>> sublicense,
>> -# 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 NONINFRINGEMENT. IN NO EVENT
>> SHALL
>> -# THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
>> OR OTHER
>> -# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> ARISING
>> -# FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>> DEALINGS
>> -# IN THE SOFTWARE.
>> -
>> -import getopt
>> -import gl_XML
>> -import license
>> -import marshal_XML
>> -import sys
>> -
>> -
>> -header = """
>> -#ifndef MARSHAL_GENERATABLE_H
>> -#define MARSHAL_GENERATABLE_H
>> -"""
>> -
>> -footer = """
>> -#endif /* MARSHAL_GENERATABLE_H */
>> -"""
>> -
>> -
>> -class PrintCode(gl_XML.gl_print_base):
>> - def __init__(self):
>> - super(PrintCode, self).__init__()
>> -
>> - self.name = 'gl_marshal_h.py'
>> - self.license = license.bsd_license_template % (
>> - 'Copyright (C) 2012 Intel Corporation', 'INTEL
>> CORPORATION')
>> -
>> - def printRealHeader(self):
>> - print header
>> -
>> - def printRealFooter(self):
>> - print footer
>> -
>> - def printBody(self, api):
>> - print 'enum marshal_dispatch_cmd_id'
>> - print '{'
>> - for func in api.functionIterateAll():
>> - flavor = func.marshal_flavor()
>> - if flavor in ('skip', 'sync'):
>> - continue
>> - print ' DISPATCH_CMD_{0},'.format(func.name)
>> - print '};'
>> -
>> -
>> -def show_usage():
>> - print 'Usage: %s [-f input_file_name]' % sys.argv[0]
>> - sys.exit(1)
>> -
>> -
>> -if __name__ == '__main__':
>> - file_name = 'gl_API.xml'
>> -
>> - try:
>> - (args, trail) = getopt.getopt(sys.argv[1:], 'm:f:')
>> - except Exception,e:
>> - show_usage()
>> -
>> - for (arg,val) in args:
>> - if arg == '-f':
>> - file_name = val
>> -
>> - printer = PrintCode()
>> -
>> - api = gl_XML.parse_GL_API(file_name,
>> marshal_XML.marshal_item_factory())
>> - printer.Print(api)
>> diff --git a/src/mesa/Android.gen.mk b/src/mesa/Android.gen.mk
>> index ee2d1de..691fb3b 100644
>> --- a/src/mesa/Android.gen.mk
>> +++ b/src/mesa/Android.gen.mk
>> @@ -41,7 +41,6 @@ sources := \
>> main/remap_helper.h \
>> main/get_hash.h \
>> main/marshal_generated.c \
>> - main/marshal_generated.h
>> LOCAL_SRC_FILES := $(filter-out $(sources), $(LOCAL_SRC_FILES))
>> @@ -110,12 +109,6 @@ $(intermediates)/main/marshal_generated.c:
>> PRIVATE_XML := -f $(glapi)/gl_and_es_
>> $(intermediates)/main/marshal_generated.c: $(dispatch_deps)
>> $(call es-gen)
>> -$(intermediates)/main/marshal_generated.h: PRIVATE_SCRIPT :=
>> $(MESA_PYTHON2) $(glapi)/gl_marshal_h.py
>> -$(intermediates)/main/marshal_generated.h: PRIVATE_XML := -f
>> $(glapi)/gl_and_es_API.xml
>> -
>> -$(intermediates)/main/marshal_generated.h: $(dispatch_deps)
>> - $(call es-gen)
>> -
>> GET_HASH_GEN := $(LOCAL_PATH)/main/get_hash_generator.py
>> $(intermediates)/main/get_hash.h: PRIVATE_SCRIPT :=
>> $(MESA_PYTHON2) $(GET_HASH_GEN)
>> diff --git a/src/mesa/Makefile.sources b/src/mesa/Makefile.sources
>> index 86fbf39..e9b1b82 100644
>> --- a/src/mesa/Makefile.sources
>> +++ b/src/mesa/Makefile.sources
>> @@ -137,7 +137,6 @@ MAIN_FILES = \
>> main/marshal.c \
>> main/marshal.h \
>> main/marshal_generated.c \
>> - main/marshal_generated.h \
>> main/matrix.c \
>> main/matrix.h \
>> main/mipmap.c \
>> diff --git a/src/mesa/SConscript b/src/mesa/SConscript
>> index b63e15a..27bcb85 100644
>> --- a/src/mesa/SConscript
>> +++ b/src/mesa/SConscript
>> @@ -133,14 +133,6 @@ env.CodeGenerate(
>> command = python_cmd + ' $SCRIPT -f $SOURCE > $TARGET'
>> )
>> -# The marshal_generated.h file is generated from the GL/ES API.xml
>> file
>> -env.CodeGenerate(
>> - target = 'main/marshal_generated.h',
>> - script = GLAPI + 'gen/gl_marshal_h.py',
>> - source = [GLAPI + 'gen/gl_and_es_API.xml'] + env.Glob(GLAPI +
>> 'gen/*.xml'),
>> - command = python_cmd + ' $SCRIPT -f $SOURCE > $TARGET'
>> - )
>> -
>> #
>> # Libraries
>> #
>> diff --git a/src/mesa/main/.gitignore b/src/mesa/main/.gitignore
>> index 8cc33cf..31dae60 100644
>> --- a/src/mesa/main/.gitignore
>> +++ b/src/mesa/main/.gitignore
>> @@ -10,4 +10,3 @@ format_info.c
>> format_pack.c
>> format_unpack.c
>> marshal_generated.c
>> -marshal_generated.h
>> diff --git a/src/mesa/main/glthread.c b/src/mesa/main/glthread.c
>> index c71c037..8c7a8e0 100644
>> --- a/src/mesa/main/glthread.c
>> +++ b/src/mesa/main/glthread.c
>> @@ -35,10 +35,15 @@
>> #include "main/mtypes.h"
>> #include "main/glthread.h"
>> #include "main/marshal.h"
>> -#include "main/marshal_generated.h"
>> #include "util/u_atomic.h"
>> #include "util/u_thread.h"
>> +static size_t glthread_unmarshal_cmd(struct gl_context *ctx, const
>> void *cmd)
>> +{
>> + const struct marshal_cmd_base *cmd_base = (struct
>> marshal_cmd_base *)cmd;
>> + cmd_base->cmd_func(ctx, cmd);
>> + return cmd_base->cmd_size;
>> +}
>> static void
>> glthread_unmarshal_batch(void *job, int thread_index)
>> @@ -50,7 +55,7 @@ glthread_unmarshal_batch(void *job, int
>> thread_index)
>> _glapi_set_dispatch(ctx->CurrentServerDispatch);
>> while (pos < batch->used)
>> - pos += _mesa_unmarshal_dispatch_cmd(ctx, &batch->buffer[pos]);
>> + pos += glthread_unmarshal_cmd(ctx, &batch->buffer[pos]);
>> assert(pos == batch->used);
>> batch->used = 0;
>> diff --git a/src/mesa/main/glthread.h b/src/mesa/main/glthread.h
>> index 306246c..07bccba 100644
>> --- a/src/mesa/main/glthread.h
>> +++ b/src/mesa/main/glthread.h
>> @@ -49,8 +49,6 @@
>> #include <stdbool.h>
>> #include "util/u_queue.h"
>> -enum marshal_dispatch_cmd_id;
>> -
>> /** A single batch of commands queued up for execution. */
>> struct glthread_batch
>> {
>> diff --git a/src/mesa/main/marshal.c b/src/mesa/main/marshal.c
>> index 8db4531..6c5c687 100644
>> --- a/src/mesa/main/marshal.c
>> +++ b/src/mesa/main/marshal.c
>> @@ -31,7 +31,6 @@
>> #include "main/macros.h"
>> #include "marshal.h"
>> #include "dispatch.h"
>> -#include "marshal_generated.h"
>> struct marshal_cmd_Flush
>> {
>> @@ -52,7 +51,7 @@ _mesa_marshal_Flush(void)
>> {
>> GET_CURRENT_CONTEXT(ctx);
>> struct marshal_cmd_Flush *cmd =
>> - _mesa_glthread_allocate_command(ctx, DISPATCH_CMD_Flush,
>> + _mesa_glthread_allocate_command(ctx,
>> (unmarshal_func)_mesa_unmarshal_Flush,
>> sizeof(struct
>> marshal_cmd_Flush));
>> (void) cmd;
>> _mesa_post_marshal_hook(ctx);
>> @@ -91,7 +90,7 @@ _mesa_marshal_Enable(GLenum cap)
>> _mesa_glthread_finish(ctx);
>> _mesa_glthread_restore_dispatch(ctx);
>> } else {
>> - cmd = _mesa_glthread_allocate_command(ctx, DISPATCH_CMD_Enable,
>> + cmd = _mesa_glthread_allocate_command(ctx,
>> (unmarshal_func)_mesa_unmarshal_Enable,
>> sizeof(*cmd));
>> cmd->cap = cap;
>> _mesa_post_marshal_hook(ctx);
>> @@ -172,7 +171,7 @@ _mesa_marshal_ShaderSource(GLuint shader, GLsizei
>> count,
>> if (total_cmd_size <= MARSHAL_MAX_CMD_SIZE) {
>> struct marshal_cmd_ShaderSource *cmd =
>> - _mesa_glthread_allocate_command(ctx,
>> DISPATCH_CMD_ShaderSource,
>> + _mesa_glthread_allocate_command(ctx,
>> (unmarshal_func)_mesa_unmarshal_ShaderSource,
>> total_cmd_size);
>> GLint *cmd_length = (GLint *) (cmd + 1);
>> GLchar *cmd_strings = (GLchar *) (cmd_length + count);
>> @@ -277,7 +276,7 @@ _mesa_marshal_BindBuffer(GLenum target, GLuint
>> buffer)
>> track_vbo_binding(ctx, target, buffer);
>> if (cmd_size <= MARSHAL_MAX_CMD_SIZE) {
>> - cmd = _mesa_glthread_allocate_command(ctx,
>> DISPATCH_CMD_BindBuffer,
>> + cmd = _mesa_glthread_allocate_command(ctx,
>> (unmarshal_func)_mesa_unmarshal_BindBuffer,
>> cmd_size);
>> cmd->target = target;
>> cmd->buffer = buffer;
>> @@ -334,7 +333,7 @@ _mesa_marshal_BufferData(GLenum target, GLsizeiptr
>> size, const GLvoid * data,
>> if (target != GL_EXTERNAL_VIRTUAL_MEMORY_BUFFER_AMD &&
>> cmd_size <= MARSHAL_MAX_CMD_SIZE) {
>> struct marshal_cmd_BufferData *cmd =
>> - _mesa_glthread_allocate_command(ctx,
>> DISPATCH_CMD_BufferData,
>> + _mesa_glthread_allocate_command(ctx,
>> (unmarshal_func)_mesa_unmarshal_BufferData,
>> cmd_size);
>> cmd->target = target;
>> @@ -393,7 +392,7 @@ _mesa_marshal_BufferSubData(GLenum target,
>> GLintptr offset, GLsizeiptr size,
>> if (target != GL_EXTERNAL_VIRTUAL_MEMORY_BUFFER_AMD &&
>> cmd_size <= MARSHAL_MAX_CMD_SIZE) {
>> struct marshal_cmd_BufferSubData *cmd =
>> - _mesa_glthread_allocate_command(ctx,
>> DISPATCH_CMD_BufferSubData,
>> + _mesa_glthread_allocate_command(ctx,
>> (unmarshal_func)_mesa_unmarshal_BufferSubData,
>> cmd_size);
>> cmd->target = target;
>> cmd->offset = offset;
>> @@ -447,7 +446,7 @@ _mesa_marshal_NamedBufferData(GLuint buffer,
>> GLsizeiptr size,
>> if (buffer > 0 && cmd_size <= MARSHAL_MAX_CMD_SIZE) {
>> struct marshal_cmd_NamedBufferData *cmd =
>> - _mesa_glthread_allocate_command(ctx,
>> DISPATCH_CMD_NamedBufferData,
>> + _mesa_glthread_allocate_command(ctx,
>> (unmarshal_func)_mesa_unmarshal_NamedBufferData,
>> cmd_size);
>> cmd->name = buffer;
>> cmd->size = size;
>> @@ -501,7 +500,7 @@ _mesa_marshal_NamedBufferSubData(GLuint buffer,
>> GLintptr offset,
>> if (buffer > 0 && cmd_size <= MARSHAL_MAX_CMD_SIZE) {
>> struct marshal_cmd_NamedBufferSubData *cmd =
>> - _mesa_glthread_allocate_command(ctx,
>> DISPATCH_CMD_NamedBufferSubData,
>> + _mesa_glthread_allocate_command(ctx,
>> (unmarshal_func)_mesa_unmarshal_NamedBufferSubData,
>> cmd_size);
>> cmd->name = buffer;
>> cmd->offset = offset;
>> @@ -569,7 +568,7 @@ _mesa_marshal_ClearBufferfv(GLenum buffer, GLint
>> drawbuffer,
>> size_t cmd_size = sizeof(struct marshal_cmd_ClearBufferfv) +
>> size;
>> if (cmd_size <= MARSHAL_MAX_CMD_SIZE) {
>> struct marshal_cmd_ClearBufferfv *cmd =
>> - _mesa_glthread_allocate_command(ctx,
>> DISPATCH_CMD_ClearBufferfv,
>> + _mesa_glthread_allocate_command(ctx,
>> (unmarshal_func)_mesa_unmarshal_ClearBufferfv,
>> cmd_size);
>> cmd->buffer = buffer;
>> cmd->drawbuffer = drawbuffer;
>> diff --git a/src/mesa/main/marshal.h b/src/mesa/main/marshal.h
>> index 999c75e..02893f8 100644
>> --- a/src/mesa/main/marshal.h
>> +++ b/src/mesa/main/marshal.h
>> @@ -34,12 +34,15 @@
>> #include "main/context.h"
>> #include "main/macros.h"
>> +
>> +typedef void (*unmarshal_func)(struct gl_context *ctx, const void
>> *data);
>> +
>> struct marshal_cmd_base
>> {
>> /**
>> - * Type of command. See enum marshal_dispatch_cmd_id.
>> + * Pointer to unmarshalling function.
>> */
>> - uint16_t cmd_id;
>> + unmarshal_func cmd_func;
>> /**
>> * Size of command, in multiples of 4 bytes, including cmd_base.
>> @@ -49,7 +52,7 @@ struct marshal_cmd_base
>> static inline void *
>> _mesa_glthread_allocate_command(struct gl_context *ctx,
>> - uint16_t cmd_id,
>> + unmarshal_func func,
>> size_t size)
>> {
>> struct glthread_state *glthread = ctx->GLThread;
>> @@ -64,7 +67,7 @@ _mesa_glthread_allocate_command(struct gl_context
>> *ctx,
>> cmd_base = (struct marshal_cmd_base
>> *)&next->buffer[next->used];
>> next->used += aligned_size;
>> - cmd_base->cmd_id = cmd_id;
>> + cmd_base->cmd_func = func;
>> cmd_base->cmd_size = aligned_size;
>> return cmd_base;
>> }
>> @@ -136,9 +139,6 @@ debug_print_unmarshal(const char *func)
>> struct _glapi_table *
>> _mesa_create_marshal_table(const struct gl_context *ctx);
>> -size_t
>> -_mesa_unmarshal_dispatch_cmd(struct gl_context *ctx, const void
>> *cmd);
>> -
>> static inline void
>> _mesa_post_marshal_hook(struct gl_context *ctx)
>> {
>>
More information about the mesa-dev
mailing list