Re: [PATCH v4 10/19] unpack-trees: add basic support for parallel checkout

[Christian Couder <christian.couder@gmail.com>]

On Wed, Nov 4, 2020 at 9:34 PM Matheus Tavares
<matheus.bernardino@usp.br> wrote:
>
> This new interface allows us to enqueue some of the entries being
> checked out to later call write_entry() for them in parallel. For now,
> the parallel checkout machinery is enabled by default and there is no
> user configuration, but run_parallel_checkout() just writes the queued
> entries in sequence (without spawning additional workers). The next
> patch will actually implement the parallelism and, later, we will make
> it configurable.

I would think that it might be more logical to first add a
configuration that does nothing, then add writing the queued entries
in sequence without parallelism, and then add actual parallelism.

> When there are path collisions among the entries being written (which
> can happen e.g. with case-sensitive files in case-insensitive file
> systems), the parallel checkout code detects the problem and marks the
> item with PC_ITEM_COLLIDED.

Is this needed in this step that only writes the queued entries in
sequence without parallelism, or could this be added later, before the
step that adds actual parallelism?

> Later, these items are sequentially fed to
> checkout_entry() again. This is similar to the way the sequential code
> deals with collisions, overwriting the previously checked out entries
> with the subsequent ones. The only difference is that, when we start
> writing the entries in parallel, we won't be able to determine which of
> the colliding entries will survive on disk (for the sequential
> algorithm, it is always the last one).

So I guess that PC_ITEM_COLLIDED will then be used to decide which
entries will not be checked out in parallel?

> I also experimented with the idea of not overwriting colliding entries,
> and it seemed to work well in my simple tests.

There are a number of co-author of this patch, so it's not very clear
who "I" is. Maybe:

"The idea of not overwriting colliding entries seemed to work well in
simple tests, however ..."

> However, because just one
> entry of each colliding group would be actually written, the others
> would have null lstat() fields on the index. This might not be a problem
> by itself, but it could cause performance penalties for subsequent
> commands that need to refresh the index: when the st_size value cached
> is 0, read-cache.c:ie_modified() will go to the filesystem to see if the
> contents match. As mentioned in the function:
>
>     * Immediately after read-tree or update-index --cacheinfo,
>     * the length field is zero, as we have never even read the
>     * lstat(2) information once, and we cannot trust DATA_CHANGED
>     * returned by ie_match_stat() which in turn was returned by
>     * ce_match_stat_basic() to signal that the filesize of the
>     * blob changed.  We have to actually go to the filesystem to
>     * see if the contents match, and if so, should answer "unchanged".
>
> So, if we have N entries in a colliding group and we decide to write and
> lstat() only one of them, every subsequent git-status will have to read,
> convert, and hash the written file N - 1 times, to check that the N - 1
> unwritten entries are dirty. By checking out all colliding entries (like
> the sequential code does), we only pay the overhead once.

Ok.

>  5 files changed, 410 insertions(+), 3 deletions(-)

It looks like a lot of new code in one patch/commit, which is why it
might be interesting to split it.

> @@ -7,6 +7,7 @@
>  #include "progress.h"
>  #include "fsmonitor.h"
>  #include "entry.h"
> +#include "parallel-checkout.h"
>
>  static void create_directories(const char *path, int path_len,
>                                const struct checkout *state)
> @@ -426,8 +427,17 @@ static void mark_colliding_entries(const struct checkout *state,
>         for (i = 0; i < state->istate->cache_nr; i++) {
>                 struct cache_entry *dup = state->istate->cache[i];
>
> -               if (dup == ce)
> -                       break;
> +               if (dup == ce) {
> +                       /*
> +                        * Parallel checkout creates the files in no particular
> +                        * order. So the other side of the collision may appear
> +                        * after the given cache_entry in the array.
> +                        */

Is it really the case right now that the code creates files in no
particular order or will that be the case later when actual
parallelism is implemented?

> +                       if (parallel_checkout_status() == PC_RUNNING)
> +                               continue;
> +                       else
> +                               break;
> +               }

> +struct parallel_checkout_item {
> +       /* pointer to a istate->cache[] entry. Not owned by us. */
> +       struct cache_entry *ce;
> +       struct conv_attrs ca;
> +       struct stat st;
> +       enum pc_item_status status;
> +};

"item" seems not very clear to me. If there is only one
parallel_checkout_item for each cache_entry then it might be better to
use "parallel_checkout_entry" instead of "parallel_checkout_item".

> +enum pc_status {
> +       PC_UNINITIALIZED = 0,
> +       PC_ACCEPTING_ENTRIES,
> +       PC_RUNNING,
> +};
> +
> +enum pc_status parallel_checkout_status(void);
> +void init_parallel_checkout(void);

Maybe a comment to tell what the above function does could be helpful.
If I had to guess, I would write something like:

/*
 * Put parallel checkout into the PC_ACCEPTING_ENTRIES state.
 * Should be used only when in the PC_UNINITIALIZED state.
 */

> +/*
> + * Return -1 if parallel checkout is currently not enabled

Is it "enabled" or "initialized" or "configured" here? Does it refer
to `enum pc_status` or a config option or something else? Looking at
the code, it is testing if the status PC_ACCEPTING_ENTRIES, so
perhaps: s/not enabled/not accepting entries/

> or if the entry is
> + * not eligible for parallel checkout. Otherwise, enqueue the entry for later
> + * write and return 0.
> + */
> +int enqueue_checkout(struct cache_entry *ce, struct conv_attrs *ca);