Re-worked non-local variable resolution
Created attachment 204104 [details] Patch
R-. Patch is too small.
Comment on attachment 204104 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=204104&action=review Looks great, but I'm confused about how you handle the inferred immutable globals - previously we made these both sound and fairly efficient: the JIT would emit a store barrier, via PutGlobalVarCheck and friends. How is this done now? > Source/JavaScriptCore/dfg/DFGNode.h:602 > - return op() == GlobalVarWatchpoint || op() == PutGlobalVarCheck; > + return op() == GlobalVarWatchpoint; What now does the work of PutGlobalVarCheck?
> Looks great, but I'm confused about how you handle the inferred immutable globals - previously we made these both sound and fairly efficient: the JIT would emit a store barrier, via PutGlobalVarCheck and friends. How is this done now? The store barrier fires at compile time now, instead of runtime: if (JSGlobalObject* globalObject = jsDynamicCast<JSGlobalObject*>(scope)) { SymbolTableEntry entry = globalObject->symbolTable()->get(ident.impl()); if (!entry.isNull()) { if (getOrPut == Put) { if (entry.isReadOnly()) { // We know the property will be at global scope, but we don't know how to cache it. op = ResolveOp(Dynamic, 0, 0, 0); return true; } // It's likely that we'll write to this var, so notify now and avoid the overhead of doing so at runtime. entry.notifyWrite(); } (There's another case of this when linking init_global_const.) I figured this was a reasonable tradeoff because this case would be rare: function f() { return 42; } if (false) f = 42;
(In reply to comment #4) > > Looks great, but I'm confused about how you handle the inferred immutable globals - previously we made these both sound and fairly efficient: the JIT would emit a store barrier, via PutGlobalVarCheck and friends. How is this done now? > > The store barrier fires at compile time now, instead of runtime: > > if (JSGlobalObject* globalObject = jsDynamicCast<JSGlobalObject*>(scope)) { > SymbolTableEntry entry = globalObject->symbolTable()->get(ident.impl()); > if (!entry.isNull()) { > if (getOrPut == Put) { > if (entry.isReadOnly()) { > // We know the property will be at global scope, but we don't know how to cache it. > op = ResolveOp(Dynamic, 0, 0, 0); > return true; > } > > // It's likely that we'll write to this var, so notify now and avoid the overhead of doing so at runtime. > entry.notifyWrite(); > } > > (There's another case of this when linking init_global_const.) > > I figured this was a reasonable tradeoff because this case would be rare: > > function f() { return 42; } > if (false) > f = 42; Interesting. The reason why it was done the other way before was that I wanted to extend the immutability inference to all globals. They would all be treated as if they were immutable until the first time you overwrote them. But I guess that change would still be easy so it doesn't matter.
> Interesting. The reason why it was done the other way before was that I wanted to extend the immutability inference to all globals. They would all be treated as if they were immutable until the first time you overwrote them. > > But I guess that change would still be easy so it doesn't matter. Yeah, this is totally doable (and a good idea, I think).
Committed r151363: <http://trac.webkit.org/changeset/151363>
Looks like this caused some concurrent compilation crashes, such as: Exception Type: EXC_BAD_ACCESS (SIGSEGV) Exception Codes: KERN_INVALID_ADDRESS at 0x00000000bbadbeef VM Regions Near 0xbbadbeef: --> __TEXT 000000010281f000-00000001028bb000 [ 624K] r-x/rwx SM=COW /Volumes/VOLUME/* Application Specific Information: CRASHING TEST: inspector/report-protocol-errors.html Thread 12 Crashed:: JSC Compilation Thread 0 com.apple.JavaScriptCore 0x0000000102ab524f JSC::DFG::AbstractState::executeEffects(unsigned int, JSC::DFG::Node*) + 19599 (DFGAbstractState.cpp:1365) 1 com.apple.JavaScriptCore 0x0000000102ab646b JSC::DFG::AbstractState::execute(unsigned int) + 107 (DFGAbstractState.cpp:1592) 2 com.apple.JavaScriptCore 0x0000000102b046af JSC::DFG::CFAPhase::performBlockCFA(unsigned int) + 175 (DFGCFAPhase.cpp:104) 3 com.apple.JavaScriptCore 0x0000000102b045e2 JSC::DFG::CFAPhase::performForwardCFA() + 98 (DFGCFAPhase.cpp:130) 4 com.apple.JavaScriptCore 0x0000000102b0455e JSC::DFG::CFAPhase::run() + 318 (DFGCFAPhase.cpp:76) 5 com.apple.JavaScriptCore 0x0000000102b04365 bool JSC::DFG::runAndLog<JSC::DFG::CFAPhase>(JSC::DFG::CFAPhase&) + 21 (DFGPhase.h:75) 6 com.apple.JavaScriptCore 0x0000000102b042ee bool JSC::DFG::runPhase<JSC::DFG::CFAPhase>(JSC::DFG::Graph&) + 46 (DFGPhase.h:85) 7 com.apple.JavaScriptCore 0x0000000102b042a8 JSC::DFG::performCFA(JSC::DFG::Graph&) + 40 (DFGCFAPhase.cpp:144) 8 com.apple.JavaScriptCore 0x0000000102c8c63c JSC::DFG::Plan::compileInThreadImpl(JSC::DFG::LongLivedState&) + 572 (DFGPlan.cpp:163) 9 com.apple.JavaScriptCore 0x0000000102c8c2b4 JSC::DFG::Plan::compileInThread(JSC::DFG::LongLivedState&) + 196 (DFGPlan.cpp:102) 10 com.apple.JavaScriptCore 0x0000000102e1b09c JSC::DFG::Worklist::runThread() + 412 (DFGWorklist.cpp:241) 11 com.apple.JavaScriptCore 0x0000000102e1a1c5 JSC::DFG::Worklist::threadFunction(void*) + 21 (DFGWorklist.cpp:264) 12 com.apple.JavaScriptCore 0x0000000103950d80 WTF::threadEntryPoint(void*) + 144 (Threading.cpp:70) 13 com.apple.JavaScriptCore 0x0000000103951778 WTF::wtfThreadEntryPoint(void*) + 104 (ThreadingPthreads.cpp:197) 14 libsystem_c.dylib 0x00007fff926ff7a2 _pthread_start + 327 15 libsystem_c.dylib 0x00007fff926ec1e1 thread_start + 13
Rolled out in r151401.
Landed in r151481.
This is causing crashes again. 1 0x000044de359e167f 0 + 75721172981375 > 2 com.apple.JavaScriptCore 0x10b92cfe1 JSC::JITCode::execute(JSC::JSStack*, JSC::ExecState*, JSC::VM*) + 0x31 3 com.apple.JavaScriptCore 0x10b91240a JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 0x28a 4 com.apple.JavaScriptCore 0x10b7f8d45 JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 0x45 5 com.apple.WebCore 0x10c2726ac WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext*, WebCore::Event*) + 0x38c 6 com.apple.WebCore 0x10bf3dbbc WebCore::EventTarget::fireEventListeners(WebCore::Event*, WebCore::EventTargetData*, WTF::Vector<WebCore::RegisteredEventListener, 1ul, WTF::CrashOnOverflow>&) + 0x16c 7 com.apple.WebCore 0x10bf3d8d6 WebCore::EventTarget::fireEventListeners(WebCore::Event*) + 0x186 8 com.apple.WebCore 0x10c537f53 WebCore::Node::handleLocalEvents(WebCore::Event*) + 0x43 9 com.apple.WebCore 0x10bf25fb7 WebCore::EventContext::handleLocalEvents(WebCore::Event*) const + 0x57 10 com.apple.WebCore 0x10bf26d61 WebCore::EventDispatcher::dispatch() + 0x291 11 com.apple.WebCore 0x10c52519f WebCore::MouseEventDispatchMediator::dispatchEvent(WebCore::EventDispatcher*) const + 0x9f 12 com.apple.WebCore 0x10bf261cc WebCore::EventDispatcher::dispatchEvent(WebCore::Node*, WTF::PassRefPtr<WebCore::EventDispatchMediator>) + 0x7c 13 com.apple.WebCore 0x10c538655 WebCore::Node::dispatchMouseEvent(WebCore::PlatformMouseEvent const&, WTF::AtomicString const&, int, WebCore::Node*) + 0x85 14 com.apple.WebCore 0x10bf2d95b WebCore::EventHandler::dispatchMouseEvent(WTF::AtomicString const&, WebCore::Node*, bool, int, WebCore::PlatformMouseEvent const&, bool) + 0x6b See <rdar://problem/14550047>
(In reply to comment #11) > This is causing crashes again. > See <rdar://problem/14550047> Phil says that it might be a subsequent commit.
(In reply to comment #12) > (In reply to comment #11) > > This is causing crashes again. > > See <rdar://problem/14550047> > > Phil says that it might be a subsequent commit. Sorry, I mean that there's so many commits after that, that rolling out this particular commit would be rough.