From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1106 invoked by alias); 20 Sep 2013 16:51:07 -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 1092 invoked by uid 89); 20 Sep 2013 16:51:07 -0000 Received: from mail-ie0-f175.google.com (HELO mail-ie0-f175.google.com) (209.85.223.175) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Fri, 20 Sep 2013 16:51:07 +0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.2 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,NO_RELAYS autolearn=ham version=3.3.2 X-HELO: mail-ie0-f175.google.com Received: by mail-ie0-f175.google.com with SMTP id e14so1315895iej.6 for ; Fri, 20 Sep 2013 09:51:04 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=1BM1flqedRnyN+XN3x7H6JiVwtuSdvbF9L3FkvNAAfo=; b=ecfDzW4hzRpOEkiuYb7FjS295wB+Qv6bvWFeKLBNzmtB5rEqHboohXAzrKmq2kWZdD nQ8jNUlojfwfFqi1M2NUrWuRs2nWPUE01C81JS1S7qBOGw5fAB+q3xVQU1UpSlNc81Mx SryF1lVEArg7TmFKtOcqKjnIlo2MCsNajG8weMksKLKcGl3XbqaSuJEHtZrYLNfjF3Hx oxYj6tKhw866o5zfUYJrKF38P6n0cFID9e8wBmXjOuxOy/+3jMdffIryu/yFIO5LtzDY nrJI61AMAUMfjo3IstPYunjExi6sjDdVxwAsy76VYnNTIuQDi5XXTWLLtPzwxqQOJCbB DKAg== X-Gm-Message-State: ALoCoQmcYO2cmRxjOlNdbkW38uQ/89j/D/vI+hbI++Zt8av97FTPOO22SDzc0WmdM3nBBuLjAxQt0fahstmtIqFh/iAesatT2fmOw1Z2GxOzYlQW3jWEsE1VbF71eLEzd0Y550slbCWbsAc0Irv6qS+Dem5/3AwrQYOQxnxb8CL03lnkQDRtw1NMPTcgUFtrr9Bkmn6ZMbnPyH/r4VrMv2xkTBVu28MWDw== MIME-Version: 1.0 X-Received: by 10.50.7.101 with SMTP id i5mr3267420iga.48.1379695864515; Fri, 20 Sep 2013 09:51:04 -0700 (PDT) Received: by 10.64.31.100 with HTTP; Fri, 20 Sep 2013 09:51:04 -0700 (PDT) In-Reply-To: <523C015E.30902@codesourcery.com> 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> <523C015E.30902@codesourcery.com> Date: Fri, 20 Sep 2013 16:51:00 -0000 Message-ID: Subject: Re: [RFC 2/3] Perf test framework From: Doug Evans To: Yao Qi Cc: gdb-patches Content-Type: text/plain; charset=ISO-8859-1 X-IsSubscribed: yes X-SW-Source: 2013-09/txt/msg00770.txt.bz2 On Fri, Sep 20, 2013 at 1:03 AM, Yao Qi 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 ]