Because why haven't we done this yet.
Created attachment 281283 [details] a little something like this I haven't really tried compiling it yet.
Created attachment 281295 [details] it just worked for a simple test case
Created attachment 281304 [details] better Fixing race conditions.
Created attachment 281311 [details] fixed more races
Created attachment 281323 [details] more
Created attachment 281331 [details] passes so many tests
Created attachment 281334 [details] better patch Removed some dataLog statements.
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.
Created attachment 281335 [details] more fixes
Created attachment 281336 [details] performance
Created attachment 281338 [details] possibly the patch
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 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
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
It looks like js/dom/prototype-chain-caching-with-impure-get-own-property-slot-traps.html is failing?
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
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 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
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 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
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
(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.
(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.
(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.
(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.
Created attachment 281399 [details] better patch
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.
Looks like this might be a 2% JSBench progression.
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.
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.
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.
Created attachment 281466 [details] the patch
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 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.
Created attachment 281472 [details] the patch
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 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.
Created attachment 281476 [details] patch for landing
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 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())
> > 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.
(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!
Created attachment 281482 [details] patch for landing Addresses Saam's comments.
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 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?
(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.
Created attachment 281497 [details] patch for landing
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 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.
Created attachment 281516 [details] patch for landing Polishing little things as I run more tests.
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.
Created attachment 281526 [details] patch for landing Found and fixed a 32-bit bug.
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.
Landed in http://trac.webkit.org/changeset/202157
Kraken got severely slower with this. Between 2% and 5% depending on the machine.
(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.
(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.
(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
(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
This is a 2.4% progression on JSBench according to A/B testing.
A/B testing shows no improvement on PLT on MacBookPro.