[webkit-reviews] review granted: [Bug 202503] Throw away baseline code if there is an optimized replacement : [Attachment 387554] patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jan 13 14:57:13 PST 2020


Yusuke Suzuki <ysuzuki at apple.com> has granted Saam Barati <sbarati at apple.com>'s
request for review:
Bug 202503: Throw away baseline code if there is an optimized replacement
https://bugs.webkit.org/show_bug.cgi?id=202503

Attachment 387554: patch

https://bugs.webkit.org/attachment.cgi?id=387554&action=review




--- Comment #9 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 387554
  --> https://bugs.webkit.org/attachment.cgi?id=387554
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?


More information about the webkit-reviews mailing list