On 2017-07-01 18:46, Marek Olšák wrote:
Instead of passing the function pointer through the queue, passing
just a call ID (uint16_t) is preferable.

If the switch statement is an issue, doing a function pointer lookup
from a static array should be OK.


OK, then let's drop this patch. gcc turns the switch/case block into an efficient jump table with the ID method, so an array for function lookup instead of that doesn't improve anything. I didn't see any measurable benefit of the function pointer method either.

Best regards
Grigori


On Fri, Jun 30, 2017 at 7:14 PM, Grigori Goronzy <g...@chown.ath.cx> wrote:
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)
  {

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to