Re: [PATCH v2 6/7] merge-ort: avoid recursing into directories when we don't need to

From: Derrick Stolee <stolee@gmail.com>
Date: Jul 15, 2021, 14:55 UTC
Message-ID: <24c5ef26-f5d3-fb0e-b2da-24cdac90450d@gmail.com>
In-Reply-To: <3409a6cd631deb361d3ecb94491c0c297c52fb53.1626204784.git.gitgitgadget@gmail.com>

On 7/13/2021 3:33 PM, Elijah Newren via GitGitGadget wrote:

> From: Elijah Newren <newren@gmail.com>

...

Show 6 quoted lines

> For the testcases mentioned in commit 557ac0350d ("merge-ort: begin
> performance work; instrument with trace2_region_* calls", 2020-10-28),
> this change improves the performance as follows:
> 
>                             Before                  After
>     no-renames:        5.235 s ±  0.042 s   205.1  ms ±  3.8  ms

Wow! This is quite the savings, and reinforces that when no renames exist we are likely to hit this optimization.

>     mega-renames:      9.419 s ±  0.107 s     1.564 s ±  0.010 s

I'm surprised that this one works so well, too. Clearly, there must be a lot of directories that can be skipped despite many renames existing.

>     just-one-mega:   480.1  ms ±  3.9  ms   479.5  ms ±  3.9  ms

And no overhead added to this case. Good.

Show 11 quoted lines

> +		/* Loop over the set of paths we need to know rename info for */
> +		strset_for_each_entry(&renames->relevant_sources[side],
> +				      &iter, entry) {
> +			char *rename_target, *dir, *dir_marker;
> +			struct strmap_entry *e;
> +
> +			/*
> +			 * if we don't know delete/rename info for this path,
> +			 * then we need to recurse into all trees to get all
> +			 * adds to make sure we have it.
> +			 */

super-nit: s/if/If/

Otherwise, I can't speak to the code being 100% correct because this area is so dense. I find the code to be well organized, which will help finding and fixing any potential bugs that might show up in strange corner cases later.

Thanks, -Stolee