On 5/5/2023 2:46 PM, Taylor Blau wrote:

On Fri, May 05, 2023 at 01:59:31PM -0400, Derrick Stolee wrote:

quoted

On 5/5/2023 1:27 PM, Taylor Blau wrote:

quoted

Here is a reroll of my series to implement a boundary-based bitmap
traversal algorithm that I worked on towards the end of 2021 with Peff.

The algorithm is unchanged from the last version, but the implementation
has been made much more straightforward, thanks to a very helpful
suggestion from Stolee.

Instead of hackily trying to write down all of the UNINTERESTING commits
between the tips and boundary within limit_list(), we can just perform a
commit-only walk, which will give us the set of commits that we need.

Something that didn't seem to get attention in this version was buried
deep in the commentary of my high-level review [1]:

Oops, sorry, I definitely missed this unintentionally and did not mean
to ignore it.

I had to dig deep to find it, even after knowing it was in there
somewhere, so I'm not upset it didn't happen this version.

quoted

quoted

For these reasons, I'm surprised that this patch completely replaces
the old algorithm for the new one. I would prefer to see a config
option that enables this new algorithm. It would be safer to deploy
that way, too.

I still think it would be nice to keep the two algorithms for at least
a little while instead of completely removing the old one. Let's gather
some more evidence and get more reps in with the new algorithm before
making it the new default. I could imagine a scenario where someone
really wants to spend the extra time to make sure none of the objects
reachable from the UNINTERESTING commits are included in the output of
this diff.

[1] https://lore.kernel.org/git/a143150d-7cf5-c697-0e48-0f7af1c6de8f@github.com/ (local)

Hmm. I'm not opposed to keeping the old algorithm around, though I
wonder what the configuration option would look like here. I imagine
that we don't want to support the old algorithm indefinitely, though.

Perhaps something like `pack.useBoundaryBitmapTraversal` (implied by
`feature.experimental`), defaulting to "false", and then eventually
"true"?

This name makes sense to me. Including it in feature.experimental right
away seems like a good idea. Incrementing it to "true" by default after
a single release would make sense, too, since the performance benefits
are so clear. Just important to have that emergency toggle.

Thanks,
-Stolee