On Mon, Nov 02, 2020 at 06:55:12PM +0000, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@xxxxxxxxx>
>
> By default, we do not use a mempool and strdup_strings is true; in this
> case, we can avoid both an extra allocation and an extra free by just
> over-allocating for the strmap_entry leaving enough space at the end to
> copy the key. FLEXPTR_ALLOC_STR exists for exactly this purpose, so
> make use of it.
>
> Also, adjust the case when we are using a memory pool and strdup_strings
> is true to just do one allocation from the memory pool instead of two so
> that the strmap_clear() and strmap_remove() code can just avoid freeing
> the key in all cases.
This turned out to be much less painful than I feared, and I think is
worth doing. Thanks for digging on it.
> + if (map->strdup_strings) {
> + if (!map->pool) {
> + FLEXPTR_ALLOC_STR(entry, key, str);
> + } else {
> + /* Remember +1 for nul byte twice below */
> + size_t len = strlen(str);
> + entry = mem_pool_alloc(map->pool,
> + st_add3(sizeof(*entry), len, 1));
> + memcpy(entry->keydata, str, len+1);
> + }
Perhaps:
size_t len = st_add(strlen(str), 1); /* include NUL */
entry = mem_pool_alloc(map->pool, st_add(sizeof(*entry), len));
memcpy(entry->keydata, str, len);
would be more obvious than the "remember to do it twice" comment?
With a FLEXPTR, I don't think you need keydata at all (since we would
never use that name; note that we don't even pass it in at all to
FLEXPTR_ALLOC_STR). Without that, I think your memcpy becomes:
memcpy(entry + 1, str, len);
Remember that "entry" is a typed pointer, so "1" is really moving
sizeof(*entry) bytes.
> + } else if (!map->pool) {
> + entry = xmalloc(sizeof(*entry));
> + } else {
> + entry = mem_pool_alloc(map->pool, sizeof(*entry));
> + }
OK, so if we're not strdup-ing then we either get a mempool or a fresh
entry. Makes sense.
> hashmap_entry_init(&entry->ent, strhash(str));
> -
> - if (map->strdup_strings)
> - key = map->pool ? mem_pool_strdup(map->pool, str)
> - : xstrdup(str);
> - entry->key = key;
> + entry->key = map->strdup_strings ? entry->keydata : str;
I think this is subtly wrong in the FLEXPTR case. The data isn't in
keydata; it's directly after the struct. That's _usually_ the same
thing, but:
- the compiler can put struct padding at the end if it wants
- FLEX_ARRAY is usually zero, but for compatibility on some platforms
it must be 1
The call to FLEXPTR_ALLOC_STR() will have already set it up properly
(and this is at best writing the same value, and at worst messing it
up).
I think you probably want to leave the FLEXPTR_ALLOC_STR() part alone,
put a:
entry->key = (void *)(entry + 1);
line in the mem_pool code path, and then here do:
if (!strdup_strings)
entry->key = str;
-Peff
