Re: [PATCH v1] config: Add hashtable for config parsing & retrival

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 10, 2014, 21:28 UTC
Message-ID: <CAPig+cT0gLzMV7p9-ZavZn2t00Oh_-4oQWZuk8ptYWEtmCuYVA@mail.gmail.com>
In-Reply-To: <5396FB7C.6000709@gmail.com>

On Tue, Jun 10, 2014 at 8:35 AM, Tanay Abhra <tanayabh@gmail.com> wrote:

> Thanks for the review, Eric. I have replied to your comments below,
> I will try to reply early and more promptly now.

Thanks for responding. More below.

Show 30 quoted lines

> On 06/10/2014 04:27 AM, Eric Sunshine wrote:
>>> ---
>>> diff --git a/config.c b/config.c
>>> index a30cb5c..6f6b04e 100644
>>> --- a/config.c
>>> +++ b/config.c
>>> @@ -9,6 +9,8 @@
>>>  #include "exec_cmd.h"
>>>  #include "strbuf.h"
>>>  #include "quote.h"
>>> +#include "hashmap.h"
>>> +#include "string-list.h"
>>>
>>>  struct config_source {
>>>         struct config_source *prev;
>>> @@ -37,6 +39,120 @@ static struct config_source *cf;
>>>
>>>  static int zlib_compression_seen;
>>>
>>> +struct config_cache_entry {
>>> +       struct hashmap_entry ent;
>>> +       char *key;
>>> +       struct string_list *value_list;
>>
>> Same question as in my v1 and v2 reviews [1][2], and reiterated by
>> Matthieu [3]: Why is 'value_list' a pointer to a structure rather than
>> just a structure?
>
> Sorry for the lack of response on this part. I thought choosing a pointer to a
> structure or just the structure itself was a stylistic choice.

Each point or question raised in a review is meaningful to the reviewer and deserves some sort of response or consideration, even if you don't agree with the reviewer's opinion. It's okay to have stylistic or other preferences (and to argue for them), but unless you voice them, the reviewer is likely to conclude that his review effort is wasted.

In this case, it's not a stylistic consideration which prompted the question, but practical concern. (More on that below.)

> Since most of the
> functions just pass the pointer to this struct, I thought it would be more natural to
> use it. But I have changed my mind on this one. In the next iteration I will be using
> a plane old struct.

There are practical reasons for choosing a structure rather than a pointer to a structure. Problems with a pointer include:

- Expense of an extra (unnecessary) heap allocation. You allocate both
config_cache_entry and string_list on the heap for each entry being
inserted into the hash, rather than allocating only a
config_cache_entry.

- Potential additional heap fragmentation.

- Increased possibility of leaking memory. The current implementation
of config_cache_free() drives this point home: although it correctly
clears the string_list, it leaks the heap-allocated string_list
itself.

- Potentially misleading readers of the code into thinking that there
is an important reason for the string_list to be allocated separately
from its containing object (for instance, "is the string list shared?"
or "does the string list need to exist beyond the lifetime of the
config_cache_entry?").

Show 16 quoted lines

>>> +}
>>> +
>>> +static void config_cache_init(struct hashmap *config_cache)
>>> +{
>>> +       hashmap_init(config_cache, (hashmap_cmp_fn) config_cache_entry_cmp_case, 0);
>>
>> In his review [4], Peff suggested giving config_cache_entry_cmp_case()
>> the correct hashmap_cmp_fn signature so that this cast can be dropped.
>> It's not clear whether you disagree with his advice or overlooked or
>> forgot about it. If you disagree with his suggestion, it's okay to say
>> so and explain why you feel the way you do, but without feedback from
>> you, he or another reviewer is going to continue suggesting the same
>> change.
>
> Now on this one, I checked the code thoroughly. Every single hashmap_init()
> incantation in git code has a hashmap_cmp_fn cast.

That's a good argument for explaining to Peff why you did it this way, but unless you voice this, he will likely feel that you simply ignored his review comment.

> I don't think that it is necessary to do so. Is it?

There are good reasons for avoiding the cast. Jonathan gives one such reason here [1]. In that case, he let it slide for the reason you mention: existing hashmap clients use the cast. (However, the implication is that it would be nice for someone -- not necessarily you -- to submit a patch series eliminating the casts.)

Whether Peff will be swayed by the same argument, only he can answer. (If he doesn't answer, taking [1] into consideration, you'd probably be on safe ground to continue using the cast.)

[1]: http://thread.gmane.org/gmane.comp.version-control.git/243818/focus=243917 (see his comments about hashmap_init()).

Show 14 quoted lines

>>> +       if (ret)
>>> +               return NULL;
>>> +
>>> +       hashmap_entry_init(&k, strhash(normalized_key));
>>> +       k.key = (char*) key;
>>
>> This looks fishy. You're hashing based upon 'normalized_key' but then
>> comparing against the unnormalized 'key'. Peff's suggestion [4] was to
>> store the normalized key in the hash, which means that you should use
>> 'normalized_key' for both hashing and comparing. (See additional
>> commentary about this issue below in config_cache_set_value().)
>
> Ouch, this I had corrected in a future commit. But forgot to include in
> this patch.

[Leaving unsnipped since it has relevance below.]

>> Style: (char *)key
>
> Noted. In function arguments the code uses (char*) key. I copied it from there.
> :)

Indeed, there's an unfortunate lack of consistency: in fact, even config.c already has a mix of "(x*)y", "(x *)y", and "(x *) y", but I think "(x *)y" is the one preferred and seems to be the most common.

Show 31 quoted lines

>>> +static int config_cache_set_value(const char *key, const char *value)
>>> +{
>>> +       struct hashmap *config_cache;
>>> +       struct config_cache_entry *e;
>>> +
>>> +       config_cache = get_config_cache();
>>> +       e = config_cache_find_entry(key);
>>> +       if (!e) {
>>> +               e = xmalloc(sizeof(*e));
>>> +               hashmap_entry_init(e, strhash(key));
>>
>> The hash computed to store the key should be the same as the hash to
>> look it up. In config_cache_find_entry(), you're correctly computing
>> the hash based upon the normalized key, but here you're doing so from
>> the unnormalized key.
>>
>>> +               e->key = xstrdup(key);
>>
>> Likewise. Normalized keys should be stored in the hash, not unnormalized.
>>
>> You should be invoking git_config_parse_key() to normalize the key
>> both for hashing and storing.
>>
>> Note, also, that call git_config_parse_key() allocates the normalize
>> key on your behalf, so you do *not* want to xstrdup() it here.
>
> config_cache_set_value() gets its values from git_config() as it the callback.
> git_config() feeds the callback only normalized values by using functions like
> get_extended_base_var(), get_base_var() etc. Thus, I didn't use
> git_config_parse_key(). Please correct me if I am wrong, but I tested this case
> thoroughly.

You're right. Documentation/api-config.txt even states that 'key' is already normalized here. Thanks for pointing it out. So, the only thing in need of fixing is the fishy bit in config_cache_find_entry() where it is incorrectly searching for the unnormalized key.

Show 16 quoted lines

>>> +               e->value_list = xcalloc(sizeof(struct string_list), 1);
>>> +               e->value_list->strdup_strings = 1;
>>
>> This code is perhaps too intimate with the implementation of
>> string_list. It may work today (because it is safe to initialize all
>> string_list fields to 0 via xcalloc()), but a future change to the
>> string_list implementation may invalidate that assumption. The
>> initialization macros in string-list.h exist to preserve the
>> abstraction, so you don't have to know the details of initialization.
>> The macros are not suitable for your use-case, but an initialization
>> function, such as string_list_init(), would be suitable, and it would
>> be appropriate to add such a function in a preparatory patch (as
>> already suggested by [1]).
>
> Noted. As I am going to use a simple struct for the list, this won't be
> a problem.

Even after 'value_list' becomes a simple struct rather than a pointer to a struct, the string_list will still require initialization, and the STRING_LIST_INIT_* macros still don't help your case, so it remains a problem. The suggested preparatory patch introducing a new string_list_init() function would allow you to initialize your string_list while preserving the abstraction.