[webkit-reviews] review granted: [Bug 175144] Use JIT probes for DFG OSR exit. : [Attachment 320151] proposed patch (removed some debugging code).

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 175144: Use JIT probes for DFG OSR exit.
https://bugs.webkit.org/show_bug.cgi?id=175144

Attachment 320151: proposed patch (removed some debugging code).

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




--- Comment #35 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 320151
  --> https://bugs.webkit.org/attachment.cgi?id=320151
proposed patch (removed some debugging code).

View in context: https://bugs.webkit.org/attachment.cgi?id=320151&action=review

r=me with comments

> Source/JavaScriptCore/assembler/ProbeFrame.h:44
> +    T getArgument(int argument)

style nit: here and below, you preface getters with get, but I don't think we
do that elsewhere, e.g, Operands. What about just calling this:
argument(...), and below, operand(...), etc.

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:80
> +    RegisterSet dontRestoreRegisters =
RegisterSet(RegisterSet::stackRegisters(), RegisterSet::allFPRs());

style nit: This same thing is used here and below, might be worth making it a
function to call.

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:135
> +	       context.gpr(entry.reg().gpr()) =
stack.get<uintptr_t>(calleeSaveBuffer, entry.offset());
> +	   else
> +	       context.gpr(entry.reg().gpr()) =
stack.get<double>(calleeSaveBuffer, entry.offset());

why do these gets need to use the stack API?

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:152
> +    ASSERT(is64Bit());

seems like this could be a static assert?

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:160
> +	   if (entry.reg().isGPR())
> +	       stack.set(calleeSaveBuffer, entry.offset(),
context.gpr<uintptr_t>(entry.reg().gpr()));
> +	   else
> +	       stack.set(calleeSaveBuffer, entry.offset(),
context.fpr<uintptr_t>(entry.reg().fpr()));

I think it'd be nice to have context expose a register API so it does this
branch for you. You also write the branch in a few places, but if context
handled it, you could just do:
stack.set(calleeSaveBuffer, entry.offset(),
context.reg<uintptr_t>(entry.reg()));

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:171
> +    RegisterSet dontSaveRegisters =
RegisterSet(RegisterSet::stackRegisters(), RegisterSet::allFPRs());

ditto about method abstracting this instead of copy-pasting.

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:176
> +#if USE(JSVALUE64)
> +    RegisterSet baselineCalleeSaves =
RegisterSet::llintBaselineCalleeSaveRegisters();
> +#endif

seems like this is guaranteed given NUMBER_OF_CALLEE_SAVES_REGISTERS.

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:187
> +#if USE(JSVALUE32_64)
> +	   UNUSED_PARAM(wasCalledViaTailCall);
> +#else

seems like this is dead given: NUMBER_OF_CALLEE_SAVES_REGISTERS

Maybe inside your:
#if NUMBER_OF_CALLEE_SAVES_REGISTERS > 0

you can add: static_assert(is64bit(), "")

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:326
> +void OSRExit::executeOSRExit(Context& context)

Should you set top call frame inside of here? I think that'd help the sampling
profiler at least. Maybe other things need it to.

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:353
> +    if (UNLIKELY(!exit.exitState)) {

suggestion: Let's cache the various calls of JITCode::dfg() here since it's a
virtual function

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:366
> +	   // Compute the value recoveries.
> +	   Operands<ValueRecovery> operands;
> +	  
codeBlock->jitCode()->dfg()->variableEventStream.reconstruct(codeBlock,
exit.m_codeOrigin, codeBlock->jitCode()->dfg()->minifiedDFG,
exit.m_streamIndex, operands);

This is an interesting tradeoff. I think it's the right one. However, I know we
use the event stream to reconstruct on demand because storing the reconstructed
data for each exit has a high memory cost. That said, we're only doing this
here for *executed* exits. That's why I think this is ok and the right choice.
But it's worth making sure we truly believe this is the best choice.

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:489
> +		   Structure* structure = jsValueFor(cpu,
exit.m_jsValueSource).asCell()->structure(vm);

how do we know the incoming value is a cell?

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:745
> +	   saveOrCopyCalleeSavesFor(context, baselineCodeBlock,
static_cast<VirtualRegister>(inlineCallFrame->stackOffset), !trueCaller);

Style: This static cast here seems weird, why not just:
VirtualRegister(inlineCallFrame->stackOffset) ?