From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24072 invoked by alias); 20 Sep 2013 08:04:26 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 24059 invoked by uid 89); 20 Sep 2013 08:04:25 -0000 Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 20 Sep 2013 08:04:25 +0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.7 required=5.0 tests=AWL,BAYES_00,GARBLED_BODY,KHOP_THREADED,RDNS_NONE,SPF_HELO_FAIL autolearn=no version=3.3.2 X-HELO: relay1.mentorg.com Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1VMvhM-0000JK-6f from Yao_Qi@mentor.com ; Fri, 20 Sep 2013 01:04:20 -0700 Received: from SVR-ORW-FEM-02.mgc.mentorg.com ([147.34.96.206]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Fri, 20 Sep 2013 01:04:20 -0700 Received: from qiyao.dyndns.org (147.34.91.1) by svr-orw-fem-02.mgc.mentorg.com (147.34.96.168) with Microsoft SMTP Server id 14.2.247.3; Fri, 20 Sep 2013 01:04:18 -0700 Message-ID: <523C015E.30902@codesourcery.com> Date: Fri, 20 Sep 2013 08:04:00 -0000 From: Yao Qi User-Agent: Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/20130110 Thunderbird/17.0.2 MIME-Version: 1.0 To: Doug Evans CC: Subject: Re: [RFC 2/3] Perf test framework References: <520B7F70.6070207@codesourcery.com> <1377663394-4975-1-git-send-email-yao@codesourcery.com> <1377663394-4975-3-git-send-email-yao@codesourcery.com> <21051.19430.251374.942418@ruffy.mtv.corp.google.com> In-Reply-To: <21051.19430.251374.942418@ruffy.mtv.corp.google.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2013-09/txt/msg00728.txt.bz2 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. > > + > > + 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. > > > + > > + 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"? > > + > > +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"? -- Yao (齐尧)