Re: What's cooking in git.git (Dec 2013, #02; Fri, 6)

[Karsten Blees <karsten.blees@gmail.com>]

Am 08.12.2013 11:20, schrieb Thomas Rast:
> Karsten Blees <karsten.blees@gmail.com> writes:
> 
>> Am 07.12.2013 23:23, schrieb Thomas Rast:
>>> Karsten Blees <karsten.blees@gmail.com> writes:
>>>
>>>> Extending 'struct hashmap_entry' with an int-sized member shouldn't waste
>>>> memory on 64-bit systems. This is already documented in api-hashmap.txt,
>>>> but needs '__attribute__((__packed__))' to work. Reduces e.g.
>>>
>>> You'd have to guard __attribute__((__packed__)) with some compiler
>>> detection in git-compat-util.h though.
>>>
>>
>> Isn't that already handled? __attribute__ is already widely used
>> (e.g. for printf formats), and platforms that don't support it define
>> it as empty (e.g. MSVC). Or do you mean I should account for
>> compiler-specific variants (#pragma pack...)?
> 
> True, __attribute__ expands to nothing on unknown compilers, but what
> does the compiler do when it sees an unknown attribute?  If some of them
> choke, you need a separate macro.
> 
> I'm a bit confused myself though, many attributes have special macros in
> git-compat-util.h but others we just use in the code.
> 

So what do you propose? I basically see three options:

1.) Trial and error

GCC supports __packed__ as of 2.3 (1992), so any other compilers that copied the __attribute__ feature probably won't complain.

2.) Accept the wasted memory

Moving the hash code from the hash table (as in hash.[ch]) to the entry is already a big improvement, as it no longer multiplies hash code + wasted bytes with the load factor. 64-bit software uses more memory in general, so we could just live with it (and only fix the documentation in api-hashmap.txt).

3.) Inject individual fields via macro

Instead of injecting a struct hashmap_entry, which implies alignment to sizeof(struct hashmap_entry), we could inject the individual fields, e.g.

 #define HASHMAP_ENTRY_HEADER struct hashmap_entry __next; unsinged int __hash;

 struct name_entry {
   HASHMAP_ENTRY_HEADER
   int namelen;
   char *name;
 };


What do you think?

Karsten