On 5/5/2023 2:46 PM, Taylor Blau wrote: > On Fri, May 05, 2023 at 01:59:31PM -0400, Derrick Stolee wrote: >> On 5/5/2023 1:27 PM, Taylor Blau wrote: >>> 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. >>> 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@xxxxxxxxxx/ > > 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
