Re: [PATCH v3 01/11] bulk-checkin: rebrand plug/unplug APIs as 'odb transactions'

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Mar 24, 2022, 16:10 UTC
Message-ID: <220324.86y20zmi84.gmgdl@evledraar.gmail.com>
In-Reply-To: <53261f0099d53524155464fe79d10f9605fe93aa.1648097906.git.gitgitgadget@gmail.com>

On Thu, Mar 24 2022, Neeraj Singh via GitGitGadget wrote:

Show 32 quoted lines

> From: Neeraj Singh <neerajsi@microsoft.com>
>
> Make it clearer in the naming and documentation of the plug_bulk_checkin
> and unplug_bulk_checkin APIs that they can be thought of as
> a "transaction" to optimize operations on the object database.
>
> Signed-off-by: Neeraj Singh <neerajsi@microsoft.com>
> ---
>  builtin/add.c  |  4 ++--
>  bulk-checkin.c |  4 ++--
>  bulk-checkin.h | 14 ++++++++++++--
>  3 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index 3ffb86a4338..9bf37ceae8e 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -670,7 +670,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  		string_list_clear(&only_match_skip_worktree, 0);
>  	}
>  
> -	plug_bulk_checkin();
> +	begin_odb_transaction();
>  
>  	if (add_renormalize)
>  		exit_status |= renormalize_tracked_files(&pathspec, flags);
> @@ -682,7 +682,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  
>  	if (chmod_arg && pathspec.nr)
>  		exit_status |= chmod_pathspec(&pathspec, chmod_arg[0], show_only);
> -	unplug_bulk_checkin();
> +	end_odb_transaction();

Aside from anything else we've (dis)agreed on, I found this part really odd when hacking on my RFC-on-top, i.e. originally I (wrongly) thought the plug_bulk_checkin() was something that originated with this series which adds the "bulk" mode.

But no, on second inspection it's a thing Junio added a long time ago so that in this case we "stream to N pack" where we'd otherwise add N loose objects.

Which, and I think Junio brought this up in an earlier round, but I didn't fully understand that at the time makes this whole thing quite odd to me.

So first, shouldn't we add this begin_odb_transaction() as a new thing? I.e. surely wanting to do that object target redirection within a given begin/end "scope" should be orthagonal to how fsync() happens within that "scope", though in this case that happens to correspond.

And secondly, per the commit message and comment when it was added in (568508e7657 (bulk-checkin: replace fast-import based implementation, 2011-10-28)) is it something we need *for that purpose* with the series to unpack-objects without malloc()ing the size of the blob[1].

And, if so and orthagonal to that: If we know how to either stream N objects to a PACK (as fast-import does), *and* we now (or SOON) know how to stream loose objects without using size(blob) amounts of memory, doesn't the "optimize fsync()" rather want to make use of the stream-to-pack approach?

I.e. have you tried for the caseses where we create say 1k objects for "git stash" tried to stream those to a pack? How does that compare (both with/without the fsync changes).

I.e. I do worry (also per [2]) that while the whole "bulk fsync" is neat (and I think can use it in either case, to defer object syncs until the "index" or "ref" sync, as my RFC does) I worry that we're adding a bunch of configuration and complexity for something that:

 1. Ultimately isn't all that important, as already for part of it we
    can mostly configure it away. I.e. "git-unpack-objects" v.s. writing
    a pack, cf. transfer.unpackLimit)
 2. We don't have #1 for "add" and "update-index", but if we stream to
    packs there is there any remaining benefit in practice?

1. https://lore.kernel.org/git/cover-v11-0.8-00000000000-20220319T001411Z-avarab@gmail.com/
2. https://lore.kernel.org/git/220323.86fsn8ohg8.gmgdl@evledraar.gmail.com/