Bug 238777 – [JSC] Reduce sizeof(BaselineCallLinkInfo) to make bug 238535 good

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

[Yusuke Suzuki]

[JSC] Reduce sizeof(BaselineCallLinkInfo) to make bug 238535 good

[Yusuke Suzuki]

Created attachment 456647 [details]
Patch

[Yusuke Suzuki]

Created attachment 456648 [details]
Patch

[Mark Lam]

Comment on attachment 456648 [details]
Patch

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

> Source/JavaScriptCore/bytecode/Repatch.cpp:1735
> +            CallFrameShuffleData shuffleData = CallFrameShuffleData::createForBaselineOrLLIntTailCall(bytecode, callerCodeBlock->numParameters());

According to my local dump-class-layout of CallFrameShuffleData, it is 616 bytes in size (not including the backing store for the Vector in it).  Here, we're returning by value, which may not be as big an issue due to forwarding.  However, see below ...

> Source/JavaScriptCore/bytecode/Repatch.cpp:1744
> +            frameShuffler.emplace(stubJit, *optimizingCallLinkInfo.frameShuffleData());

Here, we're copying the CallFrameShuffleData, right?  Can we change the idiom such that we use:

    CallFrameShuffler localFrameShuffler;
    std::optional<CallFrameShuffler*> frameShuffler;

... and in the baseline case, pass the &localFrameShuffler to an init function (factored out of createForBaselineOrLLIntTailCall()) instead of calling createForBaselineOrLLIntTailCall() directly?

[Yusuke Suzuki]

Comment on attachment 456648 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/Repatch.cpp:1744
>> +            frameShuffler.emplace(stubJit, *optimizingCallLinkInfo.frameShuffleData());
> 
> Here, we're copying the CallFrameShuffleData, right?  Can we change the idiom such that we use:
> 
>     CallFrameShuffler localFrameShuffler;
>     std::optional<CallFrameShuffler*> frameShuffler;
> 
> ... and in the baseline case, pass the &localFrameShuffler to an init function (factored out of createForBaselineOrLLIntTailCall()) instead of calling createForBaselineOrLLIntTailCall() directly?

Ah, no. CallFrameShuffler just constructs itself via `const CallFrameShuffleData&`, and does not copy it. So it is just referencing the value.

[Mark Lam]

Comment on attachment 456648 [details]
Patch

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

r=me

>>> Source/JavaScriptCore/bytecode/Repatch.cpp:1744
>>> +            frameShuffler.emplace(stubJit, *optimizingCallLinkInfo.frameShuffleData());
>> 
>> Here, we're copying the CallFrameShuffleData, right?  Can we change the idiom such that we use:
>> 
>>     CallFrameShuffler localFrameShuffler;
>>     std::optional<CallFrameShuffler*> frameShuffler;
>> 
>> ... and in the baseline case, pass the &localFrameShuffler to an init function (factored out of createForBaselineOrLLIntTailCall()) instead of calling createForBaselineOrLLIntTailCall() directly?
> 
> Ah, no. CallFrameShuffler just constructs itself via `const CallFrameShuffleData&`, and does not copy it. So it is just referencing the value.

Sorry.  I conflated CallFrameShuffleData with CallFrameShuffler.  You are correct that there's no unnecessary copying here.

[Yusuke Suzuki]

Committed r292373 (249238@trunk): <https://commits.webkit.org/249238@trunk>

[Radar WebKit Bug Importer]

<rdar://problem/91276232>