Re: [PATCH v2 24/24] pack-bitmap-write: better reuse bitmaps
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Dec 7, 2020, 18:24 UTC
- Message-ID
- <20201207182418.3034961-1-jonathantanmy@google.com>
- In-Reply-To
- <39441f40-f496-af81-87c1-9d7e04fdef20@gmail.com>
Show 24 quoted lines
> On 12/2/2020 11:35 AM, Taylor Blau wrote:
> > On Wed, Dec 02, 2020 at 12:08:08AM -0800, Jonathan Tan wrote:
> >>> + c_ent->maximal = 1;
> >>> + p = NULL;
> >>
> >> Here, we're setting maximal without also setting a bit in this commit's
> >> commit_mask. This is fine because we're not propagating this commit's
> >> commit_mask to any parents (we're not continuing the walk from this
> >> commit), but it seems like a code smell. Suggested fix is below.
> >>
> >>> + }
> >>> +
> >>> if (c_ent->maximal) {
> >>> num_maximal++;
> >>> ALLOC_GROW(bb->commits, bb->commits_nr + 1, bb->commits_alloc);
> >>> bb->commits[bb->commits_nr++] = commit;
> >>> }
> >>
> >> As far as I can tell, this means that this commit occupies a bit
> >> position in the commit mask that it doesn't need. Could this go into a
> >> separate list instead, to be appended to bb->commits at the very end?
>
> I don't see any value in having a second list here. That only makes
> things more complicated.It does make things more complicated, but it could help shrink commit bitmasks (which seem to be a concern, according to patch 23).
Suppose num_maximal was 3 and we encountered such a commit (not selected, but has an old bitmap). So we increment num_maximal. Then, we encounter a selected commit. That commit would then have a bitmask of ???01. If we had not incremented num_maximal (which would require a second list), then the bitmask would be ???1.
Show 11 quoted lines
> >> We could even skip the whole maximal stuff (for commits with existing > >> bitmaps) and replace "c_ent->maximal = 1;" above with "add to list that > >> we're going to append to bb->commits at the very end". That has the > >> advantage of not having to redefine "maximal". > > > > Hmm. I'd trust Stolee's opinion over mine here, so I'll be curious what > > he has to say. > > It would be equivalent to add it to the list and then continuing the > loop instead of piggy-backing on the if (c_ent->maximal) block, followed > by a trivial loop over the (nullified) parents.
That is true. This suggestion was for code clarity, not for correctness.