Re: [RFC 2/3] Perf test framework

[Doug Evans <dje at google dot com>]

Hi.  Various comments inline.
[Others have commented as well, I'm leaving those alone where I don't
have anything to add.]

Yao Qi writes:
 > 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..e69de29
 > diff --git a/gdb/testsuite/gdb.perf/lib/perftest/config.py b/gdb/testsuite/gdb.perf/lib/perftest/config.py
 > new file mode 100644
 > index 0000000..db24b16
 > --- /dev/null
 > +++ b/gdb/testsuite/gdb.perf/lib/perftest/config.py
 > @@ -0,0 +1,40 @@
 > +# 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 ConfigParser
 > +import reporter
 > +
 > +class PerfTestConfig(object):
 > +    """
 > +    Create the right objects according to file perftest.ini.
 > +    """
 > +
 > +    def __init__(self):
 > +        self.config = ConfigParser.ConfigParser()
 > +        self.config.read("perftest.ini")
 > +
 > +    def get_reporter(self):
 > +        """Create an instance of class Reporter which is determined by
 > +        the option 'type' in section '[Reporter]'."""
 > +        if not self.config.has_section('Reporter'):
 > +            return reporter.TextReporter()
 > +        if not self.config.has_option('Reporter', 'type'):
 > +            return reporter.TextReporter()
 > +
 > +        name = self.config.get('Reporter', 'type')
 > +        cls = getattr(reporter, name)
 > +        return cls()
 > +
 > +perftestconfig = PerfTestConfig()

What do you see perftest.ini containing over time?
While the file format is pretty trivial, it is another file format.
Do we need it?

 > 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..b15fd39
 > --- /dev/null
 > +++ b/gdb/testsuite/gdb.perf/lib/perftest/perftest.py
 > @@ -0,0 +1,49 @@
 > +# 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 gdb
 > +import testresult
 > +from config import perftestconfig
 > +
 > +class TestCase(gdb.Function):
 > +    """Base class of all performance testing cases.  It registers a GDB
 > +    convenience function 'perftest'.  Invoke this convenience function
 > +    in GDB will call method 'invoke'."""

Coding conventions for doc strings are here:
http://www.python.org/dev/peps/pep-0257

 > +
 > +    def __init__(self, result):
 > +        # Each test case registers a convenience function 'perftest'.
 > +        super (TestCase, self).__init__ ("perftest")
 > +        self.result = result
 > +
 > +    def execute_test(self):
 > +        """Abstract method  to do the actual tests."""
 > +        raise RuntimeError("Abstract Method.")
 > +
 > +    def __report(self, reporter):
 > +        # Private method to report the testing result by 'reporter'.
 > +        self.result.report (reporter)

Private methods should be _foo.
-> _report?

ref: http://www.python.org/dev/peps/pep-0008/#method-names-and-instance-variables

 > +
 > +    def invoke(self):
 > +        """Call method 'execute_test' and '__report'."""

As I read this, this comment just says what this function is doing.
I'm guessing the point is to say that all such methods must, at the least,
do these two things.  This should be spelled out.
It would also be good to document that "invoke" is what GDB calls
to perform this function.

Also, I'm wondering why execute_test is public and
__report(-> _report) is private?

 > +
 > +        self.execute_test()
 > +        self.__report(perftestconfig.get_reporter())
 > +        return "Done"
 > +
 > +class SingleVariableTestCase(TestCase):
 > +    """Test case with a single variable."""

I think this needs more documentation.
What does "single variable" refer to?  A single statistic, like wall time?

 > +
 > +    def __init__(self, name):
 > +        super (SingleVariableTestCase, self).__init__ (testresult.SingleVariableTestResult (name))
 > 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..e27b2ae
 > --- /dev/null
 > +++ b/gdb/testsuite/gdb.perf/lib/perftest/reporter.py
 > @@ -0,0 +1,38 @@
 > +# 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, which is about reporting test results in
 > +    different formatss."""

See pep 257 for doc string conventions.

 > +
 > +    def report(self, arg1, arg2, arg3):
 > +        raise RuntimeError("Abstract Method.")
 > +
 > +    def end(self):
 > +        """Invoked when reporting is done.  Usually it can be overridden
 > +        to do some cleanups, such as closing file descriptors."""
 > +        raise RuntimeError("Abstract Method:end.")
 > +
 > +class TextReporter(Reporter):
 > +    """Report results in plain text 'perftest.log'."""
 > +
 > +    def __init__(self):
 > +        self.txt_log = open ("perftest.log", 'a+');

While I can appreciate the potential user friendliness of appending
to perftest.log, I'm not comfortable with it as a default given all
the ways I envision myself using this. At least not yet.
I'd rather have the default be to overwrite.
An option to specify which would be ok.

 > +
 > +    def report(self, arg1, arg2, arg3):
 > +        print >>self.txt_log, '%s %s %s' % (arg1, arg2, arg3)

It would be good to rename arg1,2,3 and/or document their intended contents.

 > +
 > +    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..9912326
 > --- /dev/null
 > +++ b/gdb/testsuite/gdb.perf/lib/perftest/testresult.py
 > @@ -0,0 +1,42 @@
 > +# 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 or save test results."""
 > +
 > +    def __init__(self, name):
 > +        self.name = name
 > +
 > +    def record (self, variable, result):
 > +        raise RuntimeError("Abstract Method:record.")
 > +
 > +    def report (self, reporter):
 > +        """Report the test results by reporter."""
 > +        raise RuntimeError("Abstract Method:report.")
 > +
 > +class SingleVariableTestResult(TestResult):
 > +    """Test results for the test case with a single variable. """
 > +
 > +    def __init__(self, name):
 > +        super (SingleVariableTestResult, self).__init__ (name)
 > +        self.results = dict ()
 > +
 > +    def record(self, variable, result):
 > +        self.results[variable] = result

As things read (to me anyway), the class only handles a single variable,
but the "record" method makes the variable a parameter.
There's a disconnect here.

Maybe the "variable" parameter to "record" is misnamed.
E.g., if testing the wall time of performing something over a range of values,
e.g., 1 solib, 8, solibs, 256 solibs, "variable" would be 1,8,256?
If that's the case, please rename "variable".
I realize it's what is being varied run after run, but
it just doesn't read well.

There are two "variables" (so to speak) here:
1) What one is changing run after run.  E.g. # solibs
2) What one is measuring.  E.g. wall time, cpu time, memory used.

The name "variable" feels too ambiguous.
OTOH, if the performance testing world has a well established convention
for what the word "variable" means, maybe I could live with it. :-)

 > +
 > +    def report(self, reporter):
 > +        for key in sorted(self.results.iterkeys()):
 > +            reporter.report (self.name, key, self.results[key])
 > +        reporter.end ()

IIUC, calling end() here closes the file.
But this function didn't open the file.
It would be cleaner to either open+close the file here or do neither.