Attachment #222935 for bug #128072

View Details Raw Unified Return to Bug 128072

| Differences between



2014-02-02  Filip Pizlo  <fpizlo@apple.com>

        GC should keep structures alive if they are inlined into optimized code (DFG or FTL) and their globalObject is alive
        https://bugs.webkit.org/show_bug.cgi?id=128072

        Reviewed by NOBODY (OOPS!).
        
        The original role of the structure transition fixpoint was to guard against cases
        where we had code that cached on a now-dead structure with a now-dead global object.
        Originally cached structures were strong references and so such a case could lead to
        a massive memory leak.
        
        Of course, we couldn't turn these references into simply weak references because then
        we would be throwing away code too frequently. It's surprisingly common to have
        idioms that reduce to something like:
        
            for (forever) {
                var o = new SuperImportantStateObject();
                // Do a bunch of work involving o.
                o = null; // Something that kills o.
                // Do some more work, and we GC here.
            }
        
        Usually the "o = null" arises because the allocation of the object is inside of a
        function that the loop calls. This is particularly frequent on the Web because that
        outer loop is actually the runloop and the GC is typically scheduled when the runloop
        is idle. So, if we treated all cached structures as weak and the app had any types
        that existed only during event handling but typically didn't remain reachable after
        the event got handled, then we would be throwing away a lot of code.
        
        So, instead of making those structures completely weak, we threw in a clever trick:
        we would fixpoint over all cached transitions that create new structures, and if any
        of those transitions was a part of code that appeared live, then we would mark the
        target structure. This tended to keep alive structures for objects that hot code
        would tend to create.
        
        But now it looks like this isn't enough. Those transition caches are just that -
        caches. They are imperfect. So, this patch adds another rule for keeping structures
        alive:
        
            If a structure is cached by an optimizing compiler and its global object is
            otherwise still live, then mark the structure.
        
        This rule will usually dominate the transition rule, but the transition rule is still
        critical for when we haven't created optimized code yet or for structures that aren't
        associated with a global object.
        
        This fix introduces two possibilities for simplifying the code, but they probably
        require some more investigation:
        
        - https://bugs.webkit.org/show_bug.cgi?id=128078
          Consider getting rid of the GC transition fixpoint for optimized code, since the
          global object rule is likely to dominate it
            
        - https://bugs.webkit.org/show_bug.cgi?id=128079
          Consider getting rid of the StructureStubInfo::resetByGC flag because the global
          object rule implies that caching on possibly-GCable structures is no longer
          disastrous
        
        This looks like a 50% speed-up on Octane2/gbemu in the FTL.
        
        This also includes a bunch of debug support for tracking what is going on inside of
        the GC's structure clearing. That's actually the bulk of this patch.

        * bytecode/CodeBlock.cpp:
        (JSC::dumpStructure):
        (JSC::dumpChain):
        (JSC::dumpStructureStubInfo):
        (JSC::CodeBlock::printGetByIdCacheStatus):
        (JSC::forceStructureLiveness):
        (JSC::CodeBlock::propagateTransitions):
        (JSC::CodeBlock::finalizeUnconditionally):
        * runtime/Options.h:

2014-02-01  Filip Pizlo  <fpizlo@apple.com>

        JSC profiler's stub info profiling support should work again



}

#if ENABLE(JIT) || ENABLE(LLINT) // unused in some configurations
static void dumpStructure(PrintStream& out, const char* name, VM& vm, Structure* structure, const Identifier* ident)
{
    if (!structure)
        return;
    
    out.printf("%s = %p", name, structure);
    
    if (ident) {
        PropertyOffset offset = structure->getConcurrently(vm, ident->impl());
        if (offset != invalidOffset)
            out.printf(" (offset = %d)", offset);
    }
}
#endif

#if ENABLE(JIT) // unused when not ENABLE(JIT), leading to silly warnings
static void dumpChain(PrintStream& out, VM& vm, StructureChain* chain, const Identifier* ident)
{
    out.printf("chain = %p: [", chain);
    bool first = true;

            first = false;
        else
            out.printf(", ");
        dumpStructure(out, "struct", vm, currentStructure->get(), ident);
    }
    out.printf("]");
}

static void dumpStructureStubInfo(PrintStream& out, VM& vm, StructureStubInfo& stubInfo, const Identifier* ident)
{
    Structure* baseStructure = 0;
    Structure* prototypeStructure = 0;
    StructureChain* chain = 0;
    PolymorphicAccessStructureList* structureList = 0;
    int listSize = 0;
    
    switch (stubInfo.accessType) {
    case access_get_by_id_self:
        out.printf("self");
        baseStructure = stubInfo.u.getByIdSelf.baseObjectStructure.get();
        break;
    case access_get_by_id_proto:
        out.printf("proto");
        baseStructure = stubInfo.u.getByIdProto.baseObjectStructure.get();
        prototypeStructure = stubInfo.u.getByIdProto.prototypeStructure.get();
        break;
    case access_get_by_id_chain:
        out.printf("chain");
        baseStructure = stubInfo.u.getByIdChain.baseObjectStructure.get();
        chain = stubInfo.u.getByIdChain.chain.get();
        break;
    case access_get_by_id_self_list:
        out.printf("self_list");
        structureList = stubInfo.u.getByIdSelfList.structureList;
        listSize = stubInfo.u.getByIdSelfList.listSize;
        break;
    case access_get_by_id_proto_list:
        out.printf("proto_list");
        structureList = stubInfo.u.getByIdProtoList.structureList;
        listSize = stubInfo.u.getByIdProtoList.listSize;
        break;
    case access_unset:
        out.printf("unset");
        break;
    case access_get_by_id_generic:
        out.printf("generic");
        break;
    case access_get_array_length:
        out.printf("array_length");
        break;
    case access_get_string_length:
        out.printf("string_length");
        break;
    case access_in_list:
        out.printf("in_list");
        structureList = stubInfo.u.inList.structureList;
        listSize = stubInfo.u.inList.listSize;
        break;
    case access_put_by_id_transition_normal:
    case access_put_by_id_transition_direct:
    case access_put_by_id_replace:
    case access_put_by_id_list:
    case access_put_by_id_generic:
        // FIXME: Support dumping these.
        // https://bugs.webkit.org/show_bug.cgi?id=128062
        return;
    default:
        RELEASE_ASSERT_NOT_REACHED();
        break;
    }
    
    if (baseStructure) {
        out.printf(", ");
        dumpStructure(out, "struct", vm, baseStructure, ident);
    }
    
    if (prototypeStructure) {
        out.printf(", ");
        dumpStructure(out, "prototypeStruct", vm, baseStructure, ident);
    }
    
    if (chain) {
        out.printf(", ");
        dumpChain(out, vm, chain, ident);
    }
    
    if (structureList) {
        out.printf(", list = %p: [", structureList);
        for (int i = 0; i < listSize; ++i) {
            if (i)
                out.printf(", ");
            out.printf("(");
            dumpStructure(out, "base", vm, structureList->list[i].base.get(), ident);
            if (structureList->list[i].isChain) {
                if (structureList->list[i].u.chain.get()) {
                    out.printf(", ");
                    dumpChain(out, vm, structureList->list[i].u.chain.get(), ident);
                }
            } else {
                if (structureList->list[i].u.proto.get()) {
                    out.printf(", ");
                    dumpStructure(out, "proto", vm, structureList->list[i].u.proto.get(), ident);
                }
            }
            out.printf(")");
        }
        out.printf("]");
    }
}
#endif

void CodeBlock::printGetByIdCacheStatus(PrintStream& out, ExecState* exec, int location, const StubInfoMap& map)

        out.printf(" llint(array_length)");
    else if (Structure* structure = instruction[4].u.structure.get()) {
        out.printf(" llint(");
        dumpStructure(out, "struct", exec->vm(), structure, &ident);
        out.printf(")");
    }
#endif

        
        if (stubInfo.seen) {
            out.printf(" jit(");
            dumpStructureStubInfo(out, exec->vm(), stubInfo, &ident);
            Structure* baseStructure = 0;
            Structure* prototypeStructure = 0;
            StructureChain* chain = 0;
            PolymorphicAccessStructureList* structureList = 0;
            int listSize = 0;
            
            switch (stubInfo.accessType) {
            case access_get_by_id_self:
                out.printf("self");
                baseStructure = stubInfo.u.getByIdSelf.baseObjectStructure.get();
                break;
            case access_get_by_id_proto:
                out.printf("proto");
                baseStructure = stubInfo.u.getByIdProto.baseObjectStructure.get();
                prototypeStructure = stubInfo.u.getByIdProto.prototypeStructure.get();
                break;
            case access_get_by_id_chain:
                out.printf("chain");
                baseStructure = stubInfo.u.getByIdChain.baseObjectStructure.get();
                chain = stubInfo.u.getByIdChain.chain.get();
                break;
            case access_get_by_id_self_list:
                out.printf("self_list");
                structureList = stubInfo.u.getByIdSelfList.structureList;
                listSize = stubInfo.u.getByIdSelfList.listSize;
                break;
            case access_get_by_id_proto_list:
                out.printf("proto_list");
                structureList = stubInfo.u.getByIdProtoList.structureList;
                listSize = stubInfo.u.getByIdProtoList.listSize;
                break;
            case access_unset:
                out.printf("unset");
                break;
            case access_get_by_id_generic:
                out.printf("generic");
                break;
            case access_get_array_length:
                out.printf("array_length");
                break;
            case access_get_string_length:
                out.printf("string_length");
                break;
            default:
                RELEASE_ASSERT_NOT_REACHED();
                break;
            }
            
            if (baseStructure) {
                out.printf(", ");
                dumpStructure(out, "struct", exec, baseStructure, ident);
            }
            
            if (prototypeStructure) {
                out.printf(", ");
                dumpStructure(out, "prototypeStruct", exec, baseStructure, ident);
            }
            
            if (chain) {
                out.printf(", ");
                dumpChain(out, exec, chain, ident);
            }
            
            if (structureList) {
                out.printf(", list = %p: [", structureList);
                for (int i = 0; i < listSize; ++i) {
                    if (i)
                        out.printf(", ");
                    out.printf("(");
                    dumpStructure(out, "base", exec, structureList->list[i].base.get(), ident);
                    if (structureList->list[i].isChain) {
                        if (structureList->list[i].u.chain.get()) {
                            out.printf(", ");
                            dumpChain(out, exec, structureList->list[i].u.chain.get(), ident);
                        }
                    } else {
                        if (structureList->list[i].u.proto.get()) {
                            out.printf(", ");
                            dumpStructure(out, "proto", exec, structureList->list[i].u.proto.get(), ident);
                        }
                    }
                    out.printf(")");
                }
                out.printf("]");
            }
            out.printf(")");
        }
    }

#endif // ENABLE(DFG_JIT)
}

// Returns false if we want this reconsidered.
static void forceStructureLiveness(
    SlotVisitor& visitor, WriteBarrier<JSCell>& possibleStructure)
{
    JSCell* cell = possibleStructure.get();
    if (Heap::isMarked(cell))
        return;
    
    if (cell->classInfo() != Structure::info())
        return;
    
    Structure* structure = jsCast<Structure*>(cell);
    if (!Heap::isMarked(structure->globalObject()))
        return;
    
    visitor.append(&possibleStructure);
    return;
}

void CodeBlock::propagateTransitions(SlotVisitor& visitor)
{
    UNUSED_PARAM(visitor);

#if ENABLE(DFG_JIT)
    if (JITCode::isOptimizingJIT(jitType())) {
        DFG::CommonData* dfgCommon = m_jitCode->dfgCommon();
        
        // This does the following:
        // - Forces liveness of structures with live global objects
        // - Forces liveness of structures that are known to be transitioned-to,
        //   and where the sources of those transitions are live.
        
        for (unsigned i = 0; i < dfgCommon->weakReferences.size(); ++i)
            forceStructureLiveness(visitor, dfgCommon->weakReferences[i]);
        
        for (unsigned i = 0; i < dfgCommon->transitions.size(); ++i) {
            // FIXME: Consider getting rid of the transition fixpoint part of this.
            // https://bugs.webkit.org/show_bug.cgi?id=128078
            
            if ((!dfgCommon->transitions[i].m_codeOrigin
                 || Heap::isMarked(dfgCommon->transitions[i].m_codeOrigin.get()))
                && Heap::isMarked(dfgCommon->transitions[i].m_from.get())) {

                //   heap location holds the source, and if so, stores the target.
                //   Hence the source must be live for the transition to be live.
                visitor.append(&dfgCommon->transitions[i].m_to);
            }

            forceStructureLiveness(visitor, dfgCommon->transitions[i].m_from);
            forceStructureLiveness(visitor, dfgCommon->transitions[i].m_to);
            
            if (!Heap::isMarked(dfgCommon->transitions[i].m_to.get()))
                allAreMarkedSoFar = false;
        }
    }


void CodeBlock::finalizeUnconditionally()
{
    bool verbose = Options::verboseStructureClearingInGC() || Options::verboseOSR();
    
    Interpreter* interpreter = m_vm->interpreter;
    if (JITCode::couldBeInterpreted(jitType())) {
        const Vector<unsigned>& propertyAccessInstructions = m_unlinkedCode->propertyAccessInstructions();

            case op_put_by_id_out_of_line:
                if (!curInstruction[4].u.structure || Heap::isMarked(curInstruction[4].u.structure.get()))
                    break;
                if (verbose)
                    dataLogF("Clearing LLInt property access with structure %p.\n", curInstruction[4].u.structure.get());
                curInstruction[4].u.structure.clear();
                curInstruction[5].u.operand = 0;

                    && Heap::isMarked(curInstruction[6].u.structure.get())
                    && Heap::isMarked(curInstruction[7].u.structureChain.get()))
                    break;
                if (verbose) {
                    dataLogF("Clearing LLInt put transition with structures %p -> %p, chain %p.\n",
                            curInstruction[4].u.structure.get(),
                            curInstruction[6].u.structure.get(),

            case op_to_this:
                if (!curInstruction[2].u.structure || Heap::isMarked(curInstruction[2].u.structure.get()))
                    break;
                if (verbose)
                    dataLogF("Clearing LLInt to_this with structure %p.\n", curInstruction[2].u.structure.get());
                curInstruction[2].u.structure.clear();
                break;
            case op_get_callee:
                if (!curInstruction[2].u.jsCell || Heap::isMarked(curInstruction[2].u.jsCell.get()))
                    break;
                if (verbose)
                    dataLogF("Clearing LLInt get callee with function %p.\n", curInstruction[2].u.jsCell.get());
                curInstruction[2].u.jsCell.clear();
                break;

                WriteBarrierBase<JSActivation>& activation = curInstruction[5].u.activation;
                if (!activation || Heap::isMarked(activation.get()))
                    break;
                if (verbose)
                    dataLogF("Clearing dead activation %p.\n", activation.get());
                activation.clear();
                break;

                WriteBarrierBase<Structure>& structure = curInstruction[5].u.structure;
                if (!structure || Heap::isMarked(structure.get()))
                    break;
                if (verbose)
                    dataLogF("Clearing scope access with structure %p.\n", structure.get());
                structure.clear();
                break;

#if ENABLE(LLINT)
        for (unsigned i = 0; i < m_llintCallLinkInfos.size(); ++i) {
            if (m_llintCallLinkInfos[i].isLinked() && !Heap::isMarked(m_llintCallLinkInfos[i].callee.get())) {
                if (verbose)
                    dataLog("Clearing LLInt call from ", *this, "\n");
                m_llintCallLinkInfos[i].unlink();
            }

#if ENABLE(DFG_JIT)
    // Check if we're not live. If we are, then jettison.
    if (!(shouldImmediatelyAssumeLivenessDuringScan() || m_jitCode->dfgCommon()->livenessHasBeenProved)) {
        if (verbose)
            dataLog(*this, " has dead weak references, jettisoning during GC.\n");

        if (verbose || DFG::shouldShowDisassembly()) {
            dataLog(*this, " will be jettisoned because of the following dead references:\n");
            DFG::CommonData* dfgCommon = m_jitCode->dfgCommon();
            for (unsigned i = 0; i < dfgCommon->transitions.size(); ++i) {

                if (ClosureCallStubRoutine* stub = callLinkInfo(i).stub.get()) {
                    if (!Heap::isMarked(stub->structure())
                        || !Heap::isMarked(stub->executable())) {
                        if (verbose) {
                            dataLog(
                                "Clearing closure call from ", *this, " to ",
                                stub->executable()->hashFor(callLinkInfo(i).specializationKind()),

                        callLinkInfo(i).unlink(*m_vm, repatchBuffer);
                    }
                } else if (!Heap::isMarked(callLinkInfo(i).callee.get())) {
                    if (verbose) {
                        dataLog(
                            "Clearing call from ", *this, " to ",
                            RawPointer(callLinkInfo(i).callee.get()), " (",

                }
            }
            if (!!callLinkInfo(i).lastSeenCallee
                && !Heap::isMarked(callLinkInfo(i).lastSeenCallee.get())) {
                if (verbose)
                    dataLog("Clearing lastSeenCallee from ", *this, " to ", RawPointer(callLinkInfo(i).lastSeenCallee.get()), "\n");
                callLinkInfo(i).lastSeenCallee.clear();
            }
        }
        for (Bag<StructureStubInfo>::iterator iter = m_stubInfos.begin(); !!iter; ++iter) {
            StructureStubInfo& stubInfo = **iter;

            if (stubInfo.visitWeakReferences())
                continue;
            
            if (verbose) {
                dataLog("Clearing stubInfo from ", *this, " at ", stubInfo.codeOrigin, " with state: ");
                dumpStructureStubInfo(WTF::dataFile(), *vm(), stubInfo, 0);
                dataLog("\n");
            }
            
            resetStubDuringGCInternal(repatchBuffer, stubInfo);
        }
    }

void CodeBlock::resetStubDuringGCInternal(RepatchBuffer& repatchBuffer, StructureStubInfo& stubInfo)
{
    resetStubInternal(repatchBuffer, stubInfo);
    
    // FIXME: Consider getting rid of this flag; it might not be needed anymore.
    // https://bugs.webkit.org/show_bug.cgi?id=128079
    stubInfo.resetByGC = true;
}
#endif

Lines 132-137 typedef OptionRange optionRange; Source/JavaScriptCore/runtime/Options.h_sec1
- Source/JavaScriptCore/runtime/Options.h +1 lines
132	v(bool, alwaysComputeHash, false) \	132	v(bool, alwaysComputeHash, false) \
133	v(bool, testTheFTL, false) \	133	v(bool, testTheFTL, false) \
134	v(bool, verboseSanitizeStack, false) \	134	v(bool, verboseSanitizeStack, false) \
		135	v(bool, verboseStructureClearingInGC, false) \
135	\	136	\
136	v(bool, enableOSREntryToDFG, true) \	137	v(bool, enableOSREntryToDFG, true) \
137	v(bool, enableOSREntryToFTL, true) \	138	v(bool, enableOSREntryToFTL, true) \

Return to Bug 128072

Lines 1-3 Source/JavaScriptCore/ChangeLog_sec1
- Source/JavaScriptCore/ChangeLog +74 lines
		1	2014-02-02 Filip Pizlo <fpizlo@apple.com>
		2
		3	GC should keep structures alive if they are inlined into optimized code (DFG or FTL) and their globalObject is alive
		4	https://bugs.webkit.org/show_bug.cgi?id=128072
		5
		6	Reviewed by NOBODY (OOPS!).
		7
		8	The original role of the structure transition fixpoint was to guard against cases
		9	where we had code that cached on a now-dead structure with a now-dead global object.
		10	Originally cached structures were strong references and so such a case could lead to
		11	a massive memory leak.
		12
		13	Of course, we couldn't turn these references into simply weak references because then
		14	we would be throwing away code too frequently. It's surprisingly common to have
		15	idioms that reduce to something like:
		16
		17	for (forever) {
		18	var o = new SuperImportantStateObject();
		19	// Do a bunch of work involving o.
		20	o = null; // Something that kills o.
		21	// Do some more work, and we GC here.
		22	}
		23
		24	Usually the "o = null" arises because the allocation of the object is inside of a
		25	function that the loop calls. This is particularly frequent on the Web because that
		26	outer loop is actually the runloop and the GC is typically scheduled when the runloop
		27	is idle. So, if we treated all cached structures as weak and the app had any types
		28	that existed only during event handling but typically didn't remain reachable after
		29	the event got handled, then we would be throwing away a lot of code.
		30
		31	So, instead of making those structures completely weak, we threw in a clever trick:
		32	we would fixpoint over all cached transitions that create new structures, and if any
		33	of those transitions was a part of code that appeared live, then we would mark the
		34	target structure. This tended to keep alive structures for objects that hot code
		35	would tend to create.
		36
		37	But now it looks like this isn't enough. Those transition caches are just that -
		38	caches. They are imperfect. So, this patch adds another rule for keeping structures
		39	alive:
		40
		41	If a structure is cached by an optimizing compiler and its global object is
		42	otherwise still live, then mark the structure.
		43
		44	This rule will usually dominate the transition rule, but the transition rule is still
		45	critical for when we haven't created optimized code yet or for structures that aren't
		46	associated with a global object.
		47
		48	This fix introduces two possibilities for simplifying the code, but they probably
		49	require some more investigation:
		50
		51	- https://bugs.webkit.org/show_bug.cgi?id=128078
		52	Consider getting rid of the GC transition fixpoint for optimized code, since the
		53	global object rule is likely to dominate it
		54
		55	- https://bugs.webkit.org/show_bug.cgi?id=128079
		56	Consider getting rid of the StructureStubInfo::resetByGC flag because the global
		57	object rule implies that caching on possibly-GCable structures is no longer
		58	disastrous
		59
		60	This looks like a 50% speed-up on Octane2/gbemu in the FTL.
		61
		62	This also includes a bunch of debug support for tracking what is going on inside of
		63	the GC's structure clearing. That's actually the bulk of this patch.
		64
65	* bytecode/CodeBlock.cpp:
66	(JSC::dumpStructure):
67	(JSC::dumpChain):
68	(JSC::dumpStructureStubInfo):
69	(JSC::CodeBlock::printGetByIdCacheStatus):
70	(JSC::forceStructureLiveness):
71	(JSC::CodeBlock::propagateTransitions):
72	(JSC::CodeBlock::finalizeUnconditionally):
73	* runtime/Options.h:
74
1	2014-02-01 Filip Pizlo <fpizlo@apple.com>	75	2014-02-01 Filip Pizlo <fpizlo@apple.com>
2		76
3	JSC profiler's stub info profiling support should work again	77	JSC profiler's stub info profiling support should work again