{"thread":{"id":"58078","subject":"[PATCH] pack-objects.h: remove outdated pahole results","startedAt":"2022-06-28T18:30:26Z","lastAt":"2022-07-01T21:05:23Z","messageCount":7,"participants":["Taylor Blau","Derrick Stolee","brian m. carlson","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"458060","messageId":"1379af2e9d271b501ef3942398e7f159a9c77973.1656440978.git.me@ttaylorr.com","threadId":"58078","inReplyTo":null,"subject":"[PATCH] pack-objects.h: remove outdated pahole results","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-28T18:30:20Z","receivedAt":"2022-06-28T18:30:26Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The size and padding of `struct object_entry` is an important factor in\ndetermining the memory usage of `pack-objects`. For this reason,\n3b13a5f263 (pack-objects: reorder members to shrink struct object_entry,\n2018-04-14) added a comment containing some information from pahole\nindicating the size and padding of that struct.\n\nUnfortunately, this comment hasn't been updated since 9ac3f0e5b3\n(pack-objects: fix performance issues on packing large deltas,\n2018-07-22), despite the size of this struct changing many times since\nthat commit.\n\nTo see just how often the size of object_entry changes, I skimmed the\nfirst-parent history with this script:\n\n    for sha in $(git rev-list --first-parent --reverse 9ac3f0e..)\n    do\n      echo -n \"$sha \"\n      git checkout -q $sha\n      make -s pack-objects.o 2>/dev/null\n      pahole -C object_entry pack-objects.o | sed -n \\\n        -e 's/\\/\\* size: \\([0-9]*\\).*/size \\1/p' \\\n        -e 's/\\/\\*.*padding: \\([0-9]*\\).*/padding \\1/p' | xargs\n    done | uniq -f1\n\nIn between each merge, the size of object_entry changes too often to\nrecord every instance here. But the important merges (along with their\ncorresponding sizes and bit paddings) in chronological order are:\n\n    ad635e82d6 (Merge branch 'nd/pack-objects-pack-struct', 2018-05-23) size 80 padding 4\n    29d9e3e2c4 (Merge branch 'nd/pack-deltify-regression-fix', 2018-08-22) size 80 padding 9\n    3ebdef2e1b (Merge branch 'jk/pack-delta-reuse-with-bitmap', 2018-09-17) size 80 padding 8\n    33e4ae9c50 (Merge branch 'bc/sha-256', 2019-01-29) size 96 padding 8\n\n(indicating that the current size of the struct is 96 bytes, with 8\npadding bits).\n\nEven though this comment was written in a good spirit, it is updated\ninfrequently enough that is serves to confuse rather than to encourage\ncontributors to update the appropriate values when the modify the\ndefinition of object_entry.\n\nFor that reason, eliminate the confusion by removing the comment\naltogether.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\nNoticed this while reverting an old topic out of GitHub's fork, and\nrealized that this comment was severely out-of-date.\n\n pack-objects.h | 10 ----------\n 1 file changed, 10 deletions(-)\n\ndiff --git a/pack-objects.h b/pack-objects.h\nindex 393b9db546..579476687c 100644\n--- a/pack-objects.h\n+++ b/pack-objects.h\n@@ -116,16 +116,6 @@ struct object_entry {\n \tunsigned dfs_state:OE_DFS_STATE_BITS;\n \tunsigned depth:OE_DEPTH_BITS;\n \tunsigned ext_base:1; /* delta_idx points outside packlist */\n-\n-\t/*\n-\t * pahole results on 64-bit linux (gcc and clang)\n-\t *\n-\t *   size: 80, bit_padding: 9 bits\n-\t *\n-\t * and on 32-bit (gcc)\n-\t *\n-\t *   size: 76, bit_padding: 9 bits\n-\t */\n };\n\n struct packing_data {\n--\n2.37.0.1.g1379af2e9d\n"},{"id":"458065","messageId":"69ee674a-6564-b0bc-fa53-2ec7dc02f80e@github.com","threadId":"58078","inReplyTo":"1379af2e9d271b501ef3942398e7f159a9c77973.1656440978.git.me@ttaylorr.com","subject":"Re: [PATCH] pack-objects.h: remove outdated pahole results","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-06-28T20:03:17Z","receivedAt":"2022-06-28T20:13:08Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/28/2022 2:30 PM, Taylor Blau wrote:\n> The size and padding of `struct object_entry` is an important factor in\n> determining the memory usage of `pack-objects`. For this reason,\n> 3b13a5f263 (pack-objects: reorder members to shrink struct object_entry,\n> 2018-04-14) added a comment containing some information from pahole\n> indicating the size and padding of that struct.\n> \n> Unfortunately, this comment hasn't been updated since 9ac3f0e5b3\n> (pack-objects: fix performance issues on packing large deltas,\n> 2018-07-22), despite the size of this struct changing many times since\n> that commit.\n...\n> For that reason, eliminate the confusion by removing the comment\n> altogether.\n\n> -\n> -\t/*\n> -\t * pahole results on 64-bit linux (gcc and clang)\n> -\t *\n> -\t *   size: 80, bit_padding: 9 bits\n> -\t *\n> -\t * and on 32-bit (gcc)\n> -\t *\n> -\t *   size: 76, bit_padding: 9 bits\n> -\t */\n>  };\n\nI will shed no tears over this being gone. Thanks!\n\n-Stolee\n"},{"id":"458071","messageId":"Yrts22oGOS/igqH/@tapette.crustytoothpaste.net","threadId":"58078","inReplyTo":"1379af2e9d271b501ef3942398e7f159a9c77973.1656440978.git.me@ttaylorr.com","subject":"Re: [PATCH] pack-objects.h: remove outdated pahole results","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2022-06-28T21:04:27Z","receivedAt":"2022-06-28T21:04:33Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2022-06-28 at 18:30:20, Taylor Blau wrote:\n> The size and padding of `struct object_entry` is an important factor in\n> determining the memory usage of `pack-objects`. For this reason,\n> 3b13a5f263 (pack-objects: reorder members to shrink struct object_entry,\n> 2018-04-14) added a comment containing some information from pahole\n> indicating the size and padding of that struct.\n> \n> Unfortunately, this comment hasn't been updated since 9ac3f0e5b3\n> (pack-objects: fix performance issues on packing large deltas,\n> 2018-07-22), despite the size of this struct changing many times since\n> that commit.\n> \n> To see just how often the size of object_entry changes, I skimmed the\n> first-parent history with this script:\n> \n>     for sha in $(git rev-list --first-parent --reverse 9ac3f0e..)\n>     do\n>       echo -n \"$sha \"\n>       git checkout -q $sha\n>       make -s pack-objects.o 2>/dev/null\n>       pahole -C object_entry pack-objects.o | sed -n \\\n>         -e 's/\\/\\* size: \\([0-9]*\\).*/size \\1/p' \\\n>         -e 's/\\/\\*.*padding: \\([0-9]*\\).*/padding \\1/p' | xargs\n>     done | uniq -f1\n> \n> In between each merge, the size of object_entry changes too often to\n> record every instance here. But the important merges (along with their\n> corresponding sizes and bit paddings) in chronological order are:\n> \n>     ad635e82d6 (Merge branch 'nd/pack-objects-pack-struct', 2018-05-23) size 80 padding 4\n>     29d9e3e2c4 (Merge branch 'nd/pack-deltify-regression-fix', 2018-08-22) size 80 padding 9\n>     3ebdef2e1b (Merge branch 'jk/pack-delta-reuse-with-bitmap', 2018-09-17) size 80 padding 8\n>     33e4ae9c50 (Merge branch 'bc/sha-256', 2019-01-29) size 96 padding 8\n> \n> (indicating that the current size of the struct is 96 bytes, with 8\n> padding bits).\n> \n> Even though this comment was written in a good spirit, it is updated\n> infrequently enough that is serves to confuse rather than to encourage\n\nI think you wanted to say, \"that it serves:.\n\n> contributors to update the appropriate values when the modify the\n> definition of object_entry.\n> \n> For that reason, eliminate the confusion by removing the comment\n> altogether.\n> \n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n\nI agree with your rationale and that we should remove this.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"458075","messageId":"YrtyEjGvE7MyLBPl@nand.local","threadId":"58078","inReplyTo":"Yrts22oGOS/igqH/@tapette.crustytoothpaste.net","subject":"Re: [PATCH] pack-objects.h: remove outdated pahole results","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-28T21:26:42Z","receivedAt":"2022-06-28T21:26:51Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jun 28, 2022 at 09:04:27PM +0000, brian m. carlson wrote:\n> > Even though this comment was written in a good spirit, it is updated\n> > infrequently enough that is serves to confuse rather than to encourage\n>\n> I think you wanted to say, \"that it serves:.\n\nOops. Thanks for pointing it out.\n\n> > contributors to update the appropriate values when the modify the\n> > definition of object_entry.\n> >\n> > For that reason, eliminate the confusion by removing the comment\n> > altogether.\n> >\n> > Signed-off-by: Taylor Blau <me@ttaylorr.com>\n>\n> I agree with your rationale and that we should remove this.\n\nI'm glad that you agree.\n\nThanks,\nTaylor\n"},{"id":"458401","messageId":"Yr85+tcexG2geaPP@coredump.intra.peff.net","threadId":"58078","inReplyTo":"1379af2e9d271b501ef3942398e7f159a9c77973.1656440978.git.me@ttaylorr.com","subject":"Re: [PATCH] pack-objects.h: remove outdated pahole results","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-01T18:16:26Z","receivedAt":"2022-07-01T18:16:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 28, 2022 at 02:30:20PM -0400, Taylor Blau wrote:\n\n> Even though this comment was written in a good spirit, it is updated\n> infrequently enough that is serves to confuse rather than to encourage\n> contributors to update the appropriate values when the modify the\n> definition of object_entry.\n> \n> For that reason, eliminate the confusion by removing the comment\n> altogether.\n\nI agree the actual numbers aren't helping anybody. We _could_ leave a\ncomment that says \"we store a lot of these in memory; be careful of\nwhere and how you add new fields to avoid increasing the struct size\".\nAnd then people can run \"pahole\" before and after their changes.\n\nBut then that is also true of other structs (like \"struct object\"), and\nwe do not bother there. So it probably is fine not to annotate this\nspecifically.\n\nSpeaking of which, I suspect quite a lot of memory could be saved if\n\"pack-objects --revs\" freed the object structs it allocates during its\ntraversal. Unless we're generating bitmaps, I don't think they get used\nagain after the initial packing list is generated. At peak you'd\nstill be storing all of the object_entry structs alongside the objects\nas you finish the traversal, but it wouldn't overlap with any memory\nused for the delta search, and of course we'd be at that peak for a much\nsmaller time.\n\nNot a blocker for your patch obviously, but maybe a fun experiment in an\nadjacent area. Possibly even an ambitious #leftoverbits opportunity. :)\n\n-Peff\n"},{"id":"458412","messageId":"Yr9Pg9mRQWe1tgE+@nand.local","threadId":"58078","inReplyTo":"Yr85+tcexG2geaPP@coredump.intra.peff.net","subject":"Re: [PATCH] pack-objects.h: remove outdated pahole results","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-07-01T19:48:19Z","receivedAt":"2022-07-01T19:48:26Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Jul 01, 2022 at 02:16:26PM -0400, Jeff King wrote:\n> On Tue, Jun 28, 2022 at 02:30:20PM -0400, Taylor Blau wrote:\n>\n> > Even though this comment was written in a good spirit, it is updated\n> > infrequently enough that is serves to confuse rather than to encourage\n> > contributors to update the appropriate values when the modify the\n> > definition of object_entry.\n> >\n> > For that reason, eliminate the confusion by removing the comment\n> > altogether.\n>\n> I agree the actual numbers aren't helping anybody. We _could_ leave a\n> comment that says \"we store a lot of these in memory; be careful of\n> where and how you add new fields to avoid increasing the struct size\".\n> And then people can run \"pahole\" before and after their changes.\n>\n> But then that is also true of other structs (like \"struct object\"), and\n> we do not bother there. So it probably is fine not to annotate this\n> specifically.\n\nWe have such a comment at the very type of the block comment above\n`struct object_entry`'s definition:\n\n    \"The size of struct nearly determines pack-object's memory\n    consumption. This struct is packed tight for that reason. When you\n    add or reorder something in this struct, think a bit about this\".\n\nthanks to Duy back in 3b13a5f263 (pack-objects: reorder members to\nshrink struct object_entry, 2018-04-14).\n\n> Speaking of which, I suspect quite a lot of memory could be saved if\n> \"pack-objects --revs\" freed the object structs it allocates during its\n> traversal. Unless we're generating bitmaps, I don't think they get used\n> again after the initial packing list is generated. At peak you'd\n> still be storing all of the object_entry structs alongside the objects\n> as you finish the traversal, but it wouldn't overlap with any memory\n> used for the delta search, and of course we'd be at that peak for a much\n> smaller time.\n>\n> Not a blocker for your patch obviously, but maybe a fun experiment in an\n> adjacent area. Possibly even an ambitious #leftoverbits opportunity. :)\n\nChallenge accepted! ;-)\n\nThanks,\nTaylor\n"},{"id":"458418","messageId":"Yr9hj19gmoiArVat@coredump.intra.peff.net","threadId":"58078","inReplyTo":"Yr9Pg9mRQWe1tgE+@nand.local","subject":"Re: [PATCH] pack-objects.h: remove outdated pahole results","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-01T21:05:19Z","receivedAt":"2022-07-01T21:05:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 01, 2022 at 03:48:19PM -0400, Taylor Blau wrote:\n\n> > I agree the actual numbers aren't helping anybody. We _could_ leave a\n> > comment that says \"we store a lot of these in memory; be careful of\n> > where and how you add new fields to avoid increasing the struct size\".\n> > And then people can run \"pahole\" before and after their changes.\n> >\n> > But then that is also true of other structs (like \"struct object\"), and\n> > we do not bother there. So it probably is fine not to annotate this\n> > specifically.\n> \n> We have such a comment at the very type of the block comment above\n> `struct object_entry`'s definition:\n> \n>     \"The size of struct nearly determines pack-object's memory\n>     consumption. This struct is packed tight for that reason. When you\n>     add or reorder something in this struct, think a bit about this\".\n> \n> thanks to Duy back in 3b13a5f263 (pack-objects: reorder members to\n> shrink struct object_entry, 2018-04-14).\n\nOh, indeed. Then I withdraw my (non-)complaint. :)\n\n-Peff\n"}]}