Bug 138483

Summary: Refactor the get-by-pname.js test
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 138476    
Attachments:
Description Flags
the patch.
ggaren: review+
follow up patch.
msaboff: review+
follow up 2: give each test case its own unique test function msaboff: review+

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>.