[Mesa-dev] [PATCH] glsl: replace old hash table with new and faster one

[Timothy Arceri]

On Mon, 2015-08-03 at 19:32 +1000, Timothy Arceri wrote:
> On Sun, 2015-08-02 at 20:39 -0400, Ilia Mirkin wrote:
> > Given that this is a debug-only thing, I doubt perf numbers are that
> > interesting.
> > 
> > I have no clue what the diff is between the two hash tables, but if
> > one is allegedly faster than the other, that should be determined, and
> > we should just mass-migrate...
> 
> The main difference is program/hash_table.h does not resize itself. The 
> number
> of buckets needs to be specified at creation time so if this is set too low 
> we
>  will start to hit performace problems [1].
> 
> However for most uses of the tables there will likely be no measurable
> difference, this will more likely just be a clean-up exercise rather than a
> performance win.
> 
> As far as I can tell all new code is already encouraged to use the new hash
> (as nir does) and it just that no-one has gotten around to replacing the old
> hash [2].
> 
> [1] http://lists.freedesktop.org/archives/mesa-dev/2012-October/029074.html

Ok that was actualy another hash implementation but you get the point.

> [2] http://lists.freedesktop.org/archives/mesa-dev/2013-December/050524.html
>  
> 
> 
> > 
> >   -ilia
> > 
> > On Sun, Aug 2, 2015 at 8:05 PM, Chris Forbes <chrisf at ijw.co.nz> wrote:
> > > Some perf numbers would be nice. How much is this winning?
> > > 
> > > - Chris
> > > 
> > > On Mon, Aug 3, 2015 at 11:18 AM, Timothy Arceri <t_arceri at yahoo.com.au> 
> > > wrote:
> > > > On Sun, 2015-08-02 at 19:50 +0200, Alejandro Seguí wrote:
> > > > 
> > > > Maybe just for completeness you could add this to the commit message
> > > > 
> > > > The util/hash_table was intended to be a fast hash table
> > > > replacement for the program/hash_table see 35fd61bd99c1 and 
> > > > 72e55bb6888ff.
> > > > 
> > > > > ---
> > > > >  src/glsl/ir_print_visitor.cpp | 19 ++++++++++++-------
> > > > >  1 file changed, 12 insertions(+), 7 deletions(-)
> > > > > 
> > > > > diff --git a/src/glsl/ir_print_visitor.cpp 
> > > > > b/src/glsl/ir_print_visitor.cpp
> > > > > index 78475f3..641a996 100644
> > > > > --- a/src/glsl/ir_print_visitor.cpp
> > > > > +++ b/src/glsl/ir_print_visitor.cpp
> > > > > @@ -25,7 +25,7 @@
> > > > >  #include "glsl_types.h"
> > > > >  #include "glsl_parser_extras.h"
> > > > >  #include "main/macros.h"
> > > > > -#include "program/hash_table.h"
> > > > > +#include "util/hash_table.h"
> > > > > 
> > > > >  static void print_type(FILE *f, const glsl_type *t);
> > > > > 
> > > > > @@ -89,14 +89,14 @@ ir_print_visitor::ir_print_visitor(FILE *f)
> > > > >  {
> > > > >     indentation = 0;
> > > > >     printable_names =
> > > > > -      hash_table_ctor(32, hash_table_pointer_hash,
> > > > > hash_table_pointer_compare);
> > > > > +      _mesa_hash_table_create(NULL, _mesa_hash_pointer,
> > > > > _mesa_key_pointer_equal);
> > > > >     symbols = _mesa_symbol_table_ctor();
> > > > >     mem_ctx = ralloc_context(NULL);
> > > > >  }
> > > > > 
> > > > >  ir_print_visitor::~ir_print_visitor()
> > > > >  {
> > > > > -   hash_table_dtor(printable_names);
> > > > > +   _mesa_hash_table_destroy(printable_names, NULL);
> > > > >     _mesa_symbol_table_dtor(symbols);
> > > > >     ralloc_free(mem_ctx);
> > > > >  }
> > > > > @@ -121,9 +121,14 @@ ir_print_visitor::unique_name(ir_variable *var)
> > > > >     }
> > > > > 
> > > > >     /* Do we already have a name for this variable? */
> > > > > -   const char *name = (const char *) hash_table_find(this
> > > > > ->printable_names,
> > > > > var);
> > > > > -   if (name != NULL)
> > > > > -      return name;
> > > > > +   struct hash_entry * entry =
> > > > > +        _mesa_hash_table_search(this->printable_names, var);
> > > > > +
> > > > > +   if (entry != NULL) {
> > > > > +      return (const char *) entry->data;
> > > > > +   }
> > > > > +
> > > > > +   const char* name = NULL;
> > > > 
> > > > The above looks a bit funny just floating here maybe move it
> > > > 
> > > > > 
> > > > >     /* If there's no conflict, just use the original name */
> > > > Here.
> > > > >     if (_mesa_symbol_table_find_symbol(this->symbols, -1, var->name) 
> > > > > 
> > > > > ==
> > > > > NULL) {
> > > > > @@ -132,7 +137,7 @@ ir_print_visitor::unique_name(ir_variable *var)
> > > > >        static unsigned i = 1;
> > > > >        name = ralloc_asprintf(this->mem_ctx, "%s@%u", var->name, 
> > > > > ++i);
> > > > >     }
> > > > > -   hash_table_insert(this->printable_names, (void *) name, var);
> > > > > +   _mesa_hash_table_insert(this->printable_names, var, (void *) 
> > > > > name);
> > > > >     _mesa_symbol_table_add_symbol(this->symbols, -1, name, var);
> > > > >     return name;
> > > > >  }
> > > > 
> > > > With those couple of small changes you can add to the commit message
> > > > Reviewed-by: Timothy Arceri <t_arceri at yahoo.com.au>
> > > > 
> > > > _______________________________________________
> > > > mesa-dev mailing list
> > > > mesa-dev at lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev