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

Re: [PATCH] pack-objects.h: remove outdated pahole results

From
brian m. carlson <sandals@crustytoothpaste.net>
Date
Jun 28, 2022, 21:04 UTC
Message-ID
<Yrts22oGOS/igqH/@tapette.crustytoothpaste.net>
In-Reply-To
<1379af2e9d271b501ef3942398e7f159a9c77973.1656440978.git.me@ttaylorr.com>
On 2022-06-28 at 18:30:20, Taylor Blau wrote:
Show 38 quoted lines
> The size and padding of `struct object_entry` is an important factor in
> determining the memory usage of `pack-objects`. For this reason,
> 3b13a5f263 (pack-objects: reorder members to shrink struct object_entry,
> 2018-04-14) added a comment containing some information from pahole
> indicating the size and padding of that struct.
> 
> Unfortunately, this comment hasn't been updated since 9ac3f0e5b3
> (pack-objects: fix performance issues on packing large deltas,
> 2018-07-22), despite the size of this struct changing many times since
> that commit.
> 
> To see just how often the size of object_entry changes, I skimmed the
> first-parent history with this script:
> 
>     for sha in $(git rev-list --first-parent --reverse 9ac3f0e..)
>     do
>       echo -n "$sha "
>       git checkout -q $sha
>       make -s pack-objects.o 2>/dev/null
>       pahole -C object_entry pack-objects.o | sed -n \
>         -e 's/\/\* size: \([0-9]*\).*/size \1/p' \
>         -e 's/\/\*.*padding: \([0-9]*\).*/padding \1/p' | xargs
>     done | uniq -f1
> 
> In between each merge, the size of object_entry changes too often to
> record every instance here. But the important merges (along with their
> corresponding sizes and bit paddings) in chronological order are:
> 
>     ad635e82d6 (Merge branch 'nd/pack-objects-pack-struct', 2018-05-23) size 80 padding 4
>     29d9e3e2c4 (Merge branch 'nd/pack-deltify-regression-fix', 2018-08-22) size 80 padding 9
>     3ebdef2e1b (Merge branch 'jk/pack-delta-reuse-with-bitmap', 2018-09-17) size 80 padding 8
>     33e4ae9c50 (Merge branch 'bc/sha-256', 2019-01-29) size 96 padding 8
> 
> (indicating that the current size of the struct is 96 bytes, with 8
> padding bits).
> 
> Even though this comment was written in a good spirit, it is updated
> infrequently enough that is serves to confuse rather than to encourage
I think you wanted to say, "that it serves:.
Show 7 quoted lines
> contributors to update the appropriate values when the modify the
> definition of object_entry.
> 
> For that reason, eliminate the confusion by removing the comment
> altogether.
> 
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
I agree with your rationale and that we should remove this.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
Previous: Derrick StoleeNext: Taylor Blau
Message 3 of 7 in “pack-objects.h: remove outdated pahole results”
  1. pack-objects.h: remove outdated pahole resultsTaylor Blau, Jun 28, 2022
  2. Derrick StoleeJun 28, 2022
  3. brian m. carlsonJun 28, 2022
  4. Taylor BlauJun 28, 2022
  5. Jeff KingJul 1, 2022
  6. Taylor BlauJul 1, 2022
  7. Jeff KingJul 1, 2022

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.