Re: [RFC 2/3] Perf test framework

[Doug Evans <dje@google.com>]

On Fri, Sep 20, 2013 at 1:03 AM, Yao Qi <yao@codesourcery.com> wrote:
> Doug, thanks for the review.
>
>
> On 09/20/2013 03:09 AM, Doug Evans wrote:
>>
>>   > +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?
>
>
> I was wondering that we can support json format, so I create class
> PerfTestConfig and perftest.ini is used to determine which format to
> be used.  I agree that we can remove PerfTestConfig since we
> only support only one format (plain text) nowadays.

Hi.  I wasn't suggesting removing support for more reporting formats.
We'll be adding our own. :-)

I'm just wondering, if all that pertest.ini will contain is the report
format, do we need it?
Or, can we specify the format (and whatever else is desired/needed)
via some other means?
How will the user specify the desired report format?

>>   > +
>>   > +    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?
>
>
> _report is private to be used only in class TestCase.  I double checked that
> private method can be overridden in python, so execute_test can be private
> too.  I'll do it in V2.

Yeah, "private" is a convention expressed by coding style, not an enforced rule.

Another thought I had was: what if a test tried to have two perf tests?
As things stand (and IIUC!), the user is intended to instantiate the
subclass of TestCase.
But the gdb.Function name is not a parameter, it's always "perftest".
It's reasonable to only require one such test
(and thus having two gdb.Functions with name "perftest" is "pilot error"),
but we should detect and report it somehow.
[It may still work, depending on the ordering of how the test does
things, but I wouldn't want to depend on it.]
OTOH, what's the cost of relaxing this restriction and making the
gdb.Function name a parameter?
[it could have a default value so the common case is unchanged]
Could be missing something of course. :-)

>>   > +
>>   > +        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?
>>
>
> Yes, like wall time.  How about "SingleStatisticTestCase" or
> "SingleParameterTestCase"?

"Statistic" is nice and clear.

>>   > +
>>   > +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?
>
>
> Yes, for solib test, "variable" is the number of shared libraries, and
> "result" is the time usage for loading or unloading such number of
> shared libraries.
>
>
>> 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.:-)
>>
>
> How about renaming "variable" by "parameter"?

Assuming we're talking about the "variable" argument to the "record" method,
yeah, "parameter" sounds ok to me.  "test_parameter" may be even better.

[
Though, for grin's sake, one can then have a discussion about the
difference between function arguments and function parameters, and
whether someone will be confused by the fact that we now have a
parameter named "parameter".  1/2 :-)
ref: http://stackoverflow.com/questions/156767/whats-the-difference-between-an-argument-and-a-parameter
]