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.
Created attachment 254489 [details] Patch
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).
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".
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.
(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
Created attachment 255624 [details] Patch
As for now need to wait until https://bugs.webkit.org/show_bug.cgi?id=144956 will be fixed. After that can be checked
Created attachment 255700 [details] Patch
Comment on attachment 255700 [details] Patch Looks good to me.
Is this stale?
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.
Created attachment 261578 [details] Patch Reupload
Comment on attachment 261578 [details] Patch Looks like tests pass without errors
Comment on attachment 261578 [details] Patch r=me
Comment on attachment 261578 [details] Patch Clearing flags on attachment: 261578 Committed r190042: <http://trac.webkit.org/changeset/190042>
All reviewed patches have been landed. Closing bug.