Bug 117375 – Re-worked non-local variable resolution

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Geoffrey Garen]

Re-worked non-local variable resolution

[Geoffrey Garen]

Created attachment 204104 [details]
Patch

[Filip Pizlo]

R-.  Patch is too small.

[Filip Pizlo]

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?

[Geoffrey Garen]

> 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;

[Filip Pizlo]

(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.

[Geoffrey Garen]

> 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).

[Geoffrey Garen]

Committed r151363: <http://trac.webkit.org/changeset/151363>

[Geoffrey Garen]

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

[Geoffrey Garen]

Rolled out in r151401.

[Geoffrey Garen]

Landed in r151481.

[Dean Jackson]

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>

[Dean Jackson]

(In reply to comment #11)
> This is causing crashes again.
> See <rdar://problem/14550047>

Phil says that it might be a subsequent commit.

[Filip Pizlo]

(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.