Bug 202503 – Throw away baseline code if there is an optimized replacement

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

...

[Saam Barati]

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.

[Saam Barati]

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

[Saam Barati]

Created attachment 380382 [details]
WIP

[Saam Barati]

perf seems neutral on Speedo2 and JS2

[Saam Barati]

Created attachment 380710 [details]
WIP

[Saam Barati]

Created attachment 387411 [details]
patch

[Saam Barati]

Created attachment 387554 [details]
patch

[Robin Morisset]

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.

[Yusuke Suzuki]

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?

[Saam Barati]

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.

[Saam Barati]

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

[Saam Barati]

Created attachment 387591 [details]
patch for landing

[Saam Barati]

landed in:
https://trac.webkit.org/changeset/254480/webkit

[Radar WebKit Bug Importer]

<rdar://problem/58552041>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 206278

[Saam Barati]

Created attachment 388382 [details]
patch

[Saam Barati]

Created attachment 388386 [details]
patch

[Yusuke Suzuki]

Comment on attachment 388386 [details]
patch

r=me

[WebKit Commit Bot]

Comment on attachment 388386 [details]
patch

Clearing flags on attachment: 388386

Committed r254926: <https://trac.webkit.org/changeset/254926>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.