git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: struct hashmap_entry packing

From
Karsten Blees <karsten.blees@gmail.com>
Date
Jul 29, 2014, 20:40 UTC
Message-ID
<53D806AC.3070806@gmail.com>
In-Reply-To
<20140728171743.GA1927@peff.net>
Am 28.07.2014 19:17, schrieb Jeff King:
Show 30 quoted lines
> Hi Karsten,
> 
> The hashmap_entry documentation claims:
> 
>   `struct hashmap_entry`::
> 
> 	An opaque structure representing an entry in the hash table,
> 	which must be used as first member of user data structures.
> 	Ideally it should be followed by an int-sized member to prevent
> 	unused memory on 64-bit systems due to alignment.
> 
> I'm not sure if the statement about alignment is true. If I have a
> struct like:
> 
>     struct magic {
> 	    struct hashmap_entry map;
> 	    int x;
>     };
> 
> the statement above implies that I should be able to fit this into only
> 16 bytes on an LP64 system. But I can't convince gcc to do it. And I
> think that makes sense, if you consider code like:
> 
>    memset(&magic.map, 0, sizeof(struct hashmap_entry));
> 
> The sizeof() has to be the same regardless of whether the hashmap_entry
> is standalone or in another struct, and therefore must be padded up to
> 16 bytes. If we stored "x" in that padding in the combined struct, it
> would be overwritten by our memset.
> 

The struct-packing patch was ultimately dropped because there was no way to reliably make it work on all platforms. See [1] for discussion, [2] for the final, 'most compatible' version.

> Am I missing anything? If this is the case, we should probably drop that
> bit from the documentation.

Hmmm. Now that we have "__attribute__((packed))" in pack-bitmap.h, perhaps we should do the same for stuct hashmap_entry? (Which was the original proposal anyway...). Only works for GCC, but that should cover most builds / platforms.

Btw.: Using struct-packing on 'struct bitmap_disk_entry' means that the binary format of .bitmap files is incompatible between GCC and other builds, correct?

> It's possible that we could get around it by
> embedding the hashmap_entry elements directly into the parent struct,
Already tried that, see [3].

[1] http://article.gmane.org/gmane.comp.version-control.git/239069 [2] http://article.gmane.org/gmane.comp.version-control.git/241865 [3] http://article.gmane.org/gmane.comp.version-control.git/239435

Previous: Jeff KingNext: Jeff King
Message 2 of 12 in “struct hashmap_entry packing”
  1. Jeff KingJul 28, 2014
  2. Karsten BleesJul 29, 2014
  3. Jeff KingAug 1, 2014
  4. pack-bitmap: do not use gcc packed attributeJeff King, Aug 1, 2014
  5. Jeff KingAug 1, 2014
  6. Karsten BleesAug 4, 2014
  7. Vicent MartíAug 5, 2014
  8. Jeff KingAug 5, 2014
  9. Karsten BleesAug 6, 2014
  10. Junio C HamanoAug 6, 2014
  11. Karsten BleesAug 4, 2014
  12. Jeff KingAug 5, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.