RE: [PATCH V6 1/1] perf tool: perf diff support for different binaries

[Liang, Kan]

> Em Tue, Dec 02, 2014 at 10:39:18AM -0500, kan.liang@xxxxxxxxx escreveu:
> > From: Kan Liang <kan.liang@xxxxxxxxx>
> >
> > Currently, the perf diff only works with same binaries. That's because
> > it compares the symbol start address. It doesn't work if the perf.data
> > comes from different binaries. This patch matches the function names.
> 
> Sorry Kan, I saw these patches, its just that it the above statement looked
> completely strange to me, i.e. of course it should look at the dso name,
> then at the symbol name, comparing via addresses makes no sense to me,
> so I kept leaving this patch to look after processing other patches and doing
> other things, then the year end holidays, etc.
> 
> So now I'm looking at old code, from 2009:
> 
>   commit 604c5c92972dcb4b98be34775452d09a5d4ec248
>   Author: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
>   Date:   Wed Dec 16 14:09:53 2009 -0200
> 
>     perf diff: Change the default sort order to "dso,symbol"
> 
>     This is a more intuitive / more meaningful default:
> 
>     $ perf diff | head -8
>          9.02%     +1.00%  libc-2.10.1.so               [.] _IO_vfprintf_internal
>          2.91%     -1.00%  [kernel]                     [k] __kmalloc
>          2.85%     -1.00%  [kernel]                     [k] ext4_htree_store_dirent
>          1.99%     -1.00%  [kernel]                     [k] _atomic_dec_and_lock
>          2.44%             [kernel]
>     $
> 
>     Suggested-by: Ingo Molnar <mingo@xxxxxxx>
> 
> and I see:
> 
> static void perf_session__match_hists(struct perf_session *old_session,
>                                       struct perf_session *new_session) {
>         struct rb_node *nd;
> 
>         perf_session__resort_by_name(old_session);
> 
>         for (nd = rb_first(&new_session->hists); nd; nd = rb_next(nd)) {
>                 struct hist_entry *pos = rb_entry(nd, struct hist_entry, rb_node);
>                 pos->pair = perf_session__find_hist_entry_by_name(old_session,
> pos);
>         }
> }
> 
> Ok, it will process all samples, store everything in hist_entries, etc, then
> traverse all entries in the most recent perf.data file and look for a pair, _by
> name_:
> 
> static struct hist_entry *
> perf_session__find_hist_entry_by_name(struct perf_session *self,
>                                       struct hist_entry *he) {
>         struct rb_node *n = self->hists.rb_node;
> 
>         while (n) {
>                 struct hist_entry *iter = rb_entry(n, struct hist_entry, rb_node);
>                 int cmp = strcmp(he->map->dso->name, iter->map->dso->name);
> 
>                 if (cmp > 0)
>                         n = n->rb_left;
>                 else if (cmp < 0)
>                         n = n->rb_right;
>                 else {
>                         cmp = strcmp(he->sym->name, iter->sym->name);
>                         if (cmp > 0)
>                                 n = n->rb_left;
>                         else if (cmp < 0)
>                                 n = n->rb_right;
>                         else
>                                 return iter;
>                 }
>         }
> 
>         return NULL;
> }
> 
> See? Resort the old session by "dso,symbol_name", then go on pairing
> them.
> 
> So at some point this got broken :-\
> 
> I.e. this is not changing an existing correct behaviour, but fixing a bug, i.e.
> the changeset comment confused me :-\
> 
> Now I'm trying to figure out _when_ this got broken, what was the
> reasoning for that code to have changed in a way that made it not match
> by dso_name, symbol_name.
> 


It looks it got broken just after two weeks later in 2009. 
perf_session__find_hist_entry_by_name was replaced by hist_entry__cmp.
After that it doesn't compares the name any more.


commit 9c443dfdd31eddea6cbe6ee0ca469fbcc4e1dc3b
Author: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
Date:   Mon Dec 28 22:48:36 2009 -0200

    perf diff: Fix support for all --sort combinations

@@ -122,29 +112,28 @@ static void perf_session__resort_by_name(struct perf_session *self)
        self->hists = tmp;
 }

+static void perf_session__set_hist_entries_positions(struct perf_session *self)
+{
+       perf_session__output_resort(self, self->events_stats.total);
+       perf_session__resort_hist_entries(self);
+}
+
 static struct hist_entry *
-perf_session__find_hist_entry_by_name(struct perf_session *self,
-                                     struct hist_entry *he)
+perf_session__find_hist_entry(struct perf_session *self,
+                             struct hist_entry *he)
 {
        struct rb_node *n = self->hists.rb_node;

        while (n) {
                struct hist_entry *iter = rb_entry(n, struct hist_entry, rb_node);
-               int cmp = strcmp(he->map->dso->name, iter->map->dso->name);
+               int64_t cmp = hist_entry__cmp(he, iter);

-               if (cmp > 0)
+               if (cmp < 0)
                        n = n->rb_left;
-               else if (cmp < 0)
+               else if (cmp > 0)
                        n = n->rb_right;
-               else {
-                       cmp = strcmp(he->sym->name, iter->sym->name);
-                       if (cmp > 0)
-                               n = n->rb_left;
-                       else if (cmp < 0)
-                               n = n->rb_right;
-                       else
-                               return iter;
-               }
+               else
+                       return iter;
        }

        return NULL;
@@ -155,11 +144,9 @@ static void perf_session__match_hists(struct perf_session *old_session,
 {
        struct rb_node *nd;

-       perf_session__resort_by_name(old_session);
-
        for (nd = rb_first(&new_session->hists); nd; nd = rb_next(nd)) {
                struct hist_entry *pos = rb_entry(nd, struct hist_entry, rb_node);
-               pos->pair = perf_session__find_hist_entry_by_name(old_session, pos);
+               pos->pair = perf_session__find_hist_entry(old_session, pos);
        }
 }




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/