[webkit-reviews] review denied: [Bug 93884] Change behavior of MasqueradesAsUndefined to better accommodate DFG changes : [Attachment 158088] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 93884: Change behavior of MasqueradesAsUndefined to better accommodate DFG
changes
https://bugs.webkit.org/show_bug.cgi?id=93884

Attachment 158088: Patch
https://bugs.webkit.org/attachment.cgi?id=158088&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=158088&action=review


r- because the builders are angry.

Please check my performance comment below, as well.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:645
> +    JITCompiler::Jump isMasqueradesAsUndefined =
m_jit.branchTest8(JITCompiler::NonZero, JITCompiler::Address(resultPayloadGPR,
Structure::typeInfoFlagsOffset()),
JITCompiler::TrustedImm32(MasqueradesAsUndefined));
>      
> +    if (invert)
> +	   m_jit.move(TrustedImm32(0), resultPayloadGPR);
> +    else
> +	   m_jit.move(TrustedImm32(1), resultPayloadGPR);
> +
> +    JITCompiler::Jump notMasqueradesAsUndefined = m_jit.jump();
> +
> +    isMasqueradesAsUndefined.link(&m_jit);
> +    GPRTemporary localGlobalObject(this);
> +    GPRTemporary remoteGlobalObject(this);
> +    GPRReg localGlobalObjectGPR = localGlobalObject.gpr();
> +    GPRReg remoteGlobalObjectGPR = remoteGlobalObject.gpr();
> +   
m_jit.move(JITCompiler::TrustedImmPtr(m_jit.graph().globalObjectFor(m_jit.graph
()[operand].codeOrigin)), localGlobalObjectGPR);
> +    m_jit.loadPtr(JITCompiler::Address(resultPayloadGPR,
Structure::globalObjectOffset()), remoteGlobalObjectGPR);
> +    m_jit.compare32(invert ? JITCompiler::NotEqual : JITCompiler::Equal,
localGlobalObjectGPR, remoteGlobalObjectGPR, resultPayloadGPR);
> + 

This looks like it would be a performance regression. Do you have benchmark
results for this patch?

I thought our plan was:
(a) Don't test masqueradesAsUndefined at all if you global object has not
produced document.all
(b) Watchpoint equality comparisons, and fire the watchpoint if your global
object does produce document.all

I believe that plan would be a speedup over what we have now.

> Source/JavaScriptCore/runtime/Structure.h:513
> +    inline bool Structure::isMasqueradesAsUndefined(JSGlobalObject*
lexicalGlobalObject)
> +    {
> +	   return typeInfo().masqueradesAsUndefined() && globalObject() ==
lexicalGlobalObject;
> +    }

I think just plain "masqueradesAsUndefined" would be a better name here.