Bug 138483 - Refactor the get-by-pname.js test
Summary: Refactor the get-by-pname.js test
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on:
Blocks: 138476
  Show dependency treegraph
 
Reported: 2014-11-06 15:43 PST by Mark Lam
Modified: 2014-11-06 17:14 PST (History)
0 users

See Also:


Attachments
the patch. (13.02 KB, patch)
2014-11-06 15:45 PST, Mark Lam
ggaren: review+
Details | Formatted Diff | Diff
follow up patch. (6.13 KB, patch)
2014-11-06 16:32 PST, Mark Lam
msaboff: review+
Details | Formatted Diff | Diff
follow up 2: give each test case its own unique test function (17.99 KB, patch)
2014-11-06 17:04 PST, Mark Lam
msaboff: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2014-11-06 15:43:25 PST
Refactored the test code to:
1. make it easier to add more test cases.
2. test each case against different tier JITs.
3. print only one set of results for each tier being tested.
Comment 1 Mark Lam 2014-11-06 15:45:54 PST
Created attachment 241140 [details]
the patch.
Comment 2 Geoffrey Garen 2014-11-06 15:50:27 PST
Comment on attachment 241140 [details]
the patch.

r=me

You should verify that this test runs in less than 200ms.
Comment 3 Mark Lam 2014-11-06 15:54:34 PST
Thanks for the review.  Landed in r175722: <http://trac.webkit.org/r175722>.
Comment 4 Michael Saboff 2014-11-06 15:57:37 PST
Comment on attachment 241140 [details]
the patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=241140&action=review

> LayoutTests/js/script-tests/get-by-pname.js:31
> +var testCases = [
> +    [ "foo(o)", "11" ],
> +    [ "foo(p)", "10" ],
> +    [ "foo(q)", "3467" ],
> +    [ "foo(r)", "113" ],
> +    [ "foo(s)", "182" ],
> +];
> +
> +function testExpr(index) {
> +    return testCases[index][0];
> +}
> +function testExpectedResult(index) {
> +    return testCases[index][1];
> +}

Instead of using indices, use named properties.
e.g.
var testCases = [
    { expr : "foo(o)", expected : "11" },
...

> LayoutTests/js/script-tests/get-by-pname.js:41
> +var tiers = [
> +    // name, iterations
> +    [ "cold", 0 ],
> +    [ "llint", 10 ],
> +    [ "baseline", 500 ],
> +    [ "dfg", 1000 ],
> +    // [ "ftl", 100000 ],
> +];

Same thing with named properties.
Comment 5 Michael Saboff 2014-11-06 16:15:35 PST
Comment on attachment 241140 [details]
the patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=241140&action=review

> LayoutTests/js/script-tests/get-by-pname.js:24
> +    [ "foo(o)", "11" ],
> +    [ "foo(p)", "10" ],
> +    [ "foo(q)", "3467" ],
> +    [ "foo(r)", "113" ],
> +    [ "foo(s)", "182" ],
> +];

Won't having a function name listed in multiple test cases cause it to tier up that much faster?  foo() is in the list 5 times, so this should be factored into the tiering.
Comment 6 Mark Lam 2014-11-06 16:32:29 PST
Created attachment 241143 [details]
follow up patch.
Comment 7 Michael Saboff 2014-11-06 16:46:29 PST
Comment on attachment 241143 [details]
follow up patch.

This looks fine.  There is still the issue that the tiering for each of the test functions will happen at different loop iterations due to functions appearing in the test list multiple times.
Comment 8 Mark Lam 2014-11-06 16:50:10 PST
Thanks.  Follow up 1 landed in r175729: <http://trac.webkit.org/r175729>.
Comment 9 Mark Lam 2014-11-06 17:04:44 PST
Created attachment 241149 [details]
follow up 2: give each test case its own unique test function
Comment 10 Michael Saboff 2014-11-06 17:09:30 PST
Comment on attachment 241149 [details]
follow up 2: give each test case its own unique test function

r+
Comment 11 Mark Lam 2014-11-06 17:14:59 PST
Thanks.  Follow up 2 landed in r175731: <http://trac.webkit.org/r175731>.