On Fri, May 15, 2020 at 12:39 PM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > On Fri, May 15, 2020 at 9:51 AM Ian Rogers <irogers@xxxxxxxxxx> wrote: > > > > Use a hashmap between a char* string and a double* value. While bpf's > > hashmap entries are size_t in size, we can't guarantee sizeof(size_t) >= > > sizeof(double). Avoid a memory allocation when gathering ids by making 0.0 > > a special value encoded as NULL. > > > > Original map suggestion by Andi Kleen: > > https://lore.kernel.org/lkml/20200224210308.GQ160988@xxxxxxxxxxxxxxxxxxxx/ > > and seconded by Jiri Olsa: > > https://lore.kernel.org/lkml/20200423112915.GH1136647@krava/ > > > > Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx> > > --- > > tools/perf/tests/expr.c | 40 ++++++----- > > tools/perf/tests/pmu-events.c | 25 +++---- > > tools/perf/util/expr.c | 129 +++++++++++++++++++--------------- > > tools/perf/util/expr.h | 26 +++---- > > tools/perf/util/expr.y | 22 +----- > > tools/perf/util/metricgroup.c | 87 +++++++++++------------ > > tools/perf/util/stat-shadow.c | 49 ++++++++----- > > 7 files changed, 197 insertions(+), 181 deletions(-) > > > > diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c > > index 3f742612776a..5e606fd5a2c6 100644 > > --- a/tools/perf/tests/expr.c > > +++ b/tools/perf/tests/expr.c > > @@ -19,11 +19,9 @@ static int test(struct expr_parse_ctx *ctx, const char *e, double val2) > > int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused) > > { > > const char *p; > > - const char **other; > > - double val; > > - int i, ret; > > + double val, *val_ptr; > > + int ret; > > struct expr_parse_ctx ctx; > > - int num_other; > > > > expr__ctx_init(&ctx); > > expr__add_id(&ctx, "FOO", 1); > > @@ -52,25 +50,29 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused) > > ret = expr__parse(&val, &ctx, p, 1); > > TEST_ASSERT_VAL("missing operand", ret == -1); > > > > + hashmap__clear(&ctx.ids); > > hashmap__clear() will free up memory allocated for hashmap itself and > hash entries, but not keys/values. Unless it's happening somewhere > else, you'll need to do something similar to expr__ctx_clear() below? In this case the const char* keys come from the literals added on lines 27 and 28 and so didn't need free-ing - which is what expr__ctx_clear() does. I've made these literals strdups and switched to expr__ctx_clear() as you suggest, as this is more reflective of the real use. > Same below for another "lone" hashmap_clear() call. This was a memory leak, thanks! Ian > > TEST_ASSERT_VAL("find other", > > - expr__find_other("FOO + BAR + BAZ + BOZO", "FOO", &other, &num_other, 1) == 0); > > [...]
