Bug 145638 - [ES6] Arrow function. Write tests for the controlFlowProfiler
Summary: [ES6] Arrow function. Write tests for the controlFlowProfiler
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 144955
Blocks: 140855
  Show dependency treegraph
 
Reported: 2015-06-03 23:44 PDT by GSkachkov
Modified: 2015-09-20 21:36 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.07 KB, patch)
2015-06-08 06:18 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (2.98 KB, patch)
2015-06-26 02:07 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (2.91 KB, patch)
2015-06-27 08:33 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (2.94 KB, patch)
2015-09-19 10:32 PDT, GSkachkov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description GSkachkov 2015-06-03 23:44:58 PDT
To be sure and prevent regressions, it's worth writing a few simple tests for the controlFlowProfiler.
Check path JavaScriptCore/tests/controlFlowProfiler/* there is a standardized way of doing this.
Comment 1 GSkachkov 2015-06-08 06:18:10 PDT
Created attachment 254489 [details]
Patch
Comment 2 Saam Barati 2015-06-08 13:14:28 PDT
This looks good to me. You should also have a test that
tests the assumption for range of the entire function itself.
The way this works for functions now is that the entire
function executing offset will begin from the "function"
key word and will end with the closing "}" of the function.
We do this by passing in different parts of the function that
represent the textual boundary of the function.

We should match this style with arrow function syntax so the beginning
should be the "(" in the parameter list, or the beginning letter
of the parameter in the single parameter case. For example, I'd make tests
like this:

```
function foo1(test) {
   var f1 = () => { "hello"; }
   if (test)
       f1();
}
foo1(false);
checkBasicBlock(foo1, '() =>', ShouldNotHaveExecuted);
checkBasicBlock(foo1, '; }', ShouldNotHaveExecuted);
foo1(true);
checkBasicBlock(foo1, '() =>', ShouldHaveExecuted);
checkBasicBlock(foo1, '; }', ShouldHaveExecuted);
// etc, maybe different offsets, too.

function foo2(test) {
   var f1 = (xyz) => { "hello" }
   if (test)
       f1();
}
foo1(false);
checkBasicBlock(foo1, '(xyz) =>', ShouldNotHaveExecuted);
checkBasicBlock(foo1, '; }', ShouldNotHaveExecuted);
foo1(true);
checkBasicBlock(foo1, '(xyz) =>', ShouldHaveExecuted);
checkBasicBlock(foo1, '; }', ShouldHaveExecuted);
// etc, maybe other tests with single parameter syntax, etc.

``` 

(Actually, looking through the tests for the control flow profiler, it doesn't look like there are other
tests that check function boundaries for regular functions. I should open a bug for that).
Comment 3 Saam Barati 2015-06-08 13:17:03 PDT
nction foo2(test) {
>    var f1 = (xyz) => { "hello" }
>    if (test)
>        f1();
> }
> foo1(false);
> checkBasicBlock(foo1, '(xyz) =>', ShouldNotHaveExecuted);
> checkBasicBlock(foo1, '; }', ShouldNotHaveExecuted);
> foo1(true);
> checkBasicBlock(foo1, '(xyz) =>', ShouldHaveExecuted);
> checkBasicBlock(foo1, '; }', ShouldHaveExecuted);
Mistake: these should all be "foo2".
Comment 4 Saam Barati 2015-06-08 13:18:03 PDT
Also, you can actually check the ranges out visually inside the Web Inspector
by enable the TypeProfiler/ControlFlowProfiler using the "T" button in the upper
right when looking at the source code.
Comment 5 GSkachkov 2015-06-26 00:22:16 PDT
(In reply to comment #4)
> Also, you can actually check the ranges out visually inside the Web Inspector
> by enable the TypeProfiler/ControlFlowProfiler using the "T" button in the
> upper
> right when looking at the source code.

OK. I'll upload new tests today
Comment 6 GSkachkov 2015-06-26 02:07:27 PDT
Created attachment 255624 [details]
Patch
Comment 7 GSkachkov 2015-06-26 12:33:54 PDT
As for now need to wait until https://bugs.webkit.org/show_bug.cgi?id=144956 will be fixed. After that can be checked
Comment 8 GSkachkov 2015-06-27 08:33:28 PDT
Created attachment 255700 [details]
Patch
Comment 9 Saam Barati 2015-06-27 12:24:11 PDT
Comment on attachment 255700 [details]
Patch

Looks good to me.
Comment 10 Joseph Pecoraro 2015-09-17 21:16:00 PDT
Is this stale?
Comment 11 Saam Barati 2015-09-18 18:00:07 PDT
Can you make sure this still passes and then
I can r+/cq+. We may have changed some code that
affects this. Better to be safe.
Comment 12 GSkachkov 2015-09-19 10:32:23 PDT
Created attachment 261578 [details]
Patch

Reupload
Comment 13 GSkachkov 2015-09-19 11:18:29 PDT
Comment on attachment 261578 [details]
Patch

Looks like tests pass without errors
Comment 14 Saam Barati 2015-09-20 20:49:45 PDT
Comment on attachment 261578 [details]
Patch

r=me
Comment 15 WebKit Commit Bot 2015-09-20 21:36:52 PDT
Comment on attachment 261578 [details]
Patch

Clearing flags on attachment: 261578

Committed r190042: <http://trac.webkit.org/changeset/190042>
Comment 16 WebKit Commit Bot 2015-09-20 21:36:57 PDT
All reviewed patches have been landed.  Closing bug.