Re: [PATCH v2 7/7] perf expr: Migrate expr ids table to a hashmap

From: Jiri Olsa <jolsa@redhat.com>
To: Ian Rogers <irogers@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Namhyung Kim <namhyung@kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
	Yonghong Song <yhs@fb.com>, Andrii Nakryiko <andriin@fb.com>,
	John Fastabend <john.fastabend@gmail.com>,
	KP Singh <kpsingh@chromium.org>, Kajol Jain <kjain@linux.ibm.com>,
	Andi Kleen <ak@linux.intel.com>,
	John Garry <john.garry@huawei.com>,
	Jin Yao <yao.jin@linux.intel.com>,
	Kan Liang <kan.liang@linux.intel.com>,
	Cong Wang <xiyou.wangcong@gmail.com>,
	Kim Phillips <kim.phillips@amd.com>,
	Adrian Hunter <adrian.hunter@intel.com>,
	Leo Yan <leo.yan@linaro.org>,
	linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
	bpf@vger.kernel.org, Stephane Eranian <eranian@google.com>
Subject: Re: [PATCH v2 7/7] perf expr: Migrate expr ids table to a hashmap
Date: Fri, 15 May 2020 21:41:15 +0200	[thread overview]
Message-ID: <20200515194115.GA3577540@krava> (raw)
In-Reply-To: <20200515165007.217120-8-irogers@google.com>

On Fri, May 15, 2020 at 09:50:07AM -0700, Ian Rogers wrote:

SNIP

> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index b071df373f8b..37be5a368d6e 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -85,8 +85,7 @@ static void metricgroup__rblist_init(struct rblist *metric_events)
>  
>  struct egroup {
>  	struct list_head nd;
> -	int idnum;
> -	const char **ids;
> +	struct expr_parse_ctx pctx;
>  	const char *metric_name;
>  	const char *metric_expr;
>  	const char *metric_unit;
> @@ -94,19 +93,21 @@ struct egroup {
>  };
>  
>  static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> -				      const char **ids,
> -				      int idnum,
> +				      struct expr_parse_ctx *pctx,
>  				      struct evsel **metric_events,
>  				      bool *evlist_used)
>  {
>  	struct evsel *ev;
> -	int i = 0, j = 0;
>  	bool leader_found;
> +	const size_t idnum = hashmap__size(&pctx->ids);
> +	size_t i = 0;
> +	int j = 0;
> +	double *val_ptr;
>  
>  	evlist__for_each_entry (perf_evlist, ev) {
>  		if (evlist_used[j++])
>  			continue;
> -		if (!strcmp(ev->name, ids[i])) {
> +		if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr)) {

hum, you sure it's doing the same thing as before?

hashmap__find will succede all the time in here, while the
previous code was looking for the start of the group ...
the logic in here is little convoluted, so maybe I'm
missing some point in here ;-)

jirka

>  			if (!metric_events[i])
>  				metric_events[i] = ev;
>  			i++;
> @@ -118,7 +119,8 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
>  			memset(metric_events, 0,
>  				sizeof(struct evsel *) * idnum);
>  
> -			if (!strcmp(ev->name, ids[i])) {
> +			if (hashmap__find(&pctx->ids, ev->name,
> +					  (void **)&val_ptr)) {
>  				if (!metric_events[i])
>  					metric_events[i] = ev;

SNIP