From: Lidong Yan <502024330056@smail.nju.edu.cn> Date: Mon, 07 Jul 2025 14:18:45 GMT Subject: Re: [PATCH v4 4/4] bloom: optimize multiple pathspec items in revision traversal Message-ID: <23232D29-CEFA-409D-90E3-F894EC37C1BE@smail.nju.edu.cn> In-Reply-To: Derrick Stolee wrote: > > On 7/4/2025 7:14 AM, Lidong Yan wrote: >> To enable optimize multiple pathspec items in revision traversal, >> return 0 if all pathspec item is literal in forbid_bloom_filters(). >> Add code to initialize and check each pathspec item's bloom_keyvec. >> >> Add new function release_revisions_bloom_keyvecs() to free all bloom >> keyvec owned by rev_info. >> >> Add new test cases in t/t4216-log-bloom.sh to ensure >> - consistent results between the optimization for multiple pathspec >> items using bloom filter and the case without bloom filter >> optimization. >> - does not use bloom filter if any pathspec item is not literal. > > This would be a great time to add some performance statistics when > using this feature with multiple pathspecs on some standard repos (git > and the Linux kernel repo are two good examples). > > We don't have a great performance script for this, since each test > repo will have different paths to use for comparisons, but you can > use 'hyperfine' to assemble your own comparisons before and after > this change and report them here (and in your cover letter). Got it, I will add statistics in the next version. >> + int path_component_nr; > > We can move this into the interior of the loop, right? Yes, move this into loop looks better. > > The size of this diff is unfortunate. I wonder if there could first > be an extraction of this logic to operate on a single pathspec and > bloom_keyvec in a way that would be an obvious code move, then this > patch could call that method in a loop now that we have an array of > bloom_keyvecs. I will try to figure out a way to make diff cleaner. Thanks, Lidong