Mladen Turk wrote:
[...]
>I've treated the apr_table as database table, having columns and rows.
>When you think of apr_table that way, you can use the same technique
>that the databases use to quickly access particular row, using some sort
>of indexing.
>
I like the concept of adding a fast index as an internal optimization
to apr_table_t, but I have some concerns about the implementation:
From apr_table_t.h:
>/** the hash table bucket abstract data type */
>+typedef struct apr_htable_bucket_t apr_htable_bucket_t;
>+
>+struct apr_htable_bucket_t {
>+ apr_htable_bucket_t *next;
>+ int hash;
>+ int elt;
>+};
>
I propose moving the definition of this struct outside of the
public .h file, so nobody can write code that violates the
abstraction.
Also, why add another hash table implementation? Apr_hash_t won't
quite work here, because it doesn't have efficient support for
case-insensitive string keys, but I'd rather add that support
into apr_hash_t than add another version of hash tables to APR.
With this design, where correctness depends upon the array and
the hash table within the apr_table_t staying synchronized, I'd
feel much more comfortable if the table_elts() macro were modified
to return a const array. I think all the uses of this macro within
Apache treat the output as const anyway.
Based on my really quick reading of your new version of apr_tables.c,
the code looks reasonable. My main worry is that the overhead of
the hash table maintenance, including the function call overhead and
the need to reindex every time the table is expanded, may offset the
performance benefits of hashing. Do you have any benchmark data
to compare the performance of this implementation with the current
apr_table_t?
--Brian
