Re: [PATCH 2/3] Perf test framework

Doug Evans
2013-10-09T06:37:40Z

On Wed, Sep 25, 2013 at 7:26 AM, Yao Qi <yao@codesourcery.com> wrote:
> This patch adds a basic framework to do performance testing for GDB.
> perftest.py is about the test case, testresult.py is about test
> results, and how are they saved. reporter.py is about how results
> are reported (in what format). measure.py is about measuring the
> execution of tests by a collection of measurements.

Hi.
Some comments inline.

> In V2, there are several changes to address Doug and Sanimir's
> comments.
>
> - Add copyright header and docstring in perftest/__init__.py
> - Remove config.py.
> - Fix docstring format.
> - Rename classes "SingleVariable" to "SingleStatistic".
> - Don't extend gdb.Function in class TestCase. Add a new method run
> to run the test case so that we can pass parameters to test.
> - Allow to customize whether to warm up and to append test log.
> - Move time measurement into test harness. Add a new class
> Measurement for a specific measurement and a new class Measure to
> measure them for a given test case.

I'm not sure what your plans are, but another thing we need to measure
is memory usage.
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).
How will MultiStatistic tests work?

> - A new class ResultFactory to create instances of TestResult.
> - New file lib/perftest.exp, which is to do some preparations and
> cleanups to simplify each *.exp file.
> - Skip compilation step if GDB_PERFORMANCE_SKIP_COMPILE exists.

For my uses, I need to be able to do the compilation and skip running
the tests (the tests will be run in a separate step).
Just a suggestion, but how about GDB_PERFORMANCE=yes|compile|run
[or whatever spellings work - just trying to reduce the number of variables]

> gdb/testsuite/
>
> * lib/perftest.exp: New.
> * gdb.perf/lib/perftest/__init__.py: New.
> * gdb.perf/lib/perftest/measure.py: New.
> * gdb.perf/lib/perftest/perftest.py: New.
> * gdb.perf/lib/perftest/reporter.py: New.
> * gdb.perf/lib/perftest/testresult.py: New.
> ---
> gdb/testsuite/gdb.perf/lib/perftest/__init__.py | 17 +++
> gdb/testsuite/gdb.perf/lib/perftest/measure.py | 113 +++++++++++++++++++
> gdb/testsuite/gdb.perf/lib/perftest/perftest.py | 71 ++++++++++++
> gdb/testsuite/gdb.perf/lib/perftest/reporter.py | 68 ++++++++++++
> gdb/testsuite/gdb.perf/lib/perftest/testresult.py | 59 ++++++++++
> gdb/testsuite/lib/perftest.exp | 121 +++++++++++++++++++++
> 6 files changed, 449 insertions(+), 0 deletions(-)
> create mode 100644 gdb/testsuite/gdb.perf/lib/perftest/__init__.py
> create mode 100644 gdb/testsuite/gdb.perf/lib/perftest/measure.py
> create mode 100644 gdb/testsuite/gdb.perf/lib/perftest/perftest.py
> create mode 100644 gdb/testsuite/gdb.perf/lib/perftest/reporter.py
> create mode 100644 gdb/testsuite/gdb.perf/lib/perftest/testresult.py
> create mode 100644 gdb/testsuite/lib/perftest.exp
>
> diff --git a/gdb/testsuite/gdb.perf/lib/perftest/__init__.py b/gdb/testsuite/gdb.perf/lib/perftest/__init__.py
> new file mode 100644
> index 0000000..7739a3e
> --- /dev/null
> +++ b/gdb/testsuite/gdb.perf/lib/perftest/__init__.py
> @@ -0,0 +1,17 @@
> +# Copyright (C) 2013 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +"""
> +GDB performance testing framework.
> +"""
> diff --git a/gdb/testsuite/gdb.perf/lib/perftest/measure.py b/gdb/testsuite/gdb.perf/lib/perftest/measure.py
> new file mode 100644
> index 0000000..8bede59
> --- /dev/null
> +++ b/gdb/testsuite/gdb.perf/lib/perftest/measure.py
> @@ -0,0 +1,113 @@
> +# Copyright (C) 2013 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +
> +import time
> +
> +class Measure(object):
> + """
> + A class that measure and collect the interesting data for a given
> + testcase.

style nit: the first line of a doc string is on one line.
ref: http://www.python.org/dev/peps/pep-0257/

> +
> + An instance of Measure has a collection of measurements, and each
> + of them is to measure a given aspect, such as time and memory.
> +
> + """
> +
> + def __init__(self, measurements, result_factory):
> + """Constructor of measure.
> +
> + measurements is a collection of 'enum's, which are about the
> + id of measurements. A collection of measurements are created
> + according to the ids in argument measurements.
> +
> + """
> + self.ms = []
> + for m in measurements:
> + self.ms.append(Measurement.create_instance(m, result_factory))
> +
> + def measure(self, func, id):
> + """Measure the operations done by func with a collection of measurements."""
> + for m in self.ms:
> + m.start(id)
> +
> + func()
> +
> + for m in self.ms:
> + m.stop(id)
> +
> + def report(self, reporter, name):
> + """Report the measured results."""
> + for m in self.ms:
> + m.report(reporter, name)
> +
> +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.

> +
> + def __init__(self, result):
> + """Constructor of Measurement.
> +
> + Attribute result is the TestResult associated with measurement.
> +
> + """
> + self.result = result
> +
> + def start(self, id):
> + """Abstract method to start the measurement."""
> + raise NotImplementedError("Abstract Method:start")
> +
> + def stop(self, id):
> + """Abstract method to stop the measurement.
> +
> + When the measurement is stopped, we've got something, and
> + record them in result.
> +
> + """
> + raise NotImplementedError("Abstract Method:stop.")
> +
> + def decorate_name(self, name):
> + """Decorate the name when report."""
> + return name
> +
> + def report(self, reporter, name):
> + """Report the measured data by argument reporter."""
> + self.result.report(reporter, self.decorate_name(name))
> +
> +class MeasurementTime(Measurement):

"Time" is ambiguous. wall time or cpu time?
I'd rename this to make it explicit what kind is being measured (in
the class name and its output).

> + """Measurement on time."""
> + def __init__(self, result):
> + super(MeasurementTime, self).__init__(result)
> + self.start_time = 0
> +
> + def start(self, id):
> + self.start_time = time.clock()
> +
> + def stop(self, id):
> + elapsed_time = time.clock() - self.start_time
> + self.result.record (id, elapsed_time)
> +
> + def decorate_name(self, name):
> + return name + " time"
> diff --git a/gdb/testsuite/gdb.perf/lib/perftest/perftest.py b/gdb/testsuite/gdb.perf/lib/perftest/perftest.py
> new file mode 100644
> index 0000000..bd81cf0
> --- /dev/null
> +++ b/gdb/testsuite/gdb.perf/lib/perftest/perftest.py
> @@ -0,0 +1,71 @@
> +# Copyright (C) 2013 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +
> +import testresult
> +import reporter
> +from measure import Measure
> +
> +class TestCase(object):

PerfTestCase?

> + """
> + 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.

> + self.name = name
> + self.measure = measure
> +
> + def execute_test(self):
> + """Abstract method to do the actual tests."""
> + raise NotImplementedError("Abstract Method.")
> +
> + def warm_up(self):
> + """Do some operations to warm up the environment."""
> +
> + def run(self, warm_up=True, append=True):
> + """
> + Run this test case.
> +
> + It is a template method to invoke method warm_up,
> + execute_test, and finally report the measured results.
> + If parameter warm_up is True, run method warm_up. If parameter
> + append is True, the test result will be appended instead of
> + overwritten.
> +
> + """
> + if warm_up:
> + self.warm_up()
> +
> + self.execute_test()
> + self.measure.report(reporter.TextReporter(append), self.name)
> +
> +class SingleStatisticTestCase(TestCase):
> + """Test case with a single statistic."""
> +
> + def __init__(self, measures, name):
> + super (SingleStatisticTestCase, self).__init__ (name, Measure(measures,
> + testresult.SingleStatisticResultFactory()))
> diff --git a/gdb/testsuite/gdb.perf/lib/perftest/reporter.py b/gdb/testsuite/gdb.perf/lib/perftest/reporter.py
> new file mode 100644
> index 0000000..7da86cf
> --- /dev/null
> +++ b/gdb/testsuite/gdb.perf/lib/perftest/reporter.py
> @@ -0,0 +1,68 @@
> +# Copyright (C) 2013 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +
> +class Reporter(object):
> + """
> + Base class of reporter to report test results in a certain format.
> +
> + Subclass, which is specific to a report format, should overwrite
> + methods report, start and end.
> +
> + """
> +
> + def __init__(self, append):
> + """Constructor of Reporter.
> +
> + attribute append is used to determine whether to append or
> + overwrite log file.
> +
> + """
> + self.append = append
> +
> + def report(self, *args):
> + raise NotImplementedError("Abstract Method:report.")
> +
> + def start(self):
> + """Invoked when reporting is started."""
> + raise NotImplementedError("Abstract Method:start.")
> +
> + def end(self):
> + """Invoked when reporting is done.
> +
> + It must be overridden to do some cleanups, such as closing file
> + descriptors.
> +
> + """
> + raise NotImplementedError("Abstract Method:end.")
> +
> +class TextReporter(Reporter):
> + """Report results in a plain text file 'perftest.log'."""
> +
> + def __init__(self, append):
> + super (TextReporter, self).__init__(Reporter(append))
> + self.txt_log = None
> +
> + def report(self, *args):
> + self.txt_log.write(' '.join(str(arg) for arg in args))
> + self.txt_log.write('\n')
> +
> + def start(self):
> + if self.append:
> + self.txt_log = open ("perftest.log", 'a+');
> + else:
> + self.txt_log = open ("perftest.log", 'w');
> +
> + def end(self):
> + self.txt_log.close ()
> diff --git a/gdb/testsuite/gdb.perf/lib/perftest/testresult.py b/gdb/testsuite/gdb.perf/lib/perftest/testresult.py
> new file mode 100644
> index 0000000..51a066f
> --- /dev/null
> +++ b/gdb/testsuite/gdb.perf/lib/perftest/testresult.py
> @@ -0,0 +1,59 @@
> +# Copyright (C) 2013 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +
> +class TestResult(object):
> + """
> + Base class to record and report test results.
> +
> + Method record is to record the results of test case, and report
> + method is to report the recorded results by a given reporter.
> +
> + """
> +
> + def record(self, parameter, result):
> + raise NotImplementedError("Abstract Method:record.")
> +
> + def report(self, reporter, name):
> + """Report the test results by reporter."""
> + raise NotImplementedError("Abstract Method:report.")
> +
> +class SingleStatisticTestResult(TestResult):
> + """Test results for the test case with a single statistic."""
> +
> + def __init__(self):
> + super (SingleStatisticTestResult, self).__init__ ()
> + self.results = dict ()
> +
> + def record(self, parameter, result):
> + self.results[parameter] = result
> +
> + def report(self, reporter, name):
> + reporter.start()
> + for key in sorted(self.results.iterkeys()):
> + reporter.report(name, key, self.results[key])
> + reporter.end()
> +
> +class ResultFactory(object):
> + """A factory to create an instance of TestResult."""
> +
> + def create_result(self):
> + """Create an instance of TestResult."""
> + raise NotImplementedError("Abstract Method:create_result.")
> +
> +class SingleStatisticResultFactory(ResultFactory):
> + """A factory to create an instance of SingleStatisticTestResult."""
> +
> + def create_result(self):
> + return SingleStatisticTestResult()
> diff --git a/gdb/testsuite/lib/perftest.exp b/gdb/testsuite/lib/perftest.exp
> new file mode 100644
> index 0000000..3c04989
> --- /dev/null
> +++ b/gdb/testsuite/lib/perftest.exp
> @@ -0,0 +1,121 @@
> +# Copyright (C) 2013 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +
> +namespace eval PerfTest {
> + # The name of python file on build.
> + variable remote_python_file
> +
> + # The source files are compiled successfully or not.
> + variable compiled_ok
> +
> + # A private method to set up GDB for performance testing.
> + proc _setup_gdb {} {
> + variable remote_python_file
> + global srcdir subdir testfile
> +
> + set remote_python_file [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
> +
> + # Set sys.path for module perftest.
> + gdb_test_no_output "python import os, sys"
> + gdb_test_no_output "python sys.path.insert\(0, os.path.abspath\(\"${srcdir}/${subdir}/lib\"\)\)"
> + gdb_test_no_output "python exec (open ('${remote_python_file}').read ())"
> + }
> +
> + # A private method to do some cleanups when performance test is
> + # finished.
> + proc _teardown {} {
> + variable remote_python_file
> +
> + remote_file host delete $remote_python_file
> + }
> +
> + # Compile source files of test case. BODY is the tcl code to do
> + # actual compilation and it should invoke 'PerfTest::compiled' if
> + # compilation is successful.
> + proc compile {body} {
> + global GDB_PERFORMANCE_SKIP_COMPILE
> +
> + if ![info exists GDB_PERFORMANCE_SKIP_COMPILE] {
> + variable compiled_ok
> +
> + set compiled_ok 0
> + uplevel 2 $body
> +
> + if {!$compiled_ok} {
> + untested "Could not compile source files."
> + return -1
> + }
> + }
> + }
> +
> + # Mark the compilation is finished successfully.
> + proc compiled {} {
> + variable compiled_ok
> +
> + set compiled_ok 1
> + }
> +
> + # Start GDB.
> + proc setup_gdb {body} {
> + variable compiled_ok
> +
> + if {!$compiled_ok} {
> + return
> + }
> +
> + uplevel 2 $body
> + }
> +
> + # Run the performance test.
> + proc run {body} {
> + global timeout
> +
> + set timeout 3000

Probably want to parameterize the timeout.

> + 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?
OTOH, there's the call to _setup_gdb after the "eval $prepare".
It's a bit confusing.

> + variable compiled_ok
> +
> + set compiled_ok 1
> + eval $prepare
> +
> + if {!$compiled_ok} {
> + return
> + }
> +
> + _setup_gdb
> +
> + eval $run
> +
> + _teardown
> + }
> +}
> +
> +# Return true if performance tests are skipped.
> +
> +proc skip_perf_tests { } {
> + global GDB_PERFORMANCE
> +
> + if [info exists GDB_PERFORMANCE] {
> + return 0
> + }
> +
> + return 1
> +}
> --
> 1.7.7.6
>