Re: Telemetry: featurize tab_switching test for compressed swap performance (issue 606683005 by semenzato@chromium.org)

[aio...@chromium.org]

This approach is not lgtm.

The reason is that this CL relies heavily on adding more commandline
options. We
are in the process of reducing them and have a very high bar for accepting
new
ones. The reason for this is that benchmarks should be consistent across
runs.
Allowing something as drastic of a change as this to be specified at runtime
reduces the usability of the information we would get out of the benchmark.


https://codereview.chromium.org/606683005/diff/100001/tools/perf/measurements/tab_switching.py
File tools/perf/measurements/tab_switching.py (right):

https://codereview.chromium.org/606683005/diff/100001/tools/perf/measurements/tab_switching.py#newcode13
tools/perf/measurements/tab_switching.py:13: import optparse
argparse should be used instead of optparse.

https://codereview.chromium.org/606683005/diff/100001/tools/perf/measurements/tab_switching.py#newcode35
tools/perf/measurements/tab_switching.py:35: def AddCommandLineArgs(cls,
parser):
This shouldn't be specified via commandline arg.

https://codereview.chromium.org/606683005/diff/100001/tools/telemetry/telemetry/page/page_runner.py
File tools/telemetry/telemetry/page/page_runner.py (right):

https://codereview.chromium.org/606683005/diff/100001/tools/telemetry/telemetry/page/page_runner.py#newcode211
tools/telemetry/telemetry/page/page_runner.py:211:
group.add_option('--pageset-replicate', default=1, type='int',
On 2014/09/29 16:54:09, tonyg wrote:
> It's not clear from this description why this is different from
> --pageset-repeat. Perhaps we should just change the behavior of the
tab
> switching measurement given a pageset repeat? Or perhaps we should
just make a
> new page set for the case you have in mind?

It's also unclear to me why this is different.

https://codereview.chromium.org/606683005/diff/100001/tools/telemetry/telemetry/page/page_runner.py#newcode214
tools/telemetry/telemetry/page/page_runner.py:214:
group.add_option('--pageset-truncate', default=sys.maxint, type='int',
This logic shouldn't be here if it's needed and I'm not convinced it's
needed. Why can't you just create the page_set of the size you need?

https://codereview.chromium.org/606683005/

[aio...@chromium.org]

On 2014/09/29 17:19:48, Luigi Semenzato wrote:
> On 2014/09/29 16:54:09, tonyg wrote:

> > The way tab switching is implemented prior to this patch isn't ideal.
> This
> patch
> > adds more complexity on top of that system. I'd like to try to see if
> we can
> > come up with a way to make this measurement fit a little better and
> simpler
> > instead of more complex. Dave might have ideas along those lines too.
> >
> > Luigi, could you please explain your requirements so we can figure out
> the
> best
> > way to do this.

> Sure. I am happy to revisit this. I agree it's different enough from page
> cycling that it would benefit from some changes to that framework.

> Our requirements are:

> 1. open enough tabs to trigger various amounts of memory compression;
> 2. simulate the action of switching tabs cyclically (which is the worst
> case
> scenario with respect to paging)
> 3. decide when a tab switch has completed, as a trigger for starting the
> following tab switch

> We don't want to have to generate a different page set for each level of
memory
> pressure. The two options --pageset-replicate and --pageset-truncate
> give us
> the ability to adjust the pressure with a one-tab resolution. (We don't
> care
if
> some of the tabs are the same as long as there is a good mix.)

I'm curious how many levels of memory pressure you are interested in running
this on, and how disparate they are. Do you actually need a one-tab
resolution?

> --pageset-repeat is a page cycler option, and causes the pages to be
> reloaded.

> The reloading of the last page triggers the switching. But we want to go
> through multiple cycles of switching without reloading pages. This is
provided
> by --cycle-count (maybe it should be --tab-switch-cycle-count). This may
> be
due
> to the mismatched infrastructure.

> If you want I can attempt to write a tab switch infrastructure similar to
> the
> page cycler, say "tab switcher".

That would be a better approach.

https://codereview.chromium.org/606683005/diff/100001/tools/telemetry/telemetry/page/page_runner.py
> > File tools/telemetry/telemetry/page/page_runner.py (right):
> >
> >

https://codereview.chromium.org/606683005/diff/100001/tools/telemetry/telemetry/page/page_runner.py#newcode211
> > tools/telemetry/telemetry/page/page_runner.py:211:
> > group.add_option('--pageset-replicate', default=1, type='int',

> > It's not clear from this description why this is different from
> > --pageset-repeat. Perhaps we should just change the behavior of the tab
> > switching measurement given a pageset repeat? Or perhaps we should just
> make
a
> > new page set for the case you have in mind?

https://codereview.chromium.org/606683005/

[nedn...@google.com]

page_runner.py file no longer exists. You may want rethink of how this
feature
can be added under the new shape of telemetry.



https://codereview.chromium.org/606683005/

[seme...@chromium.org]

On 2015/04/03 18:16:39, aiolos wrote:
> This approach is not lgtm.

> The reason is that this CL relies heavily on adding more commandline
> options.
We
> are in the process of reducing them and have a very high bar for
> accepting new
> ones. The reason for this is that benchmarks should be consistent across
> runs.
> Allowing something as drastic of a change as this to be specified at
> runtime
> reduces the usability of the information we would get out of the
> benchmark.

We want to run these benchmarks on systems with different memory sizes, so,
since we want to stress swap, the load is going to have to be different on
each
platform. There are other factors too, for instance whether GPU buffers are
swappable or not (x86 vs. ARM). We're not going to compare platforms with
each
other.

https://codereview.chromium.org/606683005/diff/100001/tools/perf/measurements/tab_switching.py
> File tools/perf/measurements/tab_switching.py (right):


https://codereview.chromium.org/606683005/diff/100001/tools/perf/measurements/tab_switching.py#newcode13
> tools/perf/measurements/tab_switching.py:13: import optparse
> argparse should be used instead of optparse.


https://codereview.chromium.org/606683005/diff/100001/tools/perf/measurements/tab_switching.py#newcode35
> tools/perf/measurements/tab_switching.py:35: def AddCommandLineArgs(cls,
> parser):
> This shouldn't be specified via commandline arg.

https://codereview.chromium.org/606683005/diff/100001/tools/telemetry/telemetry/page/page_runner.py
> File tools/telemetry/telemetry/page/page_runner.py (right):


https://codereview.chromium.org/606683005/diff/100001/tools/telemetry/telemetry/page/page_runner.py#newcode211
> tools/telemetry/telemetry/page/page_runner.py:211:
> group.add_option('--pageset-replicate', default=1, type='int',

> It's also unclear to me why this is different.

--pageset-repeat specifies how many times a pageset is loaded sequentially
during page cycling. --pageset-replicate transforms a page set into a
large one
by replicating its

Yes, I can change the behavior of --pageset-repeat so that it produces the
desired behavior in the tab_switching test. It may be messy because right
now
it controls an outer loop which runs the rest of the experiment. But in our
case the outer loop should always be 1 because the iterations we're
interested
in are the tab switching iterations, not the loading + switching.

Maybe it's a side effect from trying to reuse too much code.

https://codereview.chromium.org/606683005/diff/100001/tools/telemetry/telemetry/page/page_runner.py#newcode214
> tools/telemetry/telemetry/page/page_runner.py:214:
> group.add_option('--pageset-truncate', default=sys.maxint, type='int',
> This logic shouldn't be here if it's needed and I'm not convinced it's
> needed.
> Why can't you just create the page_set of the size you need?

As I mentioned, it's not practical to create different page sets for all
combinations of memory sizes and architectures, not to mention other
factors.



https://codereview.chromium.org/606683005/

[seme...@chromium.org]

On 2015/04/03 18:23:36, aiolos wrote:
> On 2014/09/29 17:19:48, Luigi Semenzato wrote:

> > On 2014/09/29 16:54:09, tonyg wrote:

See earlier comment. One-tab resolution may be necessary on sets of tabs
with
large footprint.

> > --pageset-repeat is a page cycler option, and causes the pages to be
reloaded.

> > The reloading of the last page triggers the switching. But we want to
> go
> > through multiple cycles of switching without reloading pages. This is
> provided
> > by --cycle-count (maybe it should be --tab-switch-cycle-count). This
> may be
> due
> > to the mismatched infrastructure.
> >
> > If you want I can attempt to write a tab switch infrastructure similar
> to
the
> > page cycler, say "tab switcher".

> That would be a better approach.

OK thanks, I can do that, but first let's see what we're going to do about
the
extra command-line options.

> >
> >

https://codereview.chromium.org/606683005/diff/100001/tools/telemetry/telemetry/page/page_runner.py
> > > File tools/telemetry/telemetry/page/page_runner.py (right):
> > >
> > >
> >

https://codereview.chromium.org/606683005/diff/100001/tools/telemetry/telemetry/page/page_runner.py#newcode211
> > > tools/telemetry/telemetry/page/page_runner.py:211:
> > > group.add_option('--pageset-replicate', default=1, type='int',

> > > It's not clear from this description why this is different from
> > > --pageset-repeat. Perhaps we should just change the behavior of the
> tab
> > > switching measurement given a pageset repeat? Or perhaps we should
> just
make
> a
> > > new page set for the case you have in mind?

https://codereview.chromium.org/606683005/

[seme...@chromium.org]

On 2015/04/03 18:26:44, nednguyen wrote:
> page_runner.py file no longer exists. You may want rethink of how this
> feature
> can be added under the new shape of telemetry.

By all means, thanks.

https://codereview.chromium.org/606683005/

[nedn...@google.com]

https://codereview.chromium.org/606683005/diff/100001/tools/telemetry/telemetry/page/page_runner.py
> > File tools/telemetry/telemetry/page/page_runner.py (right):
> >
> >

https://codereview.chromium.org/606683005/diff/100001/tools/telemetry/telemetry/page/page_runner.py#newcode211
> > tools/telemetry/telemetry/page/page_runner.py:211:
> > group.add_option('--pageset-replicate', default=1, type='int',

> > It's also unclear to me why this is different.

> --pageset-repeat specifies how many times a pageset is loaded sequentially
> during page cycling. --pageset-replicate transforms a page set into a
> large
one
> by replicating its

> Yes, I can change the behavior of --pageset-repeat so that it produces the
> desired behavior in the tab_switching test. It may be messy because
> right now
> it controls an outer loop which runs the rest of the experiment. But in
> our
> case the outer loop should always be 1 because the iterations we're
> interested
> in are the tab switching iterations, not the loading + switching.

> Maybe it's a side effect from trying to reuse too much code.

> >
> >

https://codereview.chromium.org/606683005/diff/100001/tools/telemetry/telemetry/page/page_runner.py#newcode214
> > tools/telemetry/telemetry/page/page_runner.py:214:
> > group.add_option('--pageset-truncate', default=sys.maxint, type='int',
> > This logic shouldn't be here if it's needed and I'm not convinced it's
needed.
> > Why can't you just create the page_set of the size you need?

> As I mentioned, it's not practical to create different page sets for all
> combinations of memory sizes and architectures, not to mention other
> factors.

PageSet are also python, so I don't see how it's impractical to create a
page
set will duplicate pages. If there are some pattern of creating page sets
that
fit your testing needs, you can create s.t like a PageSetFactory to make it
convenient to create them.

The factory logic for creating page set shouldn't be a part of
telemetry/....
but just live in perf/page_sets/...

https://codereview.chromium.org/606683005/