Bug 233985 – [JSC] Introduce BaselineCallLinkInfo and OptimizingCallLinkInfo to shrink sizeof(BaselineCallLinkInfo)

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Introduce BaselineCallLinkInfo and OptimizingCallLinkInfo to shrink sizeof(BaselineCallLinkInfo)

[Yusuke Suzuki]

Created attachment 446311 [details]
Patch

[Yusuke Suzuki]

Created attachment 446314 [details]
Patch

[Yusuke Suzuki]

Created attachment 446326 [details]
Patch

[Mark Lam]

Comment on attachment 446326 [details]
Patch

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

Nice.  I like that the BaselineCallLinkInfo is now distinct from the OptimizingCallLinkInfo.  It makes the code so much easier to parse.

r=me with suggestions and questions.

> Source/JavaScriptCore/bytecode/CallLinkInfo.cpp:335
> +CCallHelpers::JumpList OptimizingCallLinkInfo::emitFastPath(CCallHelpers& jit, GPRReg calleeGPR, GPRReg callLinkInfoGPR, UseDataIC useDataIC)

Should put these OptimizingCallLinkInfo functions under a #if ENABLE(DFG_JIT) (or #if ENABLE(JIT) depending on what you decided to guard it under).  Otherwise, non-JIT or non-optimizing JIT builds will not be happy.

It's great for review to keep these functions in place like this with only minimal edits, but perhaps before you land, you should just group them together at the bottom of the file and just guard them with #if ENABLE(DFG_JIT) and do the same for the whole OptimizingCallLinkInfo class declaration in the header file.

> Source/JavaScriptCore/bytecode/CallLinkInfo.cpp:349
> +MacroAssembler::JumpList OptimizingCallLinkInfo::emitTailCallFastPath(CCallHelpers& jit, GPRReg calleeGPR, ScopedLambda<void()>&& prepareForTailCall)

Ditto.

> Source/JavaScriptCore/bytecode/CallLinkInfo.h:446
>  #if ENABLE(JIT)
>      GPRReg m_calleeGPR { InvalidGPRReg };
> +    GPRReg m_callLinkInfoGPR { InvalidGPRReg };
>  #endif

I think these can be moved to the OptimizingCallLinkInfo now.

Out of curiosity, why is m_callLinkInfoGPR (and its accessor method) needed?  I only see it being set (in existing code, and in your patch), but I don't see anyone using the value, except for a RELEASE_ASSERT in setCallLinkInfoGPR().  Is this really needed?  If not, then its getter & setter can. be removed as well.

> Source/JavaScriptCore/bytecode/CallLinkInfo.h:466
> +    GPRReg calleeGPR() const { return BaselineCallRegisters::calleeGPR; }
> +    GPRReg callLinkInfoGPR() const { return BaselineCallRegisters::callLinkInfoGPR; }

Can these be made constexpr static instead?

> Source/JavaScriptCore/bytecode/CallLinkInfo.h:467
> +    void setCallLinkInfoGPR(GPRReg callLinkInfoGPR) { RELEASE_ASSERT(callLinkInfoGPR == this->callLinkInfoGPR()); }

Can use `BaselineCallRegisters::callLinkInfoGPR` instead of `this->callLinkInfoGPR()`.

> Source/JavaScriptCore/bytecode/CallLinkInfo.h:480
> +#if ENABLE(JIT)

Why not put the whole OptimizingCallLinkInfo class under #if ENABLE(DFG_JIT)?

> Source/JavaScriptCore/dfg/DFGOperations.h:40
>  class JSPropertyNameEnumerator;

Not due to your change, but would you mind moving this up above to keep the list alphabetically sorted?

[Yusuke Suzuki]

Comment on attachment 446326 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/bytecode/CallLinkInfo.cpp:335
>> +CCallHelpers::JumpList OptimizingCallLinkInfo::emitFastPath(CCallHelpers& jit, GPRReg calleeGPR, GPRReg callLinkInfoGPR, UseDataIC useDataIC)
> 
> Should put these OptimizingCallLinkInfo functions under a #if ENABLE(DFG_JIT) (or #if ENABLE(JIT) depending on what you decided to guard it under).  Otherwise, non-JIT or non-optimizing JIT builds will not be happy.
> 
> It's great for review to keep these functions in place like this with only minimal edits, but perhaps before you land, you should just group them together at the bottom of the file and just guard them with #if ENABLE(DFG_JIT) and do the same for the whole OptimizingCallLinkInfo class declaration in the header file.

Non Baseline CallLinkInfos are now OptimizingCallLinkInfo. So, for example, IC is using that.
SO, we cannot use `ENABLE(DFG_JIT)`. But probably, we can use ENABLE(JIT).

>> Source/JavaScriptCore/bytecode/CallLinkInfo.h:446
>>  #endif
> 
> I think these can be moved to the OptimizingCallLinkInfo now.
> 
> Out of curiosity, why is m_callLinkInfoGPR (and its accessor method) needed?  I only see it being set (in existing code, and in your patch), but I don't see anyone using the value, except for a RELEASE_ASSERT in setCallLinkInfoGPR().  Is this really needed?  If not, then its getter & setter can. be removed as well.

It is right, but due to alignment / padding issue, if we move them to OptimizingCallLinkInfo, then rather it increases sizeof(OptimizingCallLinkInfo).
Since putting them in CallLinkInfo does not increase the size of CallLinkInfo (due to padding / alignment), I think putting them here is better due to memory saving.

I think callLinkInfoGPR() is not used right now, but probably, it is possible that we will use it in Repatch's linkPolymorphicCall in near future.

>> Source/JavaScriptCore/bytecode/CallLinkInfo.h:466
>> +    GPRReg callLinkInfoGPR() const { return BaselineCallRegisters::callLinkInfoGPR; }
> 
> Can these be made constexpr static instead?

Changed.

>> Source/JavaScriptCore/bytecode/CallLinkInfo.h:467
>> +    void setCallLinkInfoGPR(GPRReg callLinkInfoGPR) { RELEASE_ASSERT(callLinkInfoGPR == this->callLinkInfoGPR()); }
> 
> Can use `BaselineCallRegisters::callLinkInfoGPR` instead of `this->callLinkInfoGPR()`.

Changed.

>> Source/JavaScriptCore/bytecode/CallLinkInfo.h:480
>> +#if ENABLE(JIT)
> 
> Why not put the whole OptimizingCallLinkInfo class under #if ENABLE(DFG_JIT)?

It is because it is also used by IC, wasm etc.

>> Source/JavaScriptCore/dfg/DFGOperations.h:40
>>  class JSPropertyNameEnumerator;
> 
> Not due to your change, but would you mind moving this up above to keep the list alphabetically sorted?

Changed :)

[Yusuke Suzuki]

Committed r286769 (?): <https://commits.webkit.org/r286769>

[Radar WebKit Bug Importer]

<rdar://problem/86258681>