Re: What's cooking in git.git (Dec 2013, #02; Fri, 6)
- From
Karsten Blees <karsten.blees@gmail.com>
- Date
- Dec 18, 2013, 13:10 UTC
- Message-ID
- <52B19EBC.50609@gmail.com>
- In-Reply-To
- <20131209234500.GY29959@google.com>
Am 10.12.2013 00:45, schrieb Jonathan Nieder:
Show 40 quoted lines
> Karsten Blees wrote:
>
>> Googling some more, I believe the most protable way to achieve this
>> via 'compiler settings' is
>>
>> #pragma pack(push)
>> #pragma pack(4)
>> struct hashmap_entry {
>> struct hashmap_entry *next;
>> unsigned int hash;
>> };
>> #pragma pack(pop)
>>
>> This is supported by at least GCC, MSVC and HP (see your link). The
>> downside is that we cannot use macros (in git-compat-util.h) to emit
>> #pragmas. But we wouldn't have to, as compilers aren't supposed to
>> barf on unknown #pragmas.
>
> Technically this can be done using macros:
>
> #if (gcc)
> # define BEGIN_PACKED _Pragma("pack(push,4)")
> # define END_PACKED _Pragma("pack(pop)")
> #elif (msvc)
> # define BEGIN_PACKED __pragma(pack(push,4))
> # define END_PACKED __pragma(pack(pop))
> #else
> /* Just tolerate a little padding. */
> # define BEGIN_PACKED
> # define END_PACKED
> #end
>
> Then you can do:
>
> BEGIN_PACKED
> struct hashmap_entry {
> ...
> };
> END_PACKED
> Sorry for the delay...
My intention with #pragma pack was to support as many compilers / platforms as possible (even though I can't test them all). From what I could find on the 'net, support for the _Pragma operator is slim. And as the MSVC build is 32-bit only (AFAIK), this is pretty much a GCC-only solution (and thus equivalent, but much more verbose than the initial __attribute__((__packed__)) variant).
> Whether that's nicer or uglier than the alternatives I leave to you. > ;-)
If it was really up to me ( :-) ), I'd like to do this:
------ 8< ------- Subject: [PATCH] hashmap.h: make sure map entries are tightly packed
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 struct hashmap_entry needs to be packed for this to work. E.g.
struct name_entry {
struct hashmap_entry ent; /* consists of pointer + int */
int namelen;
char *name;
};should be 24 instead of 32 bytes.
Packing structures is compiler-specific, most compilers support [1]:
#pragma pack(n) - to set structure packing #pragma pack() - to reset structure packing to default
Compilers are supposed to ignore unknown #pragmas, so using this without further #if <compiler> guards should be safe.
Wasting a few bytes for padding is acceptable, however, compiling the rest of git with packed structures (and thus mis-aligned arrays of structs) is not. Add a test to ensure that '#pragma pack()' really resets the packing.
[1] Compiler docs regarding #pragma pack: http://gcc.gnu.org/onlinedocs/gcc-4.6.2/gcc/Structure_002dPacking-Pragmas.html#Structure_002dPacking-Pragmas http://msdn.microsoft.com/en-us/library/2e70t5y1%28v=vs.80%29.aspx http://h21007.www2.hp.com/portal/download/files/unprot/aCxx/Online_Help/pragmas.htm#pragma-PACK http://clang.llvm.org/docs/UsersManual.html#microsoft-extensions http://uw714doc.sco.com/en/SDK_cprog/_Preprocessing_Directives.html http://osr507doc.sco.com/en/tools/ANSI_F.3.13_Preprocessing.html http://docs.oracle.com/cd/E19422-01/819-3690/Pragmas_App.html#73499 http://publib.boulder.ibm.com/infocenter/comphelp/v7v91/index.jsp?topic=%2Fcom.ibm.vacpp7a.doc%2Fcompiler%2Fref%2Frnpgpack.htm
Supposedly wants #pragma pack(0) to reset: http://techpubs.sgi.com/library/tpl/cgi-bin/getdoc.cgi/0650/bks/SGI_Developer/books/Pragmas/sgi_html/ch04.html
Not supported: http://software.intel.com/sites/products/documentation/doclib/iss/2013/compiler/cpp-lin/GUID-DD32852C-A0F9-4AC6-BF67-D10D064CC87A.htm
Signed-off-by: Karsten Blees <blees@dcon.de> --- hashmap.h | 7 +++++++ t/t0011-hashmap.sh | 6 ++++++ test-hashmap.c | 17 +++++++++++++++++ 3 files changed, 30 insertions(+)
diff --git a/hashmap.h b/hashmap.h index a816ad4..93b330b 100644 --- a/hashmap.h +++ b/hashmap.h @@ -15,10 +15,17 @@ extern unsigned int memihash(const void *buf, size_t len); /* data structures */ +/* + * Set struct packing to 4 so that we don't waste memory on 64-bit systems. + * Struct hashmap_entry is used as prefix to other (un-packed) structures, + * so this won't cause alignment issues e.g. for odd elements in an array. + */ +#pragma pack(4) struct hashmap_entry { struct hashmap_entry *next; unsigned int hash; }; +#pragma pack() typedef int (*hashmap_cmp_fn)(const void *entry, const void *entry_or_key, const void *keydata); diff --git a/t/t0011-hashmap.sh b/t/t0011-hashmap.sh index 391e2b6..3f3c90b 100755 --- a/t/t0011-hashmap.sh +++ b/t/t0011-hashmap.sh @@ -237,4 +237,10 @@ test_expect_success 'grow / shrink' ' ' +test_expect_success '"#pragma pack()" resets packing to default' ' + + test_hashmap "pragma-pack" "ok" + +' + test_done diff --git a/test-hashmap.c b/test-hashmap.c index f5183fb..2d5a63a 100644 --- a/test-hashmap.c +++ b/test-hashmap.c @@ -36,6 +36,12 @@ static struct test_entry *alloc_test_entry(int hash, char *key, int klen, return entry; } +struct pointer_int +{ + void *ptr; + int i; +}; + #define HASH_METHOD_FNV 0 #define HASH_METHOD_I 1 #define HASH_METHOD_IDIV10 2 @@ -136,6 +142,7 @@ static void perf_hashmap(unsigned int method, unsigned int rounds) * remove key -> NULL / old value * iterate -> key1 value1\nkey2 value2\n... * size -> tablesize numentries + * pragma-pack -> ok / failure * * perfhashmap method rounds -> test hashmap.[ch] performance */ @@ -239,6 +246,16 @@ int main(int argc, char *argv[]) /* print table sizes */ printf("%u %u\n", map.tablesize, map.size); + } else if (!strcmp("pragma-pack", cmd)) { + + if ((sizeof(struct pointer_int) % sizeof(void *)) == 0) + printf("ok\n"); + else + printf("sizeof(pointer+int) (%u) is not a " + "multiple of sizeof(pointer) (%u)!\n", + sizeof(struct pointer_int), + sizeof(void *)); + } else if (!strcmp("perfhashmap", cmd) && l1 && l2) { perf_hashmap(atoi(p1), atoi(p2));
-- 1.8.5.1.276.g562b27a