Bug 202503

Summary: Throw away baseline code if there is an optimized replacement
Product: WebKit Reporter: Saam Barati <saam>
Component: JavaScriptCoreAssignee: Saam Barati <saam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, fpizlo, ggaren, gskachkov, guijemont, keith_miller, mark.lam, msaboff, rmorisset, ticaiolima, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=206244
Bug Depends on: 197993, 206278    
Bug Blocks: 206094    
Attachments:
Description Flags
WIP
none
WIP
none
patch
none
patch
ysuzuki: review+
patch for landing
none
patch
none
patch none

Description Saam Barati 2019-10-02 16:57:23 PDT
...
Comment 1 Saam Barati 2019-10-02 18:08:39 PDT
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.
Comment 2 Saam Barati 2019-10-02 18:09:21 PDT
(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.
Comment 3 Saam Barati 2019-10-07 18:43:40 PDT
Created attachment 380382 [details]
WIP
Comment 4 Saam Barati 2019-10-08 19:57:33 PDT
perf seems neutral on Speedo2 and JS2
Comment 5 Saam Barati 2019-10-10 18:17:26 PDT
Created attachment 380710 [details]
WIP
Comment 6 Saam Barati 2020-01-10 18:15:32 PST
Created attachment 387411 [details]
patch
Comment 7 Saam Barati 2020-01-13 12:05:17 PST
Created attachment 387554 [details]
patch
Comment 8 Robin Morisset 2020-01-13 13:41:17 PST
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 9 Yusuke Suzuki 2020-01-13 14:57:13 PST
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 10 Saam Barati 2020-01-13 16:17:46 PST
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 11 Saam Barati 2020-01-13 16:27:08 PST
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
Comment 12 Saam Barati 2020-01-13 16:39:57 PST
Created attachment 387591 [details]
patch for landing
Comment 13 Saam Barati 2020-01-13 17:44:37 PST
landed in:
https://trac.webkit.org/changeset/254480/webkit
Comment 14 Radar WebKit Bug Importer 2020-01-13 17:45:21 PST
<rdar://problem/58552041>
Comment 15 WebKit Commit Bot 2020-01-14 21:59:18 PST
Re-opened since this is blocked by bug 206278
Comment 16 Saam Barati 2020-01-21 17:29:35 PST
Created attachment 388382 [details]
patch
Comment 17 Saam Barati 2020-01-21 18:15:52 PST
Created attachment 388386 [details]
patch
Comment 18 Yusuke Suzuki 2020-01-21 19:17:58 PST
Comment on attachment 388386 [details]
patch

r=me
Comment 19 WebKit Commit Bot 2020-01-22 09:06:37 PST
Comment on attachment 388386 [details]
patch

Clearing flags on attachment: 388386

Committed r254926: <https://trac.webkit.org/changeset/254926>
Comment 20 WebKit Commit Bot 2020-01-22 09:06:39 PST
All reviewed patches have been landed.  Closing bug.