Re: [PATCH v5 4/5] perf metric: Add utilities to work on ids map.

[Namhyung Kim]

On Wed, Dec 2, 2020 at 4:40 PM Ian Rogers  wrote:
>
> Add utilities to new/free an ids hashmap, as well as to union. Add
> testing of the union. Unioning hashmaps will be used when parsing the
> metric, if a value is known then the hashmap is unnecessary, otherwise
> we need to union together all the event ids to compute their values for
> reporting.
>
> Signed-off-by: Ian Rogers 
> ---
[SNIP]

What about adding a comment that it should not access ids1 and ids2
after this function since they can be released?

> +struct hashmap *ids__union(struct hashmap *ids1, struct hashmap *ids2)
> +{
> +   size_t bkt;
> +   struct hashmap_entry *cur;
> +   int ret;
> +   struct expr_id_data *old_data = NULL;
> +   char *old_key = NULL;
> +
> +   if (!ids1)
> +   return ids2;
> +
> +   if (!ids2)
> +   return ids1;
> +
> +   if (hashmap__size(ids1) <  hashmap__size(ids2)) {
> +   struct hashmap *tmp = ids1;
> +
> +   ids1 = ids2;
> +   ids2 = tmp;
> +   }
> +   hashmap__for_each_entry(ids2, cur, bkt) {
> +   ret = hashmap__set(ids1, cur->key, cur->value,
> +   (const void **)_key, (void **)_data);
> +   free(old_key);
> +   free(old_data);
> +
> +   if (ret)
> +   break;
> +   }
> +   hashmap__free(ids2);
> +   return ids1;
> +}

[PATCH v5 4/5] perf metric: Add utilities to work on ids map.

[Ian Rogers]

Add utilities to new/free an ids hashmap, as well as to union. Add
testing of the union. Unioning hashmaps will be used when parsing the
metric, if a value is known then the hashmap is unnecessary, otherwise
we need to union together all the event ids to compute their values for
reporting.

Signed-off-by: Ian Rogers 
---
 tools/perf/tests/expr.c | 47 ++
 tools/perf/util/expr.c  | 87 +++--
 tools/perf/util/expr.h  |  9 +
 3 files changed, 139 insertions(+), 4 deletions(-)

diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index 7ccb97c73347..1c881bea7fca 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c
@@ -6,6 +6,51 @@
 #include 
 #include 
 
+static int test_ids_union(void)
+{
+   struct hashmap *ids1, *ids2;
+
+   /* Empty union. */
+   ids1 = ids__new();
+   TEST_ASSERT_VAL("ids__new", ids1);
+   ids2 = ids__new();
+   TEST_ASSERT_VAL("ids__new", ids2);
+
+   ids1 = ids__union(ids1, ids2);
+   TEST_ASSERT_EQUAL("union", (int)hashmap__size(ids1), 0);
+
+   /* Union {foo, bar} against {}. */
+   ids2 = ids__new();
+   TEST_ASSERT_VAL("ids__new", ids2);
+
+   TEST_ASSERT_EQUAL("ids__insert", ids__insert(ids1, strdup("foo"), 
NULL), 0);
+   TEST_ASSERT_EQUAL("ids__insert", ids__insert(ids1, strdup("bar"), 
NULL), 0);
+
+   ids1 = ids__union(ids1, ids2);
+   TEST_ASSERT_EQUAL("union", (int)hashmap__size(ids1), 2);
+
+   /* Union {foo, bar} against {foo}. */
+   ids2 = ids__new();
+   TEST_ASSERT_VAL("ids__new", ids2);
+   TEST_ASSERT_EQUAL("ids__insert", ids__insert(ids2, strdup("foo"), 
NULL), 0);
+
+   ids1 = ids__union(ids1, ids2);
+   TEST_ASSERT_EQUAL("union", (int)hashmap__size(ids1), 2);
+
+   /* Union {foo, bar} against {bar,baz}. */
+   ids2 = ids__new();
+   TEST_ASSERT_VAL("ids__new", ids2);
+   TEST_ASSERT_EQUAL("ids__insert", ids__insert(ids2, strdup("bar"), 
NULL), 0);
+   TEST_ASSERT_EQUAL("ids__insert", ids__insert(ids2, strdup("baz"), 
NULL), 0);
+
+   ids1 = ids__union(ids1, ids2);
+   TEST_ASSERT_EQUAL("union", (int)hashmap__size(ids1), 3);
+
+   ids__free(ids1);
+
+   return 0;
+}
+
 static int test(struct expr_parse_ctx *ctx, const char *e, double val2)
 {
double val;
@@ -24,6 +69,8 @@ int test__expr(struct test *t __maybe_unused, int subtest 
__maybe_unused)
int ret;
struct expr_parse_ctx *ctx;
 
+   TEST_ASSERT_EQUAL("ids_union", test_ids_union(), 0);
+
ctx = expr__ctx_new();
TEST_ASSERT_VAL("expr__ctx_new", ctx);
expr__add_id_val(ctx, strdup("FOO"), 1);
diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
index a248d14882cc..1adb6cd202e0 100644
--- a/tools/perf/util/expr.c
+++ b/tools/perf/util/expr.c
@@ -59,8 +59,48 @@ static bool key_equal(const void *key1, const void *key2,
return !strcmp((const char *)key1, (const char *)key2);
 }
 
-/* Caller must make sure id is allocated */
-int expr__add_id(struct expr_parse_ctx *ctx, const char *id)
+struct hashmap *ids__new(void)
+{
+   return hashmap__new(key_hash, key_equal, NULL);
+}
+
+void ids__free(struct hashmap *ids)
+{
+   struct hashmap_entry *cur;
+   size_t bkt;
+
+   if (ids == NULL)
+   return;
+
+#ifdef PARSER_DEBUG
+   fprintf(stderr, "freeing ids: ");
+   ids__print(ids);
+   fprintf(stderr, "\n");
+#endif
+
+   hashmap__for_each_entry(ids, cur, bkt) {
+   free((char *)cur->key);
+   free(cur->value);
+   }
+
+   hashmap__free(ids);
+}
+
+void ids__print(struct hashmap *ids)
+{
+   size_t bkt;
+   struct hashmap_entry *cur;
+
+   if (!ids)
+   return;
+
+   hashmap__for_each_entry(ids, cur, bkt) {
+   fprintf(stderr, "key:%s, ", (const char *)cur->key);
+   }
+}
+
+int ids__insert(struct hashmap *ids, const char *id,
+   struct expr_id *parent)
 {
struct expr_id_data *data_ptr = NULL, *old_data = NULL;
char *old_key = NULL;
@@ -70,10 +110,10 @@ int expr__add_id(struct expr_parse_ctx *ctx, const char 
*id)
if (!data_ptr)
return -ENOMEM;
 
-   data_ptr->parent = ctx->parent;
+   data_ptr->parent = parent;
data_ptr->kind = EXPR_ID_DATA__PARENT;
 
-   ret = hashmap__set(ctx->ids, id, data_ptr,
+   ret = hashmap__set(ids, id, data_ptr,
   (const void **)_key, (void **)_data);
if (ret)
free(data_ptr);
@@ -82,6 +122,45 @@ int expr__add_id(struct expr_parse_ctx *ctx, const char *id)
return ret;
 }
 
+struct hashmap *ids__union(struct hashmap *ids1, struct hashmap *ids2)
+{
+   size_t bkt;
+   struct hashmap_entry *cur;
+   int ret;
+   struct expr_id_data *old_data = NULL;
+   char *old_key = NULL;
+
+   if (!ids1)
+   return ids2;
+
+   if (!ids2)
+   return