Re: [PATCH v1 14/40] perf print-events: Avoid unnecessary strlist

[Liang, Kan]

On 2023-04-26 3:00 a.m., Ian Rogers wrote:
> The strlist in print_hwcache_events holds the event names as they are
> generated, and then it is iterated and printed. This is unnecessary
> and each event can just be printed as it is processed.
> Rename the variable i to res, to be more intention revealing and
> consistent with other code.
> 

Looks good to me.

Reviewed-by: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>

Thanks,
Kan

> Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>> ---
>  tools/perf/util/print-events.c | 60 ++++++++++++++++++----------------
>  1 file changed, 31 insertions(+), 29 deletions(-)
> 
> diff --git a/tools/perf/util/print-events.c b/tools/perf/util/print-events.c
> index 386b1ab0b60e..93bbb868d400 100644
> --- a/tools/perf/util/print-events.c
> +++ b/tools/perf/util/print-events.c
> @@ -226,58 +226,60 @@ void print_sdt_events(const struct print_callbacks *print_cb, void *print_state)
>  
>  int print_hwcache_events(const struct print_callbacks *print_cb, void *print_state)
>  {
> -	struct strlist *evt_name_list = strlist__new(NULL, NULL);
> -	struct str_node *nd;
> +	const char *event_type_descriptor = event_type_descriptors[PERF_TYPE_HW_CACHE];
>  
> -	if (!evt_name_list) {
> -		pr_debug("Failed to allocate new strlist for hwcache events\n");
> -		return -ENOMEM;
> -	}
>  	for (int type = 0; type < PERF_COUNT_HW_CACHE_MAX; type++) {
>  		for (int op = 0; op < PERF_COUNT_HW_CACHE_OP_MAX; op++) {
>  			/* skip invalid cache type */
>  			if (!evsel__is_cache_op_valid(type, op))
>  				continue;
>  
> -			for (int i = 0; i < PERF_COUNT_HW_CACHE_RESULT_MAX; i++) {
> +			for (int res = 0; res < PERF_COUNT_HW_CACHE_RESULT_MAX; res++) {
>  				struct perf_pmu *pmu = NULL;
>  				char name[64];
>  
> -				__evsel__hw_cache_type_op_res_name(type, op, i, name, sizeof(name));
> +				__evsel__hw_cache_type_op_res_name(type, op, res,
> +								   name, sizeof(name));
>  				if (!perf_pmu__has_hybrid()) {
>  					if (is_event_supported(PERF_TYPE_HW_CACHE,
> -							       type | (op << 8) | (i << 16)))
> -						strlist__add(evt_name_list, name);
> +								type | (op << 8) | (res << 16))) {
> +						print_cb->print_event(print_state,
> +								"cache",
> +								/*pmu_name=*/NULL,
> +								name,
> +								/*event_alias=*/NULL,
> +								/*scale_unit=*/NULL,
> +								/*deprecated=*/false,
> +								event_type_descriptor,
> +								/*desc=*/NULL,
> +								/*long_desc=*/NULL,
> +								/*encoding_desc=*/NULL);
> +					}
>  					continue;
>  				}
>  				perf_pmu__for_each_hybrid_pmu(pmu) {
>  					if (is_event_supported(PERF_TYPE_HW_CACHE,
> -					    type | (op << 8) | (i << 16) |
> +					    type | (op << 8) | (res << 16) |
>  					    ((__u64)pmu->type << PERF_PMU_TYPE_SHIFT))) {
>  						char new_name[128];
> -							snprintf(new_name, sizeof(new_name),
> -								 "%s/%s/", pmu->name, name);
> -							strlist__add(evt_name_list, new_name);
> +						snprintf(new_name, sizeof(new_name),
> +							"%s/%s/", pmu->name, name);
> +						print_cb->print_event(print_state,
> +								"cache",
> +								pmu->name,
> +								name,
> +								new_name,
> +								/*scale_unit=*/NULL,
> +								/*deprecated=*/false,
> +								event_type_descriptor,
> +								/*desc=*/NULL,
> +								/*long_desc=*/NULL,
> +								/*encoding_desc=*/NULL);
>  					}
>  				}
>  			}
>  		}
>  	}
> -
> -	strlist__for_each_entry(nd, evt_name_list) {
> -		print_cb->print_event(print_state,
> -				"cache",
> -				/*pmu_name=*/NULL,
> -				nd->s,
> -				/*event_alias=*/NULL,
> -				/*scale_unit=*/NULL,
> -				/*deprecated=*/false,
> -				event_type_descriptors[PERF_TYPE_HW_CACHE],
> -				/*desc=*/NULL,
> -				/*long_desc=*/NULL,
> -				/*encoding_desc=*/NULL);
> -	}
> -	strlist__delete(evt_name_list);
>  	return 0;
>  }
>