This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC 2/3] Perf test framework
- From: Doug Evans <dje at google dot com>
- To: Yao Qi <yao at codesourcery dot com>
- Cc: gdb-patches <gdb-patches at sourceware dot org>
- Date: Fri, 20 Sep 2013 09:51:04 -0700
- Subject: Re: [RFC 2/3] Perf test framework
- Authentication-results: sourceware.org; auth=none
- References: <520B7F70 dot 6070207 at codesourcery dot com> <1377663394-4975-1-git-send-email-yao at codesourcery dot com> <1377663394-4975-3-git-send-email-yao at codesourcery dot com> <21051 dot 19430 dot 251374 dot 942418 at ruffy dot mtv dot corp dot google dot com> <523C015E dot 30902 at codesourcery dot 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
]