| Summary: | [JSC] Reduce sizeof(BaselineCallLinkInfo) to make bug 238535 good | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||
| Component: | New Bugs | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Yusuke Suzuki
2022-04-04 16:27:02 PDT
Created attachment 456647 [details]
Patch
Created attachment 456648 [details]
Patch
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? 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. 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. Committed r292373 (249238@trunk): <https://commits.webkit.org/249238@trunk> |