All of lore.kernel.org
 help / color / mirror / Atom feed
From: Taylor Blau <me@ttaylorr.com>
To: Jeff King <peff@peff.net>
Cc: Taylor Blau <me@ttaylorr.com>,
	git@vger.kernel.org, gitster@pobox.com, pclouds@gmail.com
Subject: Re: [PATCH] pack-objects.h: remove outdated pahole results
Date: Fri, 1 Jul 2022 15:48:19 -0400	[thread overview]
Message-ID: <Yr9Pg9mRQWe1tgE+@nand.local> (raw)
In-Reply-To: <Yr85+tcexG2geaPP@coredump.intra.peff.net>

On Fri, Jul 01, 2022 at 02:16:26PM -0400, Jeff King wrote:
> On Tue, Jun 28, 2022 at 02:30:20PM -0400, Taylor Blau wrote:
>
> > Even though this comment was written in a good spirit, it is updated
> > infrequently enough that is serves to confuse rather than to encourage
> > 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.
>
> I agree the actual numbers aren't helping anybody. We _could_ leave a
> comment that says "we store a lot of these in memory; be careful of
> where and how you add new fields to avoid increasing the struct size".
> And then people can run "pahole" before and after their changes.
>
> But then that is also true of other structs (like "struct object"), and
> we do not bother there. So it probably is fine not to annotate this
> specifically.

We have such a comment at the very type of the block comment above
`struct object_entry`'s definition:

    "The size of struct nearly determines pack-object's memory
    consumption. This struct is packed tight for that reason. When you
    add or reorder something in this struct, think a bit about this".

thanks to Duy back in 3b13a5f263 (pack-objects: reorder members to
shrink struct object_entry, 2018-04-14).

> Speaking of which, I suspect quite a lot of memory could be saved if
> "pack-objects --revs" freed the object structs it allocates during its
> traversal. Unless we're generating bitmaps, I don't think they get used
> again after the initial packing list is generated. At peak you'd
> still be storing all of the object_entry structs alongside the objects
> as you finish the traversal, but it wouldn't overlap with any memory
> used for the delta search, and of course we'd be at that peak for a much
> smaller time.
>
> Not a blocker for your patch obviously, but maybe a fun experiment in an
> adjacent area. Possibly even an ambitious #leftoverbits opportunity. :)

Challenge accepted! ;-)

Thanks,
Taylor

  reply	other threads:[~2022-07-01 19:48 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-06-28 18:30 [PATCH] pack-objects.h: remove outdated pahole results Taylor Blau
2022-06-28 20:03 ` Derrick Stolee
2022-06-28 21:04 ` brian m. carlson
2022-06-28 21:26   ` Taylor Blau
2022-07-01 18:16 ` Jeff King
2022-07-01 19:48   ` Taylor Blau [this message]
2022-07-01 21:05     ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Yr9Pg9mRQWe1tgE+@nand.local \
    --to=me@ttaylorr.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=pclouds@gmail.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.