Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`

From: Elijah Newren <newren@gmail.com>
Date: Nov 10, 2023, 23:51 UTC
Message-ID: <CABPp-BEfy9VOvimP9==ry_rZXu=metOQ8s=_-XiG_Pdx9c06Ww@mail.gmail.com>
In-Reply-To: <3595db76a525fcebc3c896e231246704b044310c.1698101088.git.me@ttaylorr.com>

Hi,

Sorry for taking so long to find some time to review. And sorry for the bad news, but...

On Mon, Oct 23, 2023 at 3:45 PM Taylor Blau <me@ttaylorr.com> wrote:

Show 25 quoted lines

>
> When using merge-tree often within a repository[^1], it is possible to
> generate a relatively large number of loose objects, which can result in
> degraded performance, and inode exhaustion in extreme cases.
>
> Building on the functionality introduced in previous commits, the
> bulk-checkin machinery now has support to write arbitrary blob and tree
> objects which are small enough to be held in-core. We can use this to
> write any blob/tree objects generated by ORT into a separate pack
> instead of writing them out individually as loose.
>
> This functionality is gated behind a new `--write-pack` option to
> `merge-tree` that works with the (non-deprecated) `--write-tree` mode.
>
> The implementation is relatively straightforward. There are two spots
> within the ORT mechanism where we call `write_object_file()`, one for
> content differences within blobs, and another to assemble any new trees
> necessary to construct the merge. In each of those locations,
> conditionally replace calls to `write_object_file()` with
> `index_blob_bulk_checkin_incore()` or `index_tree_bulk_checkin_incore()`
> depending on which kind of object we are writing.
>
> The only remaining task is to begin and end the transaction necessary to
> initialize the bulk-checkin machinery, and move any new pack(s) it
> created into the main object store.

I believe the above is built on an assumption that any objects written will not need to be read until after the merge is completed. And we might have a nesting issue too...

Show 24 quoted lines

> @@ -2108,10 +2109,19 @@ static int handle_content_merge(struct merge_options *opt,
>                 if ((merge_status < 0) || !result_buf.ptr)
>                         ret = error(_("failed to execute internal merge"));
>
> -               if (!ret &&
> -                   write_object_file(result_buf.ptr, result_buf.size,
> -                                     OBJ_BLOB, &result->oid))
> -                       ret = error(_("unable to add %s to database"), path);
> +               if (!ret) {
> +                       ret = opt->write_pack
> +                               ? index_blob_bulk_checkin_incore(&result->oid,
> +                                                                result_buf.ptr,
> +                                                                result_buf.size,
> +                                                                path, 1)
> +                               : write_object_file(result_buf.ptr,
> +                                                   result_buf.size,
> +                                                   OBJ_BLOB, &result->oid);
> +                       if (ret)
> +                               ret = error(_("unable to add %s to database"),
> +                                           path);
> +               }
>
>                 free(result_buf.ptr);
>                 if (ret)

This is unsafe; the object may need to be read later within the same merge. Perhaps the simplest example related to your testcase is modifying the middle section of that testcase (I'll highlight where when I comment on the testcase) to read:

    test_commit -C repo base file "$(test_seq 3 5)" &&
    git -C repo branch -M main &&
    git -C repo checkout -b side main &&
    test_commit -C repo side-file file "$(test_seq 1 5)" &&
    test_commit -C repo side-file2 file2 "$(test_seq 1 6)" &&
    git -C repo checkout -b other main &&
    test_commit -C repo other-file file "$(test_seq 3 7)" &&
    git -C repo mv file file2 &&
    git -C repo commit -m other-file2 &&

    find repo/$packdir -type f -name "pack-*.idx" >packs.before &&
    git -C repo merge-tree --write-pack side other &&

In words, what I'm doing here is having both sides modify "file" (the same as you did) but also having one side rename "file"->"file2". The side that doesn't rename "file" also introduces a new "file2". ort needs to merge the three versions of "file" to get a new blob object, and then merge that with the content from the brand new "file2". More complicated cases are possible, of course. Anyway, with the modified testcase above, merge-tree will fail with:

    fatal: unable to read blob object 06e567b11dfdafeaf7d3edcc89864149383aeab6

I think (untested) that you could fix this by creating two packs instead of just one. In particular, add a call to flush_odb_transcation() after the "redo_after_renames" block in merge_ort_nonrecursive_internal(). (It's probably easier to see that you could place the flush_odb_transaction() call inside detect_and_process_renames() just after the process_renames() call, but when redo_after_renames is true you'd end up with three packs instead of just two).

What happens with the odb transaction stuff if no new objects are written before the call to flush_odb_transaction? Will that be a problem?

(Since any tree written will not be re-read within the same merge, the other write_object_file() call you changed does not have the same problem as the one here.)

Show 13 quoted lines

>@@ -4332,9 +4349,13 @@ static int process_entries(struct merge_options *opt,
>                fflush(stdout);
>                BUG("dir_metadata accounting completely off; shouldn't happen");
>        }
>-       if (write_tree(result_oid, &dir_metadata.versions, 0,
>+       if (write_tree(opt, result_oid, &dir_metadata.versions, 0,
>                       opt->repo->hash_algo->rawsz) < 0)
>                ret = -1;
>
> +
> +       if (opt->write_pack)
> +               end_odb_transaction();
> +

Is the end_odb_transaction() here going to fail with an "Unbalanced ODB transaction nesting" when faced with a recursive merge?

Perhaps flushing here, and then calling end_odb_transaction() in merge_finalize(), much as you do in your replay-and-write-pack series, should actually be moved to this commit and included here?

This does mean that for a recursive merge, that you'll get up to 2*N packfiles, where N is the depth of the recursive merge.

Show 10 quoted lines

> +       test_commit -C repo base file "$(test_seq 3 5)" &&
> +       git -C repo branch -M main &&
> +       git -C repo checkout -b side main &&
> +       test_commit -C repo side file "$(test_seq 1 5)" &&
> +       git -C repo checkout -b other main &&
> +       test_commit -C repo other file "$(test_seq 3 7)" &&
> +
> +       find repo/$packdir -type f -name "pack-*.idx" >packs.before &&
> +       tree="$(git -C repo merge-tree --write-pack \
> +               refs/tags/side refs/tags/other)" &&

These were the lines from your testcase that I replaced to show the bug.