Re: [PATCH 06/14] perf c2c: add comparison functions for function view sorting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] total_stores_cmp performs an O(M) traversal of child hists during the sorting phase, introducing an O(N * M log N) scaling bottleneck.
- [Low] Comment in iaddr_symbol_cmp incorrectly describes the sorting order as descending when the implementation sorts in ascending order.
--

commit f2c1f87b66997d6b6a124f1e2e94dfbb11092062
Author: Jiebin Sun <jiebin.sun@intel.com>

perf c2c: add comparison functions for function view sorting

This patch adds sort comparison functions for the function view columns in 
perf c2c. It includes functions like cycles_percent_cmp, iaddr_symbol_cmp, 
and total_stores_cmp to support sorting.

> diff --git a/tools/perf/ui/browsers/c2c-function.c b/tools/perf/ui/browsers/c2c-function.c
> --- a/tools/perf/ui/browsers/c2c-function.c
> +++ b/tools/perf/ui/browsers/c2c-function.c
[ ... ]
> +static __maybe_unused int64_t
> +iaddr_symbol_cmp(struct perf_hpp_fmt *fmt __maybe_unused,
> +		 struct hist_entry *left, struct hist_entry *right)
> +{
[ ... ]
> +	left_iaddr = hist_entry__iaddr(left);
> +	right_iaddr = hist_entry__iaddr(right);
> +
> +	/*
> +	 * Descending by instruction address (same direction as sort__iaddr_cmp(),
> +	 * which also returns r - l): the expression is +1 when left < right.

[Severity: Low]
This isn't a bug, but the comment describes the sorting order as descending,
while returning +1 when left < right instructs the sorting algorithm to place
smaller elements before larger ones, resulting in an ascending order. 

Should the comment be updated to accurately reflect the actual sort order?

> +	 * Uses hist_entry__iaddr(), which falls back to he->ip when mem_info is
> +	 * NULL, so it matches what iaddr_symbol_entry() displays.
> +	 */
> +	return (left_iaddr < right_iaddr) - (left_iaddr > right_iaddr);
> +}
[ ... ]
> +static __maybe_unused int64_t
> +total_stores_cmp(struct perf_hpp_fmt *fmt __maybe_unused,
> +		 struct hist_entry *left, struct hist_entry *right)
> +{
> +	struct c2c_hist_entry *c2c_left = container_of(left, struct c2c_hist_entry, he);
> +	struct c2c_hist_entry *c2c_right = container_of(right, struct c2c_hist_entry, he);
> +	u64 left_store, right_store;
> +
> +	/* Match total_stores_entry(): L1 sums child stores, L2/L3 use their own. */
> +	left_store = left->parent_he ? (u64)c2c_left->stats.store :
> +				       hist_entry__child_stores(left);
> +	right_store = right->parent_he ? (u64)c2c_right->stats.store :
> +					 hist_entry__child_stores(right);

[Severity: Medium]
Will this cause a severe UI slowdown or hang on large profiles? 

During the sorting phase, the tree resort calls total_stores_cmp for node
comparisons. Since hist_entry__child_stores() performs a full traversal of
child entries, this introduces a scaling bottleneck on every comparison. 

Can the sum be cached instead of dynamically recalculating it by iterating
through children over and over during the tree sort?

> +
> +	return (left_store > right_store) - (left_store < right_store);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260626070355.1556721-1-jiebin.sun@intel.com?part=6