> Em Tue, Dec 02, 2014 at 10:39:18AM -0500, kan.li...@intel.com escreveu: > > From: Kan Liang <kan.li...@intel.com> > > > > 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 <a...@redhat.com> > 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 <mi...@elte.hu> > > 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 <a...@redhat.com> 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/