Bug 145638

Summary: [ES6] Arrow function. Write tests for the controlFlowProfiler
Product: WebKit Reporter: GSkachkov <gskachkov>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, joepeck, mark.lam, saam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 144955    
Bug Blocks: 140855    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

GSkachkov
Reported 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.
Attachments
Patch (2.07 KB, patch)
2015-06-08 06:18 PDT, GSkachkov 		no flags
DetailsFormatted DiffDiff
Patch (2.98 KB, patch)
2015-06-26 02:07 PDT, GSkachkov 		no flags
DetailsFormatted DiffDiff
Patch (2.91 KB, patch)
2015-06-27 08:33 PDT, GSkachkov 		no flags
DetailsFormatted DiffDiff
Patch (2.94 KB, patch)
2015-09-19 10:32 PDT, GSkachkov 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
GSkachkov
Comment 1 2015-06-08 06:18:10 PDT
Created attachment 254489 [details] Patch
Saam Barati
Comment 2 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).
Saam Barati
Comment 3 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".
Saam Barati
Comment 4 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.
GSkachkov
Comment 5 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
GSkachkov
Comment 6 2015-06-26 02:07:27 PDT
Created attachment 255624 [details] Patch
GSkachkov
Comment 7 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
GSkachkov
Comment 8 2015-06-27 08:33:28 PDT
Created attachment 255700 [details] Patch
Saam Barati
Comment 9 2015-06-27 12:24:11 PDT
Comment on attachment 255700 [details] Patch Looks good to me.
Joseph Pecoraro
Comment 10 2015-09-17 21:16:00 PDT
Is this stale?
Saam Barati
Comment 11 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.
GSkachkov
Comment 12 2015-09-19 10:32:23 PDT
Created attachment 261578 [details] Patch Reupload
GSkachkov
Comment 13 2015-09-19 11:18:29 PDT
Comment on attachment 261578 [details] Patch Looks like tests pass without errors
Saam Barati
Comment 14 2015-09-20 20:49:45 PDT
Comment on attachment 261578 [details] Patch r=me
WebKit Commit Bot
Comment 15 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>
WebKit Commit Bot
Comment 16 2015-09-20 21:36:57 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.