Re: [PATCH 4/4] maple_tree: Simplify the code of mas_mab_cp()

From: "Liam R. Howlett" <Liam.Howlett@Oracle.com>
To: Peng Zhang <zhangpeng.00@bytedance.com>
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	maple-tree@lists.infradead.org
Subject: Re: [PATCH 4/4] maple_tree: Simplify the code of mas_mab_cp()
Date: Fri, 10 Mar 2023 13:45:01 -0500	[thread overview]
Message-ID: <20230310184501.6cdc34ask7fqelxq@revolver> (raw)
In-Reply-To: <20230310140848.94485-5-zhangpeng.00@bytedance.com>

* Peng Zhang <zhangpeng.00@bytedance.com> [230310 09:09]:
> Simplify the code of mas_mab_cp(), and improve readability.
> No change in functionality.
> 
> Signed-off-by: Peng Zhang <zhangpeng.00@bytedance.com>
> ---
>  lib/maple_tree.c | 24 +++++-------------------
>  1 file changed, 5 insertions(+), 19 deletions(-)
> 
> diff --git a/lib/maple_tree.c b/lib/maple_tree.c
> index de43ff19da72..688b062728a2 100644
> --- a/lib/maple_tree.c
> +++ b/lib/maple_tree.c
> @@ -1914,32 +1914,18 @@ static inline void mas_mab_cp(struct ma_state *mas, unsigned char mas_start,
>  	void __rcu **slots;
>  	unsigned long *pivots, *gaps;
>  	int i = mas_start, j = mab_start;
> -	unsigned char piv_end;
>  
>  	node = mas_mn(mas);
>  	mt = mte_node_type(mas->node);
>  	pivots = ma_pivots(node, mt);
> -	if (!i) {
> -		b_node->pivot[j] = pivots[i++];
> -		if (unlikely(i > mas_end))
> -			goto complete;
> -		j++;
> -	}
>  
> -	piv_end = min(mas_end, mt_pivots[mt]);
> -	for (; i < piv_end; i++, j++) {
> -		b_node->pivot[j] = pivots[i];
> -		if (unlikely(!b_node->pivot[j]))
> +	for (; i < min(mas_end, mt_pivots[mt]); i++, j++) {

Please don't inline the min here, it is not improving readability.

> +		if (unlikely(!pivots[i] && i) ||
> +		    unlikely(mas->max == pivots[i]))

By not doing the special case outside the loop, you have added a check
to every loop iteration.  I took these special cases out after profiling
the code with perf.  I get they aren't as easy to read but they are
faster which is important for something executed this much.

You have also made this if statement more complex which is not improving
readability.

>  			break;
> -
> -		if (unlikely(mas->max == b_node->pivot[j]))
> -			goto complete;
> +		b_node->pivot[j] = pivots[i];
>  	}
> -
> -	if (likely(i <= mas_end))
> -		b_node->pivot[j] = mas_safe_pivot(mas, pivots, i, mt);
> -
> -complete:
> +	b_node->pivot[j] = mas_safe_pivot(mas, pivots, i, mt);
>  	b_node->b_end = ++j;
>  	j -= mab_start;
>  	slots = ma_slots(node, mt);
> -- 
> 2.20.1
> 