Re: [PATCH 2/3] Perf test framework

Doug Evans
2013-10-15T20:14:47Z

Yao Qi writes:
> On 10/09/2013 02:37 PM, Doug Evans wrote:
> > I'm not sure what your plans are, but another thing we need to measure
> > is memory usage.
>
> I plan to add memory measurement later. Current framework supports
> multiple measurements in a single test case.
>
> class SolibLoadUnload(perftest.SingleStatisticTestCase):
> def __init__(self, solib_number):
> # We want to measure time in this test.
> super (SolibLoadUnload, self).__init__ ([measure.Measurement.TIME],
> "solib")
>
>
> Once we want to measure memory usage, we can modify it like:
>
> class SolibLoadUnload(perftest.SingleStatisticTestCase):
> def __init__(self, solib_number):
> # We want to measure time in this test.
> super (SolibLoadUnload, self).__init__ ([measure.Measurement.TIME,
> measure.Measurement.MEMORY],
> "solib")

SingleStatisticTestCase seems out of place now.
MultiStatisticTestCase?
OTOH, if the set of statistics is passed as a list, do
we need to distinguish Single vs Multi here?
[We may need to distinguish them elsewhere still.]

> > There are other things too, but time (both cpu and wall) and memory
> > usage are a good start.
> > Most basic performance tests might want all three (e.g., why run a
> > test three times, it's all good data).
>
> All data needed by each measurement can be collected in one run.

Awesome.

> > How will MultiStatistic tests work?
>
> All our test cases are SingleStatistic because they are quite simple,
> so we don't think carefully how MultiStatistic tests work. I'd like to
> consider this when we really need a MultiStatistic test case.

We don't need to implement the Multi case in the first pass,
but we do need it today IMO.
In my perf testing/monitoring, anytime I've wanted any of cpu/wall time
and memory usage I've found it very useful to have all the others as well.
And since we're only going to run the tests once (instead of, e.g., once
for time and once for memory), I'd say let's just plan for collecting
all three anytime we want any one of them.
[Well, if someone *really* only wants to collect one statistic, ok,
but I wouldn't be surprised if I submit patches to add the others.]
Depending on the test case there are other statistics as well
that I've found useful, but they're more test-specific.

> >> >+
> >> >+class Measurement(object):
> >> >+ """A measurement for a certain aspect."""
> >> >+
> >> >+ # enum of each measurement.
> >> >+ TIME = 1
> >> >+
> >> >+ @classmethod
> >> >+ def create_instance(cls, id, result_factory):
> >> >+ """A factory method to create an instance of Measurement subclass
> >> >+ according to id."""
> >> >+ if id == Measurement.TIME:
> >> >+ return MeasurementTime(result_factory.create_result())
> >> >+ else:
> >> >+ raise RuntimeError("Unknown type of measurement.")
> > I don't know factories that well, but IWBN if every measurement kind
> > didn't need an entry here.
> >
>
> In each test case, the enum of Measurement are specified in a
> collection, like,
>
> [measure.Measurement.CPU_TIME, measure.Measurement.WALL_TIME,
> measure.Measurement.MEMORY]
>
> the static method create_instance creates the instance of Measurement
> sub-classes, which looks reasonable to me.
>
> Without this static method, each test case has to create a collection
> of Measurement instances for its own.

measure.Measurement.CPU_TIME is already a lot of characters.
If one replaced that with, say, a call to construct a measurement object
what is lost?
[That's an honest question ... I don't know the answer.]

Or, if using a factory is the way to go, can new measurements register
themselves with the factory, and then have them be requested by name
(as a string)?
[The problem I'm wondering about, and maybe it's not a problem, is
having to edit a central location every time a new measurement is added.]

> >> >+
> >> >+import testresult
> >> >+import reporter
> >> >+from measure import Measure
> >> >+
> >> >+class TestCase(object):
> > PerfTestCase?
> >
>
> I'd like to leave "TestCase" as-is because they have been in package
> perftest.

Ah.

> >> >+ """
> >> >+ Base class of all performance testing cases.
> >> >+
> >> >+ Each sub-class should override methods execute_test, in which
> >> >+ several GDB operations are performed and measured by attribute
> >> >+ measure. Sub-class can also override method warm_up optionally
> >> >+ if the test case needs warm up.
> >> >+
> >> >+ """
> >> >+
> >> >+ def __init__(self, name, measure):
> >> >+ """
> >> >+ Constructor of TestCase.
> >> >+
> >> >+ Construct an instance of TestCase with a name and a measure
> >> >+ which is to measure the test by several different measurements.
> >> >+
> > It's odd to have a blank line here. I couldn't find anything
> > definitive in pep257 though.
> >
> >> >+ """
> > IIUC, pep257 says have a blank line here.
> >
>
> I was confused by this sentence "Docstrings documenting functions or
> methods generally don't have this requirement, unless the function or
> method's body is written as a number of blank-line separated sections
> -- in this case, treat the docstring as another section, and precede it
> with a blank line".
>
> and follow the example in pep-0257
>
> def complex(real=0.0, imag=0.0):
> """Form a complex number.
>
> Keyword arguments:
> real -- the real part (default 0.0)
> imag -- the imaginary part (default 0.0)
>
> """
> if imag == 0.0 and real == 0.0: return complex_zero
> ...
>

That example is odd IMO.
[The stated reasons are so that emacs's fill-paragraph works,
but I'm not sure it's a convention one is expected to always follow.]

> >> >+ uplevel 2 $body
> >> >+ }
> >> >+
> >> >+ # The top-level interface to PerfTest.
> >> >+ # PREPARE is the tcl code to prepare for the performance testing,
> >> >+ # including generating and compiling source files, starting up GDB.
> >> >+ # RUN is the tcl code to drive GDB to do some operations.
> >> >+ proc assemble {prepare run} {
> > According to this comment "prepare" combines compiling source files
> > with starting up gdb.
> > Can we separate them?
>
> OK.
>
> > OTOH, there's the call to _setup_gdb after the "eval $prepare".
> > It's a bit confusing.
> >
>
> What is confusing? the name "_setup_gdb"? $prepare is to compile
> sources and start up gdb, and proc _setup_gdb is to set up perf test in
> a running gdb. How about rename "_setup_gdb" with "_setup" or
> "_setup_perftest"?

"setup" and "prepare" are essentially synonymous,
thus it's confusing to appear to be doing the same thing twice.
The names used need to better distinguish what each is doing.

_setup_perftest is a definite improvement.