Bug 158755 – Baseline JIT should be concurrent

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

Because why haven't we done this yet.

[Filip Pizlo]

Created attachment 281283 [details]
a little something like this

I haven't really tried compiling it yet.

[Filip Pizlo]

Created attachment 281295 [details]
it just worked

for a simple test case

[Filip Pizlo]

Created attachment 281304 [details]
better

Fixing race conditions.

[Filip Pizlo]

Created attachment 281311 [details]
fixed more races

[Filip Pizlo]

Created attachment 281323 [details]
more

[Filip Pizlo]

Created attachment 281331 [details]
passes so many tests

[Filip Pizlo]

Created attachment 281334 [details]
better patch

Removed some dataLog statements.

[WebKit Commit Bot]

Attachment 281334 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:102:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:120:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:155:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 281335 [details]
more fixes

[Filip Pizlo]

Created attachment 281336 [details]
performance

[Filip Pizlo]

Created attachment 281338 [details]
possibly the patch

[WebKit Commit Bot]

Attachment 281338 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:102:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:120:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:155:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 281338 [details]
possibly the patch

Attachment 281338 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1505400

New failing tests:
js/dom/prototype-chain-caching-with-impure-get-own-property-slot-traps.html

[Build Bot]

Created attachment 281339 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Ryosuke Niwa]

It looks like js/dom/prototype-chain-caching-with-impure-get-own-property-slot-traps.html is failing?

[Build Bot]

Comment on attachment 281338 [details]
possibly the patch

Attachment 281338 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1505422

New failing tests:
js/dom/prototype-chain-caching-with-impure-get-own-property-slot-traps.html

[Build Bot]

Created attachment 281340 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 281338 [details]
possibly the patch

Attachment 281338 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1505434

New failing tests:
js/dom/prototype-chain-caching-with-impure-get-own-property-slot-traps.html

[Build Bot]

Created attachment 281341 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 281338 [details]
possibly the patch

Attachment 281338 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1506012

New failing tests:
js/dom/prototype-chain-caching-with-impure-get-own-property-slot-traps.html

[Build Bot]

Created attachment 281345 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Filip Pizlo]

(In reply to comment #15)
> It looks like
> js/dom/prototype-chain-caching-with-impure-get-own-property-slot-traps.html
> is failing?

Sure looks like it!  I hadn't yet built WebCore when I posted "possibly the patch".  There's still a lot of testing to be done.

[Filip Pizlo]

(In reply to comment #22)
> (In reply to comment #15)
> > It looks like
> > js/dom/prototype-chain-caching-with-impure-get-own-property-slot-traps.html
> > is failing?
> 
> Sure looks like it!  I hadn't yet built WebCore when I posted "possibly the
> patch".  There's still a lot of testing to be done.

Hmmm, I have a suspicion that this is simply revealing a LLInt vs. Baseline difference.  I bet it's an old LLInt bug.  I'll investigate.

[Filip Pizlo]

(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #15)
> > > It looks like
> > > js/dom/prototype-chain-caching-with-impure-get-own-property-slot-traps.html
> > > is failing?
> > 
> > Sure looks like it!  I hadn't yet built WebCore when I posted "possibly the
> > patch".  There's still a lot of testing to be done.
> 
> Hmmm, I have a suspicion that this is simply revealing a LLInt vs. Baseline
> difference.  I bet it's an old LLInt bug.  I'll investigate.

Ha!  This is totally a latent LLInt bug.  This:

JSC_useJIT=false DYLD_FRAMEWORK_PATH=WebKitBuild/Release/ WebKitBuild/Release/DumpRenderTree LayoutTests/js/dom/prototype-chain-caching-with-impure-get-own-property-slot-traps.html

fails.

[Filip Pizlo]

(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #22)
> > > (In reply to comment #15)
> > > > It looks like
> > > > js/dom/prototype-chain-caching-with-impure-get-own-property-slot-traps.html
> > > > is failing?
> > > 
> > > Sure looks like it!  I hadn't yet built WebCore when I posted "possibly the
> > > patch".  There's still a lot of testing to be done.
> > 
> > Hmmm, I have a suspicion that this is simply revealing a LLInt vs. Baseline
> > difference.  I bet it's an old LLInt bug.  I'll investigate.
> 
> Ha!  This is totally a latent LLInt bug.  This:
> 
> JSC_useJIT=false DYLD_FRAMEWORK_PATH=WebKitBuild/Release/
> WebKitBuild/Release/DumpRenderTree
> LayoutTests/js/dom/prototype-chain-caching-with-impure-get-own-property-slot-
> traps.html
> 
> fails.

I was able to write a test that causes ToT to fail reliably.  The problem is that the LLInt prototype load optimization forgot about impure property watchpoints on the base structure.

[Filip Pizlo]

Created attachment 281399 [details]
better patch

[WebKit Commit Bot]

Attachment 281399 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1903:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:103:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:121:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:156:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 4 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Looks like this might be a 2% JSBench progression.

[Filip Pizlo]

Comment on attachment 281399 [details]
better patch

I found another race condition while running PLT.  I think I have a fix for that, but that means I don't have PLT numbers yet.

[Filip Pizlo]

Created attachment 281447 [details]
even better patch

Fixed a couple more race conditions:

- ArrayProfile access paths in CodeBlock were racy.  LLInt can add array profiles as can the JIT.

- The eval path in the JIT generates a stub.  It's kinda weird.  The stub generation code is racy.  So, I used a link task to put that on the main thread.

So far this looks like a PLT win.  But, I'll need to do a few more runs to be sure.

[WebKit Commit Bot]

Attachment 281447 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1907:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITCall.cpp:130:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:103:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:121:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:156:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 5 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 281466 [details]
the patch

[WebKit Commit Bot]

Attachment 281466 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1907:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITCall.cpp:130:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:103:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:121:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:156:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 5 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Comment on attachment 281466 [details]
the patch

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

> Source/JavaScriptCore/ChangeLog:39
> +        This looks like it might be a ~1% Octane speed-up (on command line) and a ~7% PLT3 speed-up.

Ooops I meant 0.7%, not 7%.

> Source/JavaScriptCore/jit/JIT.h:943
> +        Deque<GetPutInfo> m_getPutInfos;
> +        Deque<ResolveType> m_resolveTypes;

I will remove this.  This was from an earlier attempt at resolving some races.

[Filip Pizlo]

Created attachment 281472 [details]
the patch

[WebKit Commit Bot]

Attachment 281472 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1907:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITCall.cpp:130:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:103:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:121:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:156:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 5 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 281472 [details]
the patch

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

r=me

> Source/JavaScriptCore/heap/Heap.cpp:1162
> -
> +    

Revert.

> Source/JavaScriptCore/jit/JIT.cpp:498
> -
> +    

Revert.

> Source/JavaScriptCore/jit/JITWorklist.cpp:205
> +    RELEASE_ASSERT(!codeBlock->m_didFailJITCompilation);

I think this can happen due to running out of executable memory on iOS.

> Source/JavaScriptCore/runtime/Options.cpp:372
> -
> +    

Revert.

> Source/WTF/wtf/RefCountedArray.h:60
> -
> +    

Revert.

[Filip Pizlo]

Created attachment 281476 [details]
patch for landing

[WebKit Commit Bot]

Attachment 281476 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1907:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITCall.cpp:130:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:103:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:121:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:156:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 5 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 281466 [details]
the patch

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

Quick comments:
I'm going to keep reading

> Source/JavaScriptCore/jit/JIT.cpp:113
> +        addSlowCase(Jump());

Was this a latent bug?

> Source/JavaScriptCore/jit/JIT.cpp:161
> +    if (false)

Do we have a verbose flag?

> Source/JavaScriptCore/jit/JIT.h:916
> +        Instruction* originalInstruction(Instruction*);

The name of this confused me, because I thought it was pointing to the non-copied instruction. Where this is really returning the copied instruction.
Maybe just call it copiedInstruction(...)?

> Source/JavaScriptCore/jit/JITInlines.h:1359
> +{

I think it's worth adding a debug assert here that
(inst >= m_codeBlock->instructions().begin() && inst < m_codeBlock->instructions().end())

[Geoffrey Garen]

> > Source/JavaScriptCore/jit/JIT.h:916
> > +        Instruction* originalInstruction(Instruction*);
> 
> The name of this confused me, because I thought it was pointing to the
> non-copied instruction. Where this is really returning the copied
> instruction.

Harrumph. I also thought it returned the non-copied instruction.

[Filip Pizlo]

(In reply to comment #40)
> Comment on attachment 281466 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281466&action=review
> 
> Quick comments:
> I'm going to keep reading
> 
> > Source/JavaScriptCore/jit/JIT.cpp:113
> > +        addSlowCase(Jump());
> 
> Was this a latent bug?

No, we were querying whether the watchpoint is valid a second time on the slow path to determine if there would be a jump.  That's a race.  This fixes the race, but ensuring that the slow path doesn't have to check the watchpoint: there will always be a Jump, but it may not be valid and the linking handles this gracefully.

> 
> > Source/JavaScriptCore/jit/JIT.cpp:161
> > +    if (false)
> 
> Do we have a verbose flag?

No, and we have a few other places that use "if (false)".

> 
> > Source/JavaScriptCore/jit/JIT.h:916
> > +        Instruction* originalInstruction(Instruction*);
> 
> The name of this confused me, because I thought it was pointing to the
> non-copied instruction. Where this is really returning the copied
> instruction.
> Maybe just call it copiedInstruction(...)?

OK.

> 
> > Source/JavaScriptCore/jit/JITInlines.h:1359
> > +{
> 
> I think it's worth adding a debug assert here that
> (inst >= m_codeBlock->instructions().begin() && inst <
> m_codeBlock->instructions().end())

Sure!

[Filip Pizlo]

Created attachment 281482 [details]
patch for landing

Addresses Saam's comments.

[WebKit Commit Bot]

Attachment 281482 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1907:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITCall.cpp:130:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:103:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:121:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:156:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 5 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 281476 [details]
patch for landing

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

> Source/JavaScriptCore/jit/JIT.cpp:688
> +    m_codeBlock->setCalleeSaveRegisters(RegisterSet::llintBaselineCalleeSaveRegisters()); // Might be able to remove as this is probably already set to this value.

yeah it should be already set to this. I believe CodeBlock::finishCreation (the non copy version) does this

> Source/JavaScriptCore/jit/JIT.h:197
> +        JIT(VM*, CodeBlock* = 0);

I know this was moved to be public, but is it even valid to create a JIT without a CodeBlock?

> Source/JavaScriptCore/jit/JIT.h:916
> +        Instruction* originalInstruction(Instruction*);

It might also be worth adding a comment here as to why one might want to use this

> Source/JavaScriptCore/jit/JITWorklist.cpp:149
> +void JITWorklist::poll(VM& vm)

Style: I think a better name would be finalizeCompletedCompiles or something similar

> Source/JavaScriptCore/jit/JITWorklist.cpp:186
> +    codeBlock->jitSoon();

Maybe it's profitable have a number specialized for the baseline here, since compiles will often finish much faster than DFG compiles, and LLInt code runs slowly

> Source/JavaScriptCore/jit/JITWorklist.cpp:223
> +    // Now it might be compiled!

might? Shouldn't it definitely be compiled?

[Saam Barati]

Comment on attachment 281476 [details]
patch for landing

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

> Source/JavaScriptCore/jit/JIT.cpp:688
> +    m_codeBlock->setCalleeSaveRegisters(RegisterSet::llintBaselineCalleeSaveRegisters()); // Might be able to remove as this is probably already set to this value.

yeah it should be already set to this. I believe CodeBlock::finishCreation (the non copy version) does this

> Source/JavaScriptCore/jit/JIT.h:197
> +        JIT(VM*, CodeBlock* = 0);

I know this was moved to be public, but is it even valid to create a JIT without a CodeBlock?

> Source/JavaScriptCore/jit/JIT.h:916
> +        Instruction* originalInstruction(Instruction*);

It might also be worth adding a comment here as to why one might want to use this

> Source/JavaScriptCore/jit/JITWorklist.cpp:149
> +void JITWorklist::poll(VM& vm)

Style: I think a better name would be finalizeCompletedCompiles or something similar

> Source/JavaScriptCore/jit/JITWorklist.cpp:186
> +    codeBlock->jitSoon();

Maybe it's profitable have a number specialized for the baseline here, since compiles will often finish much faster than DFG compiles, and LLInt code runs slowly

> Source/JavaScriptCore/jit/JITWorklist.cpp:223
> +    // Now it might be compiled!

might? Shouldn't it definitely be compiled?

[Filip Pizlo]

(In reply to comment #46)
> Comment on attachment 281476 [details]
> patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281476&action=review
> 
> > Source/JavaScriptCore/jit/JIT.cpp:688
> > +    m_codeBlock->setCalleeSaveRegisters(RegisterSet::llintBaselineCalleeSaveRegisters()); // Might be able to remove as this is probably already set to this value.
> 
> yeah it should be already set to this. I believe CodeBlock::finishCreation
> (the non copy version) does this
> 
> > Source/JavaScriptCore/jit/JIT.h:197
> > +        JIT(VM*, CodeBlock* = 0);
> 
> I know this was moved to be public, but is it even valid to create a JIT
> without a CodeBlock?

Yes.  There are still a few places that use a JIT instance for compiling a stub.

> 
> > Source/JavaScriptCore/jit/JIT.h:916
> > +        Instruction* originalInstruction(Instruction*);
> 
> It might also be worth adding a comment here as to why one might want to use
> this

OK.

> 
> > Source/JavaScriptCore/jit/JITWorklist.cpp:149
> > +void JITWorklist::poll(VM& vm)
> 
> Style: I think a better name would be finalizeCompletedCompiles or something
> similar

But that would leak the word "finalize" out through the JITWorklist interface.  I got burned by using longer names in DFG::Worklist and now I have a hard time keeping those methods straight in my head.  I like "poll".  It's easy to remember.

> 
> > Source/JavaScriptCore/jit/JITWorklist.cpp:186
> > +    codeBlock->jitSoon();
> 
> Maybe it's profitable have a number specialized for the baseline here, since
> compiles will often finish much faster than DFG compiles, and LLInt code
> runs slowly

There's already a number specialized for the baseline.

> 
> > Source/JavaScriptCore/jit/JITWorklist.cpp:223
> > +    // Now it might be compiled!
> 
> might? Shouldn't it definitely be compiled?

isPlanned could have been false, above.  I.e. the CodeBlock wasn't known to the worklist.  In that case we need to compile it ourselves.

[Filip Pizlo]

Created attachment 281497 [details]
patch for landing

[WebKit Commit Bot]

Attachment 281497 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1907:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITCall.cpp:130:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:103:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:121:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:156:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 5 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 281476 [details]
patch for landing

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

>>>> Source/JavaScriptCore/jit/JITWorklist.cpp:223
>>>> +    // Now it might be compiled!
>>> 
>>> might? Shouldn't it definitely be compiled?
>> 
>> might? Shouldn't it definitely be compiled?
> 
> isPlanned could have been false, above.  I.e. the CodeBlock wasn't known to the worklist.  In that case we need to compile it ourselves.

I see, I thought this comment was just describing this branch, but it's really describing the rest of the region of code until the end of the function.

[Filip Pizlo]

Created attachment 281516 [details]
patch for landing

Polishing little things as I run more tests.

[WebKit Commit Bot]

Attachment 281516 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1907:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITCall.cpp:130:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:103:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:121:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:156:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 5 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 281526 [details]
patch for landing

Found and fixed a 32-bit bug.

[WebKit Commit Bot]

Attachment 281526 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1907:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITCall.cpp:130:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:103:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:121:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITWorklist.cpp:156:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 5 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/202157

[Benjamin Poulain]

Kraken got severely slower with this. Between 2% and 5% depending on the machine.

[Filip Pizlo]

(In reply to comment #56)
> Kraken got severely slower with this. Between 2% and 5% depending on the
> machine.

Looks like it's all because of ai-astar.

[Filip Pizlo]

(In reply to comment #57)
> (In reply to comment #56)
> > Kraken got severely slower with this. Between 2% and 5% depending on the
> > machine.
> 
> Looks like it's all because of ai-astar.

I'll investigate this more tomorrow.  Easy to repro in browser.

Note that this change is also looking like a significant JSBench speed-up.  I guess it's OK to roll this out, but on the other hand, we can use the JSBench perf.

[Filip Pizlo]

(In reply to comment #58)
> (In reply to comment #57)
> > (In reply to comment #56)
> > > Kraken got severely slower with this. Between 2% and 5% depending on the
> > > machine.
> > 
> > Looks like it's all because of ai-astar.
> 
> I'll investigate this more tomorrow.  Easy to repro in browser.
> 
> Note that this change is also looking like a significant JSBench speed-up. 
> I guess it's OK to roll this out, but on the other hand, we can use the
> JSBench perf.

Tracking here: https://bugs.webkit.org/show_bug.cgi?id=158906

[Filip Pizlo]

(In reply to comment #59)
> (In reply to comment #58)
> > (In reply to comment #57)
> > > (In reply to comment #56)
> > > > Kraken got severely slower with this. Between 2% and 5% depending on the
> > > > machine.
> > > 
> > > Looks like it's all because of ai-astar.
> > 
> > I'll investigate this more tomorrow.  Easy to repro in browser.
> > 
> > Note that this change is also looking like a significant JSBench speed-up. 
> > I guess it's OK to roll this out, but on the other hand, we can use the
> > JSBench perf.
> 
> Tracking here: https://bugs.webkit.org/show_bug.cgi?id=158906

There's a fix: https://bugs.webkit.org/attachment.cgi?id=281618&action=review

[Filip Pizlo]

This is a 2.4% progression on JSBench according to A/B testing.

[Chris Dumez]

A/B testing shows no improvement on PLT on MacBookPro.