git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Patrick Steinhardt <ps@pks.im>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Aaron Plattner <aplattner@nvidia.com>
Subject: Re: [PATCH] odb: do not use "blank" substitute for NULL
Date: Thu, 18 Dec 2025 07:31:32 +0100	[thread overview]
Message-ID: <aUOfxNGdkJe8ARM1@pks.im> (raw)
In-Reply-To: <xmqqpl8cxy0j.fsf@gitster.g>

On Thu, Dec 18, 2025 at 12:35:40PM +0900, Junio C Hamano wrote:
> When various *object_info() functions are given an extended object
> info structure as NULL by a caller that does not want any details,
> the code uses a file-scope static blank_oi to pass it down to the
> helper functions they use, to avoid handling NULL specifically.
> 
> The ps/object-read-stream topic graduated to 'master' recently
> however had a bug that assumed that two identically named file-scope
> static variables in two functions are the same, which of course is
> not the case.  This made "git commit" take 0.38 seconds to 1508
> seconds in some case, as reported by Aaron Plattner here:
> 
>   https://lore.kernel.org/git/f4ba7e89-4717-4b36-921f-56537131fd69@nvidia.com/
> 
> We _could_ move the blank_oi variable to a global scope in BSS to
> fix this regression, but explicitly handling the NULL is a much
> safer fix.  It would also reduce the chance of errors that somebody
> accidentally writes into blank_oi, making its contents dirty, which
> potentially will make subsequent calls into the callpath misbehave.
> 
> By explicitly handling NULL input, we no longer have to worry about
> it.

Thanks for handling this, Junio!

I've sent out an alternative fix via a patch series that I already had
cooking locally in [1]. It also goes a bit further than your series as
it also recognizes the case where the caller passes a blank object info
again.

That series also contains some other fixes related to reading object
info where we had been inconsistent with returned results, and another
performance improvement where we can skip unpacking packed objects.

I'm happy to go either route though -- I can hold off my series for a
bit longer and rebase it on top of your fix, or we replace your fix with
my series. Just let me know your preference.

Thanks!

Patrick

[1]: <20251218-b4-pks-odb-read-object-info-improvements-v1-7-81c8368492be@pks.im>

  parent reply	other threads:[~2025-12-18  6:31 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-12-18  3:35 [PATCH] odb: do not use "blank" substitute for NULL Junio C Hamano
2025-12-18  4:51 ` Aaron Plattner
2025-12-18  8:02   ` Kristoffer Haugsbakk
2025-12-18 10:59     ` Carlo Marcelo Arenas Belón
2025-12-19  7:39       ` Kristoffer Haugsbakk
2025-12-19 12:25         ` Junio C Hamano
2025-12-18  6:31 ` Patrick Steinhardt [this message]
2025-12-18  8:50 ` Patrick Steinhardt

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=aUOfxNGdkJe8ARM1@pks.im \
    --to=ps@pks.im \
    --cc=aplattner@nvidia.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).