Linux-perf-users Archive on lore.kernel.org
 help / color / Atom feed
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>,
	LKML <linux-kernel@vger.kernel.org>,
	Andi Kleen <ak@linux.intel.com>,
	Jin Yao <yao.jin@linux.intel.com>,
	John Garry <john.garry@huawei.com>, Paul Clarke <pc@us.ibm.com>,
	kajoljain <kjain@linux.ibm.com>,
	Stephane Eranian <eranian@google.com>,
	Sandeep Dasgupta <sdasgup@google.com>,
	linux-perf-users <linux-perf-users@vger.kernel.org>
Subject: Re: [PATCH v2 5/5] perf metric: Don't compute unused events.
Date: Fri, 20 Nov 2020 20:35:01 +0100
Message-ID: <20201120193501.GO1475102@krava> (raw)
In-Reply-To: <CAP-5=fWjzwDg7SbfBq+mHqT+zDEEpei8M5AF8+Bk8sU19Ta_hg@mail.gmail.com>

On Thu, Nov 19, 2020 at 01:37:15PM -0800, Ian Rogers wrote:
> On Thu, Nov 19, 2020 at 12:59 PM Jiri Olsa <jolsa@redhat.com> wrote:
> 
> > On Tue, Nov 17, 2020 at 09:03:35PM -0800, Ian Rogers wrote:
> >
> > SNIP
> >
> > > +                     ids__free($1.ids);
> > > +                     ids__free($3.ids);
> > > +             }
> > > +     } else {
> > > +             $$.val = NAN;
> > > +             $$.ids = ids__union($1.ids, $3.ids);
> > > +     }
> > > +}
> > > +| expr '*' expr
> > > +{
> > > +     if (!compute_ids || (isfinite($1.val) && isfinite($3.val))) {
> > > +             $$.val = $1.val * $3.val;
> > > +             $$.ids = NULL;
> > > +             if (compute_ids) {
> > > +                     ids__free($1.ids);
> > > +                     ids__free($3.ids);

thanks for the explanation below, really nice, one more question ;-)

why do we need to call ids__free in here, for compute_ids and constants
case in here it should be always NULL, no?

> > > +             }
> > > +     } else {
> > > +             $$.val = NAN;
> > > +             $$.ids = ids__union($1.ids, $3.ids);
> > > +     }
> > > +}
> > > +| expr '/' expr
> > > +{
> > > +     if (fpclassify($3.val) == FP_ZERO) {
> > > +             pr_debug("division by zero\n");
> > > +             YYABORT;
> > > +     } else if (!compute_ids || (isfinite($1.val) && isfinite($3.val)))
> > {
> > > +             $$.val = $1.val / $3.val;
> > > +             $$.ids = NULL;
> >
> > hum, I'm confused with this.. compute_ids with finite values?
> > why do we erase ids then? also val should be NAN then, no?
> > could you please put in some comment with reasoning?
> >
> 
> Each expr parsing step needs to create a val and an ids. If we're not
> computing ids then we know we don't need ids. If the values are both
> constants (aka finite), again we don't need ids as  we can just divide. It
> is invariant that if you have ids then the value is NAN which isn't finite.
> So when computing ids we may want to constant evaluate:
> 
> event1 if 0.5 > 1.0/3.0 else event2
> 
> which is quite hypothetical but the idea is to have a general approach for
> all the operators. The simple lattice is something like:
> 
> Constants: 0.0, 1.0, ....
>                     \.    |.     /
> Bottom:         NAN
> 
> Bottom means we really have a set of values and we don't know which it
> could be. So:
> 
> 3.0 if event1 > 10.0 else 4.0
> 
> has possible values of 3.0 or 4.0 which we could represent with the set
> {3.0, 4.0}, but because the lattice is simple we just say bottom, meaning
> the set of all values - which is conservatively correct as 3.0 and 4.0 are
> in the set of all values. It would be incorrect to say the value was 3.0.
> Even with a simple lattice we could represent that:
> 
> 3.0 if event1 > 10.0 else 3.0
> 
> evaluates to 3.0 and so there is no need to measure event1. Note, this
> peephole optimization isn't performed here, just the peephole optimization
> that if the condition is true or false we can remove events.
> 
> The code in general needs to handle the compute_ids case and the evaluation
> case. So what the code is trying to do is propagate constants with sets of
> ids in the compute_ids case, or just evaluate in the other case. NAN is
> used as a bottom just for simplicity and to avoid inventing a type lattice
> abstraction.

could you please put it in comment, will be helpful for future ;-)

thanks,
jirka


      parent reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-18  5:03 [PATCH v2 0/5] Don't compute events that won't be used in a metric Ian Rogers
2020-11-18  5:03 ` [PATCH v2 1/5] perf metric: Restructure struct expr_parse_ctx Ian Rogers
2020-11-18  5:03 ` [PATCH v2 2/5] perf metric: Use NAN for missing event IDs Ian Rogers
2020-11-19 19:47   ` Jiri Olsa
2020-11-18  5:03 ` [PATCH v2 3/5] perf metric: Rename expr__find_other Ian Rogers
2020-11-18  5:03 ` [PATCH v2 4/5] perf metric: Add utilities to work on ids map Ian Rogers
2020-11-18  5:03 ` [PATCH v2 5/5] perf metric: Don't compute unused events Ian Rogers
2020-11-19 20:59   ` Jiri Olsa
     [not found]     ` <CAP-5=fWjzwDg7SbfBq+mHqT+zDEEpei8M5AF8+Bk8sU19Ta_hg@mail.gmail.com>
2020-11-20 19:35       ` Jiri Olsa [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201120193501.GO1475102@krava \
    --to=jolsa@redhat.com \
    --cc=acme@kernel.org \
    --cc=ak@linux.intel.com \
    --cc=alexander.shishkin@linux.intel.com \
    --cc=eranian@google.com \
    --cc=irogers@google.com \
    --cc=john.garry@huawei.com \
    --cc=kjain@linux.ibm.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-perf-users@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mingo@redhat.com \
    --cc=namhyung@kernel.org \
    --cc=pc@us.ibm.com \
    --cc=peterz@infradead.org \
    --cc=sdasgup@google.com \
    --cc=yao.jin@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-perf-users Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-perf-users/0 linux-perf-users/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-perf-users linux-perf-users/ https://lore.kernel.org/linux-perf-users \
		linux-perf-users@vger.kernel.org
	public-inbox-index linux-perf-users

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-perf-users


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git