Adjust the ranges of basic block statements in JSC's control flow profiler to be mutually exclusive

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

Specifically:
1. While loops, for loops, and if statements with block statements as their m_statement member variable.

We want basic block boundaries for these conditional block statements to start one character after the closing brace and end one character before the closing brace.
We're doing this to tune the UI that will be displayed inside the Web Inspector.

[Saam Barati]

Created attachment 245791 [details]
Patch

[Mark Lam]

Comment on attachment 245791 [details]
Patch

LGTM but would be better with regression tests so that we don't unknowingly mess this up in the future.

[Saam Barati]

Created attachment 246155 [details]
patch

Now with regression tests.

[Saam Barati]

I'm going to do a more thorough overhaul of the text ranges for the control flow profiler. There's a flaw in the profile now that one block's end offset may be another block's start offset. We should really make all basic block ranges mutually exclusive. 

This will make reasoning about the text offset locations more sound.

How it will be implemented: place the text offset that is an argument to BytecodeGenerator::emitProfileControlFlow at the *start* of a textual basic block. I reason about the correctness of this inductively: 

For a function w/ 1 block, we place the offset at the start of the function. For n blocks, we assume all n-1 were correct, and for every statement that makes up an nth block, make sure that the offset is at the start of that block.

This isn't a huge change, most things are already pretty close to correct.

[Saam Barati]

Created attachment 246167 [details]
patch

[Mark Lam]

Comment on attachment 246167 [details]
patch

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

> Source/JavaScriptCore/ChangeLog:16
> +

I think you should also mention that you adjusted the profiler control flow markers to also not start on open braces in the source code.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1900
> +        generator.emitProfileControlFlow(m_ifBlock->endOffset() + (!m_ifBlock->isBlock() ? 1 : 0));

nit: I’d prefer you use don’t negate the isBlock() condition.  Instead, just swap the adjustments to be “... ? 0 : 1”.  I think that better highlights the symmetry at the closing brace.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1905
> +    generator.emitProfileControlFlow(m_elseBlock ? m_elseBlock->endOffset() + 1 : m_ifBlock->endOffset() + (!m_ifBlock->isBlock() ? 1 : 0));

Ditto.  Don’t negate the isBlock() condition.

Right now, you would have:
if (x) { <marker> a; <marker> } ...
if (x) { <marker> a; <marker> } else { b; } <marker> ...

I would think to be consistent, it should be:
if (x) { <marker> a; <marker> } ...
if (x) { <marker> a; <marker> } else { b; <marker> } ...

or:
if (x) { <marker> a; } <marker> ...
if (x) { <marker> a; } <marker> else { b; } <marker> ...

Is there a good reason for the different treatment of the close brace for the if block and the else block?

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1951
> +    generator.emitProfileControlFlow(endOffset() + (!m_statement->isBlock() ? 1 : 0));

Ditto.  Don’t negate the isBlock() condition.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1986
> +    generator.emitProfileControlFlow(endOffset() + (!m_statement->isBlock() ? 1 : 0));

Ditto.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2090
> +    int profilerEndOffset = m_statement->endOffset() + (!m_statement->isBlock() ? 1 : 0);

Ditto.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2264
> +    generator.emitProfileControlFlow(m_statement->endOffset() + (!m_statement->isBlock() ? 1 : 0));

Ditto.

> Source/JavaScriptCore/tests/controlFlowProfiler/brace-location.js:8
> +function testIf(x) {
> +    if (x < 10) { foo(); } else if (x < 20) { bar(); } else { baz() }
> +}

For each of these test cases, can you also add cases without braces?  e.g.
    if (x < 10) foo(); else if (x < 20) bar(); else baz()

We should test that code path as well.

> Source/JavaScriptCore/tests/controlFlowProfiler/brace-location.js:29
> +assert(hasBasicBlockExecuted(testIf, "{ foo"), "should have executed.");

why “{ foo”?  Why not "if (x < 10) {“?

> Source/JavaScriptCore/tests/controlFlowProfiler/brace-location.js:34
> +testIf(21);

Can you create a testIf2() and run this test to ensure that the “{ foo” and “foo” parts are also not run?  Basically, I see that your test cases are not comprehensive.  Do you have a good reason for leaving those cases out?

Ditto for some of the test cases below.

> Source/JavaScriptCore/tests/controlFlowProfiler/brace-location.js:45
> +assert(hasBasicBlockExecuted(testForRegular, "{ bar"), "should have executed."); 

There is no “{ bar” in testForRegular.  So, how can this be executed?  I know this is a typo, and I also built and ran your patch + test, and found this test case to pass which makes me suspicious of the test mechanism.  Can you take a second look to see why it is passing when it shouldn’t?

[Saam Barati]

Comment on attachment 246167 [details]
patch

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

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1900
>> +        generator.emitProfileControlFlow(m_ifBlock->endOffset() + (!m_ifBlock->isBlock() ? 1 : 0));
> 
> nit: I’d prefer you use don’t negate the isBlock() condition.  Instead, just swap the adjustments to be “... ? 0 : 1”.  I think that better highlights the symmetry at the closing brace.

Will change this and the others below.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1905
>> +    generator.emitProfileControlFlow(m_elseBlock ? m_elseBlock->endOffset() + 1 : m_ifBlock->endOffset() + (!m_ifBlock->isBlock() ? 1 : 0));
> 
> Ditto.  Don’t negate the isBlock() condition.
> 
> Right now, you would have:
> if (x) { <marker> a; <marker> } ...
> if (x) { <marker> a; <marker> } else { b; } <marker> ...
> 
> I would think to be consistent, it should be:
> if (x) { <marker> a; <marker> } ...
> if (x) { <marker> a; <marker> } else { b; <marker> } ...
> 
> or:
> if (x) { <marker> a; } <marker> ...
> if (x) { <marker> a; } <marker> else { b; } <marker> ...
> 
> Is there a good reason for the different treatment of the close brace for the if block and the else block?

Yeah, there is:
Because the it's comprised like this: "<marker> } else ...", the marker will always have the "else" text offsets as being part of a new basic block.
If that else statement never executes, then we ensure that "else { ... }" is marked entirely as not executed. 

If we did it this way: "else { b; <marker> }", if the else clause doesn't execute, then the "else b;" will be shown as not executing, but the last brace will be shown as executing, and this is just confusing to not have the braces match each other.

>> Source/JavaScriptCore/tests/controlFlowProfiler/brace-location.js:8
>> +}
> 
> For each of these test cases, can you also add cases without braces?  e.g.
>     if (x < 10) foo(); else if (x < 20) bar(); else baz()
> 
> We should test that code path as well.

I will add this, but I think it should be added to the controlFlowProfiler/if-statement.js test.

>> Source/JavaScriptCore/tests/controlFlowProfiler/brace-location.js:29
>> +assert(hasBasicBlockExecuted(testIf, "{ foo"), "should have executed.");
> 
> why “{ foo”?  Why not "if (x < 10) {“?

Because the offset that is tested against for membership of a particular basic block is the first index of the matched substring. 
I'm just making sure that the "{" is part of the "if (x < 10) {" basic block, and not the " foo();" basic block. 
This will catch future off by one errors.

>> Source/JavaScriptCore/tests/controlFlowProfiler/brace-location.js:34
>> +testIf(21);
> 
> Can you create a testIf2() and run this test to ensure that the “{ foo” and “foo” parts are also not run?  Basically, I see that your test cases are not comprehensive.  Do you have a good reason for leaving those cases out?
> 
> Ditto for some of the test cases below.

There are more comprehensive if-statement tests inside the controlFlowProfiler/if-statement.js file.
But I should add a couple more clauses here just to make sure the braces are executed/not executed as expected.

>> Source/JavaScriptCore/tests/controlFlowProfiler/brace-location.js:45
>> +assert(hasBasicBlockExecuted(testForRegular, "{ bar"), "should have executed."); 
> 
> There is no “{ bar” in testForRegular.  So, how can this be executed?  I know this is a typo, and I also built and ran your patch + test, and found this test case to pass which makes me suspicious of the test mechanism.  Can you take a second look to see why it is passing when it shouldn’t?

Good catch.
My guess is that the test is passing because I do a WTFString::find on the source, which will return -1, which indicates it's not a substring. 
But, when searching for the offset, I add the offset of the start of the function, so it's just checking if that function ran, which it indeed did.

Testing hypothesis now. And also adding an assert to make sure that the supposed substring is indeed a substring.

[Saam Barati]

> Because the it's comprised like this: "<marker> } else ...", the marker will
/s/it's/if's/

> execute, then the "else b;" will be shown as not executing, but the last
/s/"else b;"/"else {b;"/

[Mark Lam]

(In reply to comment #7)
> > Right now, you would have:
> > if (x) { <marker> a; <marker> } ...
> > if (x) { <marker> a; <marker> } else { b; } <marker> ...
> > 
> > I would think to be consistent, it should be:
> > if (x) { <marker> a; <marker> } ...
> > if (x) { <marker> a; <marker> } else { b; <marker> } ...
> > 
> > or:
> > if (x) { <marker> a; } <marker> ...
> > if (x) { <marker> a; } <marker> else { b; } <marker> ...
> > 
> > Is there a good reason for the different treatment of the close brace for the if block and the else block?
> 
> Yeah, there is:
> Because the it's comprised like this: "<marker> } else ...", the marker will
> always have the "else" text offsets as being part of a new basic block.
> If that else statement never executes, then we ensure that "else { ... }" is
> marked entirely as not executed. 
> 
> If we did it this way: "else { b; <marker> }", if the else clause doesn't
> execute, then the "else b;" will be shown as not executing, but the last
> brace will be shown as executing, and this is just confusing to not have the
> braces match each other.

But how about the 3rd case:
if (x) { <marker> a; } <marker> ...
if (x) { <marker> a; } <marker> else { b; } <marker> ...

Wouldn’t that satisfy the requirement?  Or am I missing something?

For that matter, why not this alternative:

if (x) <marker> { a; } <marker> ...
if (x) <marker> { a; } <marker> else { b; } <marker> ...

Why do we need to exclude the open brace?

[Saam Barati]

(In reply to comment #9)
> (In reply to comment #7)
> > > Right now, you would have:
> > > if (x) { <marker> a; <marker> } ...
> > > if (x) { <marker> a; <marker> } else { b; } <marker> ...
> > > 
> > > I would think to be consistent, it should be:
> > > if (x) { <marker> a; <marker> } ...
> > > if (x) { <marker> a; <marker> } else { b; <marker> } ...
> > > 
> > > or:
> > > if (x) { <marker> a; } <marker> ...
> > > if (x) { <marker> a; } <marker> else { b; } <marker> ...
> > > 
> > > Is there a good reason for the different treatment of the close brace for the if block and the else block?
> > 
> > Yeah, there is:
> > Because the it's comprised like this: "<marker> } else ...", the marker will
> > always have the "else" text offsets as being part of a new basic block.
> > If that else statement never executes, then we ensure that "else { ... }" is
> > marked entirely as not executed. 
> > 
> > If we did it this way: "else { b; <marker> }", if the else clause doesn't
> > execute, then the "else b;" will be shown as not executing, but the last
> > brace will be shown as executing, and this is just confusing to not have the
> > braces match each other.
> 
> But how about the 3rd case:
> if (x) { <marker> a; } <marker> ...
> if (x) { <marker> a; } <marker> else { b; } <marker> ...
> 
> Wouldn’t that satisfy the requirement?  Or am I missing something?
> 
> For that matter, why not this alternative:
> 
> if (x) <marker> { a; } <marker> ...
> if (x) <marker> { a; } <marker> else { b; } <marker> ...
> 
> Why do we need to exclude the open brace?

There were two bugs that falsely lead me to think we could do:
"if (x) { <marker> a; <marker> } else { b; } <marker> ..."
and have the meta data associating the if statement's closing curly braces as always being executed. 
We had an off by one bug in the parser that gives the wrong opening brace offset 
to BlockNode's starting offset and also there was a UI bug in the inspector that 
also had a off by one error. This made things tricky to reason about.

After looking at the underlying data (not through the inspector's UI), it's obvious 
that if only one of the paths in an if/else execute, you can't have the closing if 
statement's brace always indicating that it's been executed. It must be tied to 
either the if-true branch executing or the else branch executing. Instead, my next patch 
will use Mark's above suggested scheme:
if (x) <marker> { a; } <marker> ...
if (x) <marker> { a; } <marker> else { b; } <marker> ...

which will always result in having both the open/close brace of the if statement tied
to the if-true branch.

[Saam Barati]

Created attachment 246258 [details]
perf tests

performance tests for upcoming patch

[Saam Barati]

Created attachment 246259 [details]
patch

Made the changes.

[Mark Lam]

Saam, in the benchmark results, I see:

JSRegress:
   array-splice-contiguous                           45.6274+-4.8273     !     54.3857+-3.7530        ! definitely 1.1920x slower

Can you take a look at what’s really happening there?  Is this regression real?

[Filip Pizlo]

(In reply to comment #13)
> Saam, in the benchmark results, I see:
> 
> JSRegress:
>    array-splice-contiguous                           45.6274+-4.8273     !  
> 54.3857+-3.7530        ! definitely 1.1920x slower
> 
> Can you take a look at what’s really happening there?  Is this regression
> real?

Let's not get into this.

The selection statistics of having so many tiny tests in JSRegress means that any code change will cause some subset of those tests to fluctuate, often by a lot.  This isn't a large or important test, so I don't think we should spend time investigating what happened unless there is also a trend of slow-downs in larger tests or in test averages.

[Saam Barati]

Created attachment 246296 [details]
more perf tests

Yeah, ran a two more runs of JSRegress before I saw Fil's comments. 
It looks like perf isn't really changed from this. 
Says it's "definitely" faster/slower in a few instances which I don't buy either way.

[Saam Barati]

There are a few more changes I need to make, actually. I've discovered another area that needs tweaking. Will upload patch soon. It will be 95% or more the same patch as this.

[Saam Barati]

Created attachment 246331 [details]
patch

[Mark Lam]

Comment on attachment 246331 [details]
patch

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

> Source/JavaScriptCore/ChangeLog:32
> +        * bytecode/CodeBlock.cpp:
> +        (JSC::CodeBlock::insertBasicBlockBoundariesForControlFlowProfiler):
> +        When computing basic block boundaries in CodeBlock, we ensure that every
> +        block's end offset is one less than its successor's start offset to
> +        maintain that boundaries' ranges should be mutually exclusive.
> +

Duplicate comment.  Please remove.

> Source/JavaScriptCore/tests/controlFlowProfiler/brace-location.js:41
> +assert(!hasBasicBlockExecuted(testIf, "{ bar"), "should not have executed."); 
> +assert(!hasBasicBlockExecuted(testIf, " bar"), "should not have executed."); 

I think you should also test this case:
assert(hasBasicBlockExecuted(testIf, "else if"), "should not have executed.");
assert(!hasBasicBlockExecuted(testIf, "else {"), "should have executed.");
assert(!hasBasicBlockExecuted(testIf, "{ baz"), "should have executed.");
assert(!hasBasicBlockExecuted(testIf, " baz"), "should have executed.");

Basically, you need to re-check all paths that remains un-executed, and paths that are newly executed.  And for this patch, we're particularly interested in the edge cases i.e "else if" and "else {".

> Source/JavaScriptCore/tests/controlFlowProfiler/brace-location.js:45
> +assert(hasBasicBlockExecuted(testIf, "{ bar"), "should have executed."); 
> +assert(hasBasicBlockExecuted(testIf, " bar"), "should have executed."); 

I think you should also test this case:
assert(hasBasicBlockExecuted(testIf, "else {"), "should have executed.");

> Source/JavaScriptCore/tests/controlFlowProfiler/brace-location.js:106
> +// So in this following case, the block that contains the '{'.

Invalid comment from copy paste error.  Please remove.

> Source/JavaScriptCore/tests/controlFlowProfiler/brace-location.js:110
> +assert(hasBasicBlockExecuted(testIfNoBraces, " foo"), "should have executed.");
> +assert(!hasBasicBlockExecuted(testIfNoBraces, " else if"), "should not have executed.");

Thinking about edge cases, how about the case:
assert(!hasBasicBlockExecuted(testIfNoBraces, "; else if"), "should have executed.");
assert(!hasBasicBlockExecuted(testIfNoBraces, ") bar"), "should not have executed.");
assert(!hasBasicBlockExecuted(testIfNoBraces, "; else baz"), "should not have executed.");

> Source/JavaScriptCore/tests/controlFlowProfiler/brace-location.js:112
> +assert(!hasBasicBlockExecuted(testIfNoBraces, "else"), "should not have executed."); 

Isn't "else" here ambiguous and will actually match the block for "else if (" instead of "else baz"?  I think you should test for "else baz".

nit: I happened to notice that there are empty spaces at the end of these lines (and possibly some of the ones above and below as well).  Can you remove them please?

> Source/JavaScriptCore/tests/controlFlowProfiler/brace-location.js:116
> +assert(!hasBasicBlockExecuted(testIfNoBraces, "bar"), "should not have executed."); 
> +assert(hasBasicBlockExecuted(testIfNoBraces, "else baz"), "should have executed."); 

Consider adding:
assert(!hasBasicBlockExecuted(testIfNoBraces, "else if"), "should not have executed.");
assert(!hasBasicBlockExecuted(testIfNoBraces, ") bar"), "should not have executed.");
assert(!hasBasicBlockExecuted(testIfNoBraces, "; else baz"), "should have executed.");

> Source/JavaScriptCore/tests/controlFlowProfiler/brace-location.js:119
> +assert(hasBasicBlockExecuted(testIfNoBraces, " bar"), "should have executed."); 
> +assert(hasBasicBlockExecuted(testIfNoBraces, "bar"), "should have executed."); 

Consider adding cases:
assert(!hasBasicBlockExecuted(testIfNoBraces, "else if"), "should have executed.");
assert(!hasBasicBlockExecuted(testIfNoBraces, ") bar"), "should have executed.");

> Source/JavaScriptCore/tests/controlFlowProfiler/brace-location.js:128
> +assert(hasBasicBlockExecuted(testForRegularNoBraces, " foo"), "should have executed."); 
> +assert(hasBasicBlockExecuted(testForRegularNoBraces, "foo"), "should have executed."); 

Consider adding:
assert(!hasBasicBlockExecuted(testForRegularNoBraces, "; bar"), "should have executed.");

> Source/JavaScriptCore/tests/controlFlowProfiler/brace-location.js:137
> +assert(hasBasicBlockExecuted(testForInNoBraces, " foo"), "should have executed."); 
> +assert(hasBasicBlockExecuted(testForInNoBraces, "foo"), "should have executed."); 

Consider adding:
assert(!hasBasicBlockExecuted(testForInNoBraces, "; bar"), "should have executed.");

> Source/JavaScriptCore/tests/controlFlowProfiler/brace-location.js:146
> +assert(hasBasicBlockExecuted(testForOfNoBraces, " foo"), "should have executed."); 
> +assert(hasBasicBlockExecuted(testForOfNoBraces, "foo"), "should have executed."); 

Consider adding:
assert(!hasBasicBlockExecuted(testForOfNoBraces, "; bar"), "should have executed.");

> Source/JavaScriptCore/tests/controlFlowProfiler/brace-location.js:155
> +assert(hasBasicBlockExecuted(testWhileNoBraces, " foo"), "should have executed."); 
> +assert(hasBasicBlockExecuted(testWhileNoBraces, "foo"), "should have executed."); 

Consider adding:
assert(!hasBasicBlockExecuted(testWhileNoBraces, "; bar"), "should have executed.");

> Source/JavaScriptCore/tests/controlFlowProfiler/conditional-expression.js:11
> +function testConditionalFunction(x, y) {

What makes this "testConditionalFunction"?  Maybe "testConditionalNested" might be a better name (assuming I'm reading your naming convention correctly)?

> Source/JavaScriptCore/tests/controlFlowProfiler/conditional-expression.js:20
> +assert(hasBasicBlockExecuted(testConditionalBasic, "x"), "should have executed.");
> +assert(hasBasicBlockExecuted(testConditionalBasic, "20"), "should have executed.");
> +assert(!hasBasicBlockExecuted(testConditionalBasic, "10"), "should not have executed yet.");

Consider adding:
assert(hasBasicBlockExecuted(testConditionalBasic, "?"), "should have executed.");
assert(hasBasicBlockExecuted(testConditionalBasic, ":"), "should have executed.");

I think you also need a copy of testConditionalBasic (maybe testConditionalBasic2) so that you can test testConditionalBasic2(true) first and ensure that the ":" is not executed.

> Source/JavaScriptCore/tests/controlFlowProfiler/conditional-expression.js:30
> +assert(hasBasicBlockExecuted(testConditionalFunction, "x"), "should have executed.");
> +assert(hasBasicBlockExecuted(testConditionalFunction, "bar"), "should have executed.");
> +assert(!hasBasicBlockExecuted(testConditionalFunction, "foo"), "should not have executed yet.");
> +assert(!hasBasicBlockExecuted(testConditionalFunction, "baz"), "should not have executed yet.");

Consider adding:
assert(hasBasicBlockExecuted(testConditionalFunction, "? y"), "should have executed.");
assert(hasBasicBlockExecuted(testConditionalFunction, "? foo"), "should not have executed.");
assert(!hasBasicBlockExecuted(testConditionalFunction, ": baz"), "should not have executed yet.");
assert(hasBasicBlockExecuted(testConditionalFunction, ": bar"), "should have executed.");

> Source/JavaScriptCore/tests/controlFlowProfiler/conditional-expression.js:34
> +assert(hasBasicBlockExecuted(testConditionalFunction, "baz"), "should have executed.");
> +assert(!hasBasicBlockExecuted(testConditionalFunction, "foo"), "should not have executed yet.");

Consider adding:
assert(hasBasicBlockExecuted(testConditionalFunction, "? foo"), "should have executed.");
assert(!hasBasicBlockExecuted(testConditionalFunction, ": baz"), "should have executed yet.");

[Saam Barati]

Created attachment 247024 [details]
patch

new patch with more edge cases in tests.

[Mark Lam]

Comment on attachment 247024 [details]
patch

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

> Source/JavaScriptCore/tests/controlFlowProfiler/brace-location.js:75
> +assert(hasBasicBlockExecuted(testWhile, " bar"), "should not have executed."); 

Shouldn't this be "should have executed."?

After spotting this mismatch between the assertion and the text string (and noticed a few others below), I'd like to suggest a different approach to expressing the tests:

1. At the top of this file, declare:

var ShouldHaveExecuted = true;
var ShouldNotHaveExecuted = false;

function checkBasicBlock(func, expr, expectation)
{
    if (expectation = ShouldHaveExecuted)
        assert(hasBasicBlockExecuted(func, expr, "should have executed");
    else
        assert(!hasBasicBlockExecuted(func, expr, "should not have executed");
}

2. When expressing the tests, express them like this:

checkBasicBlock(testWhile, " bar", ShouldHaveExecuted);

I think expressing them this way will eliminate cut-and-paste errors that caused these mismatches between the assertion and the text string.  Sorry that I didn't catch this the first time I read thru the tests.

> Source/JavaScriptCore/tests/controlFlowProfiler/brace-location.js:100
> +assert(hasBasicBlockExecuted(testIfNoBraces, " bar"), "should have executed."); 

This is strange that the white space before "bar" is attributed to the "else if (x < 20)" preceding it rather than the "bar" after it.  To the average human being, " bar" reads the same as "bar", and hence, would be interpreted as "bar" should have executed.  Hence, I was expecting this to result in "should not have executed." but was surprised to find the contrary.

Is this intentional?

> Source/JavaScriptCore/tests/controlFlowProfiler/brace-location.js:110
> +assert(hasBasicBlockExecuted(testForRegularNoBraces, " foo"), "should have executed."); 

Same here with the preceding white space.

> Source/JavaScriptCore/tests/controlFlowProfiler/brace-location.js:117
> +assert(hasBasicBlockExecuted(testForRegularNoBraces, " bar"), "should not have executed."); 

The text does not match the assertion.  It should read "should have executed".

> Source/JavaScriptCore/tests/controlFlowProfiler/brace-location.js:123
> +assert(hasBasicBlockExecuted(testForInNoBraces, " foo"), "should have executed."); 

Same here with the preceding white space.

> Source/JavaScriptCore/tests/controlFlowProfiler/brace-location.js:136
> +assert(hasBasicBlockExecuted(testForOfNoBraces, " foo"), "should have executed."); 

Same here with the preceding white space.

> Source/JavaScriptCore/tests/controlFlowProfiler/brace-location.js:149
> +assert(hasBasicBlockExecuted(testWhileNoBraces, " foo"), "should have executed."); 

Same here with the preceding white space.

[Saam Barati]

Created attachment 247133 [details]
patch

With changelog and test case describing why we split:
"if (cond) bar();"
into the blocks:
"if (cond) " and "bar();"

[Mark Lam]

Comment on attachment 247133 [details]
patch

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

> Source/JavaScriptCore/tests/controlFlowProfiler/brace-location.js:75
> +assert(hasBasicBlockExecuted(testWhile, " bar"), "should not have executed."); 

Still a mismatch here.

[Saam Barati]

Created attachment 247138 [details]
patch

Looks Mark's suggestion and re-wrote tests using the form:

checkBasicBlock(testWhile, " bar", ShouldHaveExecuted);

And things are much cleaner now.

[Saam Barati]

(In reply to comment #23)
> Created attachment 247138 [details]
> patch
> 
> Looks Mark's suggestion and re-wrote tests using the form:
Took*

[Mark Lam]

Comment on attachment 247138 [details]
patch

r=me

[WebKit Commit Bot]

Comment on attachment 247138 [details]
patch

Clearing flags on attachment: 247138

Committed r180518: <http://trac.webkit.org/changeset/180518>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.