On Mon, 2015-08-03 at 03:54 +0200, Alejandro Seguí Díaz wrote:
> 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.
If you do this all at once you would still want to have a different commit for
each table that is replaced so things can be easily bisected and reverted if
need be.
You would just send it as a patch series.
>
> 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.
Thats fine it can be added before committing I think this patch is fine as is.
>
> 2015-08-03 2:40 GMT+02:00 Chris Forbes <chr...@ijw.co.nz>:
> > Oh, fair enough then.
> > On Aug 3, 2015 12:39 PM, "Ilia Mirkin" <imir...@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 <chr...@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_arc...@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_arc...@yahoo.com.au>
> > > >>
> > > >> _______________________________________________
> > > >> mesa-dev mailing list
> > > >> mesa-dev@lists.freedesktop.org
> > > >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > > _______________________________________________
> > > > mesa-dev mailing list
> > > > mesa-dev@lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > >
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
