...
Some things to look out for: - We could do this severing at GC finalization time (or on every Nth GC, or some other heuristic piggy backing on GC timing). - If it's on the stack, we can't throw it away, at least not naively. We could make a GC managed stub. But it might be simplest to just skip tossing it away if it's on the stack and wait until next GC cycle. - If an OSR exit has been wired to jump to baseline code, we can't throw it away, at least not naively. We could toss the OSR exit ramp code and rebuild it. Alternatively, we could just skip throwing away any such baseline code. It might be a good heuristic anyways to keep baseline code that's been existed to.
(In reply to Saam Barati from comment #1) > Some things to look out for: > - We could do this severing at GC finalization time (or on every Nth GC, or > some other heuristic piggy backing on GC timing). > - If it's on the stack, we can't throw it away, at least not naively. We > could make a GC managed stub. But it might be simplest to just skip tossing > it away if it's on the stack and wait until next GC cycle. > - If an OSR exit has been wired to jump to baseline code, we can't throw it > away, at least not naively. We could toss the OSR exit ramp code and rebuild > it. Alternatively, we could just skip throwing away any such baseline code. > It might be a good heuristic anyways to keep baseline code that's been > existed to. - We also need to unlink any incoming calls.
Created attachment 380382 [details] WIP
perf seems neutral on Speedo2 and JS2
Created attachment 380710 [details] WIP
Created attachment 387411 [details] patch
Created attachment 387554 [details] patch
Comment on attachment 387411 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=387411&action=review This patch looks reasonable to me, but I have no experience modifying the OSR code, so don't feel comfortable r+-ing it without someone else looking. > Source/JavaScriptCore/bytecode/CodeBlock.cpp:1396 > + if (size_t count = m_unlinkedCode->numberOfExceptionHandlers()) { This "if" is not required, I think you can just do count = ...; for (...) {...} > Source/JavaScriptCore/bytecode/CodeBlock.cpp:1620 > jitData->m_rareCaseProfiles.clear(); How is the choice of which fields of m_jitData to clear made? > Source/JavaScriptCore/bytecode/CodeBlock.h:263 > + // guaranteed we haven't finalized a GC between calling this and its last use. I think I kind of get what you mean, but the phrasing is weird. In particular, the restriction is less on when you can call the function, and more on when you can last use its result, right? > Source/JavaScriptCore/heap/Heap.cpp:1523 > + finalizeUnconditionalFinalizers(); // We rely on this before running clearCurrentlyExecuting since CodeBlock's finalizer relies on querying currently executing. "before running" -> "running before", at least if I understand things correctly.
Comment on attachment 387554 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=387554&action=review r=me > Source/JavaScriptCore/ChangeLog:12 > + This patch's goal is to help us save JIT executable memory by throwing > + away baseline code when it has an optimized replacement. To make it > + easy to reason about, we do this when finalizing a GC, and when the > + CodeBlock is not on the stack. When we do this, we throw away all JIT > + data and unlink all incoming calls. Nice. > Source/JavaScriptCore/bytecode/CodeBlock.cpp:1389 > + if (!Options::forceBaseline() && jitType() == JITType::BaselineJIT && !m_vm->heap.codeBlockSet().isCurrentlyExecuting(this)) { Adding some comment about what it is doing would be nice, something like, // If BaselineJIT code is not executed and optimized replacement exists, we attempt to make BaselineJIT CodeBlock back to LLInt CodeBlock to purge unused JIT code. > Source/JavaScriptCore/bytecode/CodeBlock.cpp:1390 > + if (CodeBlock* optimizedCodeBlock = optimizedReplacement()) { Is it safe to call this in CodeBlock::finalizeUnconditionally while ExecutableToCodeBlockEdge::finalizeUnconditionally is not called yet? Should we replace the order of finalizeUnconditionally between CodeBlock and ExecutableToCodeBlockEdge? > Source/JavaScriptCore/bytecode/CodeBlock.cpp:1391 > + if (!optimizedCodeBlock->m_osrExitCounter) { This is heuristic like, "If DFG / FTL OSR exit happens, we could recompile it so we keep Baseline JIT code", right? > Source/JavaScriptCore/bytecode/CodeBlock.cpp:1622 > + m_jitData = nullptr; Nice. We can now purge JITData for CodeBlock by changing BaselineJIT -> LLInt. > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:-2005 > - auto nextBytecode = codeBlock->jitCodeMap().find(nextBytecodeIndex); I wonder if we can remove jitCodeMap completely, or shrink size of it by only recording some part of it (only recording op_loop_hint place). Can you file a bug and put FIXME to JITCodeMap.h?
Comment on attachment 387411 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=387411&action=review >> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1396 >> + if (size_t count = m_unlinkedCode->numberOfExceptionHandlers()) { > > This "if" is not required, I think you can just do count = ...; for (...) {...} will fix. >> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1620 >> jitData->m_rareCaseProfiles.clear(); > > How is the choice of which fields of m_jitData to clear made? good call. I don't think this is needed anymore now that I'm deleting m_jitData below. I can remove the clears here. Unfortunately walking the StructureStubInfo is still needed above... >> Source/JavaScriptCore/bytecode/CodeBlock.h:263 >> + // guaranteed we haven't finalized a GC between calling this and its last use. > > I think I kind of get what you mean, but the phrasing is weird. > In particular, the restriction is less on when you can call the function, and more on when you can last use its result, right? Right, it's the result. I'll try to make it more clear.
Comment on attachment 387554 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=387554&action=review >> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1389 >> + if (!Options::forceBaseline() && jitType() == JITType::BaselineJIT && !m_vm->heap.codeBlockSet().isCurrentlyExecuting(this)) { > > Adding some comment about what it is doing would be nice, something like, > > // If BaselineJIT code is not executed and optimized replacement exists, we attempt to make BaselineJIT CodeBlock back to LLInt CodeBlock to purge unused JIT code. will do >> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1390 >> + if (CodeBlock* optimizedCodeBlock = optimizedReplacement()) { > > Is it safe to call this in CodeBlock::finalizeUnconditionally while ExecutableToCodeBlockEdge::finalizeUnconditionally is not called yet? > Should we replace the order of finalizeUnconditionally between CodeBlock and ExecutableToCodeBlockEdge? I've swapped the order to prevent the pathological perf issue that might happen >> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1391 >> + if (!optimizedCodeBlock->m_osrExitCounter) { > > This is heuristic like, "If DFG / FTL OSR exit happens, we could recompile it so we keep Baseline JIT code", right? yeah, exactly >> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1622 >> + m_jitData = nullptr; > > Nice. We can now purge JITData for CodeBlock by changing BaselineJIT -> LLInt. Yeah. I've also removed a bunch of the clears() above since it's redundant with running the destructor for JITData >> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:-2005 >> - auto nextBytecode = codeBlock->jitCodeMap().find(nextBytecodeIndex); > > I wonder if we can remove jitCodeMap completely, or shrink size of it by only recording some part of it (only recording op_loop_hint place). Can you file a bug and put FIXME to JITCodeMap.h? Good point. I'll add a FIXME
Created attachment 387591 [details] patch for landing
landed in: https://trac.webkit.org/changeset/254480/webkit
<rdar://problem/58552041>
Re-opened since this is blocked by bug 206278
Created attachment 388382 [details] patch
Created attachment 388386 [details] patch
Comment on attachment 388386 [details] patch r=me
Comment on attachment 388386 [details] patch Clearing flags on attachment: 388386 Committed r254926: <https://trac.webkit.org/changeset/254926>
All reviewed patches have been landed. Closing bug.