Bug 158755 - Baseline JIT should be concurrent
Summary: Baseline JIT should be concurrent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-14 14:44 PDT by Filip Pizlo
Modified: 2016-07-08 09:01 PDT (History)
14 users (show)

See Also:


Attachments
a little something like this (15.73 KB, patch)
2016-06-14 14:45 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
it just worked (27.03 KB, patch)
2016-06-14 16:19 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
better (39.92 KB, patch)
2016-06-14 17:55 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
fixed more races (40.02 KB, patch)
2016-06-14 18:48 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
more (44.29 KB, patch)
2016-06-14 21:18 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
passes so many tests (44.28 KB, patch)
2016-06-14 21:49 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
better patch (42.78 KB, patch)
2016-06-14 21:53 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
more fixes (43.63 KB, patch)
2016-06-14 22:07 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
performance (78.78 KB, text/plain)
2016-06-14 22:47 PDT, Filip Pizlo
no flags Details
possibly the patch (49.96 KB, patch)
2016-06-14 22:57 PDT, Filip Pizlo
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (804.23 KB, application/zip)
2016-06-14 23:56 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (809.32 KB, application/zip)
2016-06-15 00:00 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-yosemite (1.47 MB, application/zip)
2016-06-15 00:06 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (660.10 KB, application/zip)
2016-06-15 02:05 PDT, Build Bot
no flags Details
better patch (57.79 KB, patch)
2016-06-15 16:01 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
even better patch (61.37 KB, patch)
2016-06-16 00:26 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (61.50 KB, patch)
2016-06-16 12:01 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (61.40 KB, patch)
2016-06-16 14:16 PDT, Filip Pizlo
ggaren: review+
Details | Formatted Diff | Diff
patch for landing (59.95 KB, patch)
2016-06-16 14:52 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
patch for landing (59.97 KB, patch)
2016-06-16 15:29 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
patch for landing (60.21 KB, patch)
2016-06-16 15:58 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
patch for landing (62.07 KB, patch)
2016-06-16 18:17 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
patch for landing (63.48 KB, patch)
2016-06-16 20:08 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2016-06-14 14:44:34 PDT
Because why haven't we done this yet.
Comment 1 Filip Pizlo 2016-06-14 14:45:12 PDT
Created attachment 281283 [details]
a little something like this

I haven't really tried compiling it yet.
Comment 2 Filip Pizlo 2016-06-14 16:19:44 PDT
Created attachment 281295 [details]
it just worked

for a simple test case
Comment 3 Filip Pizlo 2016-06-14 17:55:25 PDT
Created attachment 281304 [details]
better

Fixing race conditions.
Comment 4 Filip Pizlo 2016-06-14 18:48:33 PDT
Created attachment 281311 [details]
fixed more races
Comment 5 Filip Pizlo 2016-06-14 21:18:01 PDT
Created attachment 281323 [details]
more
Comment 6 Filip Pizlo 2016-06-14 21:49:49 PDT
Created attachment 281331 [details]
passes so many tests
Comment 7 Filip Pizlo 2016-06-14 21:53:01 PDT
Created attachment 281334 [details]
better patch

Removed some dataLog statements.
Comment 8 WebKit Commit Bot 2016-06-14 21:54:05 PDT
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.
Comment 9 Filip Pizlo 2016-06-14 22:07:53 PDT
Created attachment 281335 [details]
more fixes
Comment 10 Filip Pizlo 2016-06-14 22:47:55 PDT
Created attachment 281336 [details]
performance
Comment 11 Filip Pizlo 2016-06-14 22:57:03 PDT
Created attachment 281338 [details]
possibly the patch
Comment 12 WebKit Commit Bot 2016-06-14 22:58:10 PDT
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.
Comment 13 Build Bot 2016-06-14 23:56:29 PDT
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
Comment 14 Build Bot 2016-06-14 23:56:33 PDT
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
Comment 15 Ryosuke Niwa 2016-06-14 23:57:14 PDT
It looks like js/dom/prototype-chain-caching-with-impure-get-own-property-slot-traps.html is failing?
Comment 16 Build Bot 2016-06-15 00:00:32 PDT
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
Comment 17 Build Bot 2016-06-15 00:00:38 PDT
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
Comment 18 Build Bot 2016-06-15 00:06:01 PDT
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
Comment 19 Build Bot 2016-06-15 00:06:08 PDT
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
Comment 20 Build Bot 2016-06-15 02:05:00 PDT
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
Comment 21 Build Bot 2016-06-15 02:05:04 PDT
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
Comment 22 Filip Pizlo 2016-06-15 12:05:52 PDT
(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.
Comment 23 Filip Pizlo 2016-06-15 12:07:02 PDT
(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.
Comment 24 Filip Pizlo 2016-06-15 15:23:32 PDT
(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.
Comment 25 Filip Pizlo 2016-06-15 15:55:04 PDT
(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.
Comment 26 Filip Pizlo 2016-06-15 16:01:20 PDT
Created attachment 281399 [details]
better patch
Comment 27 WebKit Commit Bot 2016-06-15 16:03:28 PDT
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.
Comment 28 Filip Pizlo 2016-06-15 16:54:17 PDT
Looks like this might be a 2% JSBench progression.
Comment 29 Filip Pizlo 2016-06-15 20:03:06 PDT
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.
Comment 30 Filip Pizlo 2016-06-16 00:26:10 PDT
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.
Comment 31 WebKit Commit Bot 2016-06-16 00:27:10 PDT
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.
Comment 32 Filip Pizlo 2016-06-16 12:01:58 PDT
Created attachment 281466 [details]
the patch
Comment 33 WebKit Commit Bot 2016-06-16 12:04:40 PDT
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.
Comment 34 Filip Pizlo 2016-06-16 12:05:59 PDT
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.
Comment 35 Filip Pizlo 2016-06-16 14:16:05 PDT
Created attachment 281472 [details]
the patch
Comment 36 WebKit Commit Bot 2016-06-16 14:17:55 PDT
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.
Comment 37 Geoffrey Garen 2016-06-16 14:46:09 PDT
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.
Comment 38 Filip Pizlo 2016-06-16 14:52:44 PDT
Created attachment 281476 [details]
patch for landing
Comment 39 WebKit Commit Bot 2016-06-16 14:54:41 PDT
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.
Comment 40 Saam Barati 2016-06-16 15:11:36 PDT
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())
Comment 41 Geoffrey Garen 2016-06-16 15:14:09 PDT
> > 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.
Comment 42 Filip Pizlo 2016-06-16 15:16:43 PDT
(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!
Comment 43 Filip Pizlo 2016-06-16 15:29:06 PDT
Created attachment 281482 [details]
patch for landing

Addresses Saam's comments.
Comment 44 WebKit Commit Bot 2016-06-16 15:31:04 PDT
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.
Comment 45 Saam Barati 2016-06-16 15:46:37 PDT
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?
Comment 46 Saam Barati 2016-06-16 15:46:43 PDT
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?
Comment 47 Filip Pizlo 2016-06-16 15:56:17 PDT
(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.
Comment 48 Filip Pizlo 2016-06-16 15:58:23 PDT
Created attachment 281497 [details]
patch for landing
Comment 49 WebKit Commit Bot 2016-06-16 16:01:44 PDT
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.
Comment 50 Saam Barati 2016-06-16 16:42:30 PDT
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.
Comment 51 Filip Pizlo 2016-06-16 18:17:22 PDT
Created attachment 281516 [details]
patch for landing

Polishing little things as I run more tests.
Comment 52 WebKit Commit Bot 2016-06-16 18:20:20 PDT
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.
Comment 53 Filip Pizlo 2016-06-16 20:08:53 PDT
Created attachment 281526 [details]
patch for landing

Found and fixed a 32-bit bug.
Comment 54 WebKit Commit Bot 2016-06-16 20:11:03 PDT
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.
Comment 55 Filip Pizlo 2016-06-16 21:49:16 PDT
Landed in http://trac.webkit.org/changeset/202157
Comment 56 Benjamin Poulain 2016-06-17 22:34:45 PDT
Kraken got severely slower with this. Between 2% and 5% depending on the machine.
Comment 57 Filip Pizlo 2016-06-17 23:49:58 PDT
(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.
Comment 58 Filip Pizlo 2016-06-18 00:12:43 PDT
(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.
Comment 59 Filip Pizlo 2016-06-18 12:32:24 PDT
(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
Comment 60 Filip Pizlo 2016-06-18 13:55:32 PDT
(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
Comment 61 Filip Pizlo 2016-06-27 16:30:02 PDT
This is a 2.4% progression on JSBench according to A/B testing.
Comment 62 Chris Dumez 2016-07-08 09:01:19 PDT
A/B testing shows no improvement on PLT on MacBookPro.