Re: [PATCH v4 3/5] unpack-trees: optimize walking same trees with cache-tree
- From
Duy Nguyen <pclouds@gmail.com>
- Date
- Aug 15, 2018, 16:38 UTC
- Message-ID
- <CACsJy8BFKzQurH6v-geY_ZJjtwF+S2YnZQA=Q6FajLm=GvHjiw@mail.gmail.com>
- In-Reply-To
- <f3403347-607b-b67c-297c-eeb9190a7de7@gmail.com>
On Mon, Aug 13, 2018 at 8:58 PM Ben Peart <peartben@gmail.com> wrote:
Show 30 quoted lines
> > + *
> > + * D/F conflicts and higher stage entries are not a concern
> > + * because cache-tree would be invalidated and we would never
> > + * get here in the first place.
> > + */
> > + for (i = 0; i < nr_entries; i++) {
> > + struct cache_entry *tree_ce;
> > + int len, rc;
> > +
> > + src[0] = o->src_index->cache[pos + i];
> > +
> > + len = ce_namelen(src[0]);
> > + tree_ce = xcalloc(1, cache_entry_size(len));
> > +
> > + tree_ce->ce_mode = src[0]->ce_mode;
> > + tree_ce->ce_flags = create_ce_flags(0);
> > + tree_ce->ce_namelen = len;
> > + oidcpy(&tree_ce->oid, &src[0]->oid);
> > + memcpy(tree_ce->name, src[0]->name, len + 1);
> > +
> > + for (d = 1; d <= nr_names; d++)
> > + src[d] = tree_ce;
> > +
> > + rc = call_unpack_fn((const struct cache_entry * const *)src, o);
>
> I don't fully understand why this is still necessary since "we detect
> that all trees are the same as cache-tree at this path." I do know
> (because I tried it :)) that if we don't actually call the unpack
> function the patch fails a bunch of tests so clearly something important
> is being missed.Yeah because removing this line assumes n-way logic, which most likely means "use the index version if all trees are the same as the index" but it's not necessarily true. There could be flags that make n-way behave differently. And even if we make that assumption, we need to copy src[0] to o->result (heh I tried that "skip call_unpack_fn" thing too when I thought this would be the same as the diff-index --cached optimization path, and only realized copying to o->result was needed afterwards).
-- Duy