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

[Alejandro Seguí Díaz]

I'm starting to contribute and Timothy Arceri told me about this
task. The change was made at that sole file because we talked
about no need to migrate all tables at once, but we discuss this
and I take note to migrate all hash tables at once if everyone
agrees.

PS.: I forgot to add Reviewed-by Timothy Arceri at the commit
message. If we are ok with migrating in steps, I'll edit it and send
the patch again.

2015-08-03 2:40 GMT+02:00 Chris Forbes <chrisf at ijw.co.nz>:

> Oh, fair enough then.
> On Aug 3, 2015 12:39 PM, "Ilia Mirkin" <imirkin at alum.mit.edu> 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...
>>
>>   -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
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150803/73c814f2/attachment.html>