Bug 238535

Summary: [JSC] Store CodeBlock in caller side
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, mcatanzaro, msaboff, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=238956
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
saam: review+
Patch
none
Patch none

Description Yusuke Suzuki 2022-03-29 20:12:43 PDT 
Let's not store it in the callee side since the caller typically already knows well about it.
It makes unlinked code simpler.
Comment 1 Yusuke Suzuki 2022-03-29 20:21:36 PDT 
Created attachment 456088 [details]
Patch
Comment 2 Yusuke Suzuki 2022-03-30 02:14:29 PDT 
Created attachment 456103 [details]
Patch
Comment 3 Yusuke Suzuki 2022-03-30 03:47:21 PDT 
Created attachment 456110 [details]
Patch
Comment 4 Yusuke Suzuki 2022-03-30 11:29:01 PDT 
Created attachment 456157 [details]
Patch
Comment 5 Yusuke Suzuki 2022-03-30 14:44:48 PDT 
Created attachment 456176 [details]
Patch
Comment 6 Yusuke Suzuki 2022-03-30 19:03:52 PDT 
Created attachment 456193 [details]
Patch
Comment 7 Yusuke Suzuki 2022-03-30 21:14:26 PDT 
Created attachment 456199 [details]
Patch
Comment 8 Yusuke Suzuki 2022-03-31 18:29:43 PDT 
Created attachment 456303 [details]
Patch
Comment 9 Yusuke Suzuki 2022-03-31 19:02:13 PDT 
Created attachment 456307 [details]
Patch
Comment 10 Yusuke Suzuki 2022-03-31 20:39:23 PDT 
Created attachment 456317 [details]
Patch
Comment 11 Yusuke Suzuki 2022-03-31 23:27:24 PDT 
Created attachment 456325 [details]
Patch
Comment 12 Yusuke Suzuki 2022-04-01 13:24:14 PDT 
Created attachment 456395 [details]
Patch
Comment 13 Yusuke Suzuki 2022-04-01 22:35:06 PDT 
Created attachment 456447 [details]
Patch
Comment 14 Yusuke Suzuki 2022-04-04 15:37:02 PDT 
Created attachment 456642 [details]
Patch
Comment 15 Saam Barati 2022-04-04 18:39:01 PDT 
Comment on attachment 456642 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:9
> +        This helps unlinked Baseline and DFG since we no longer need to load CodeBlock from callee via costly dependent loads: unlinked ones

and LLInt

> Source/JavaScriptCore/jit/JIT.cpp:754
> +#if ASSERT_ENABLED
> +        probeDebug([=](Probe::Context& ctx) {
> +            CodeBlock* codeBlock = ctx.fp<CallFrame*>()->codeBlock();
> +            if (codeBlock->jitType() != JITType::BaselineJIT) {
> +                dataLogLn("FP ", RawPointer(ctx.fp<CallFrame*>()));
> +                RELEASE_ASSERT_NOT_REACHED();
> +            }
> +        });
> +#endif

Why do we only do this release assert for function code? It should always be correct. Same for DFG/FTL

> Source/JavaScriptCore/runtime/FunctionExecutable.h:302
> +        return 0;

can you RELEASE_ASSERT_NOT_REACHED here?
Comment 16 Saam Barati 2022-04-04 18:39:56 PDT 
Comment on attachment 456642 [details]
Patch

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

>> Source/JavaScriptCore/jit/JIT.cpp:754
>> +#endif
> 
> Why do we only do this release assert for function code? It should always be correct. Same for DFG/FTL

My unlinked baseline patch might've done things this way. But I think it's worth doing always.
Comment 17 Yusuke Suzuki 2022-04-04 19:11:53 PDT 
Comment on attachment 456642 [details]
Patch

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

Thank you!

>> Source/JavaScriptCore/ChangeLog:9
>> +        This helps unlinked Baseline and DFG since we no longer need to load CodeBlock from callee via costly dependent loads: unlinked ones
> 
> and LLInt

Fixed.

>>> Source/JavaScriptCore/jit/JIT.cpp:754
>>> +#endif
>> 
>> Why do we only do this release assert for function code? It should always be correct. Same for DFG/FTL
> 
> My unlinked baseline patch might've done things this way. But I think it's worth doing always.

Sounds good! Changed.

>> Source/JavaScriptCore/runtime/FunctionExecutable.h:302
>> +        return 0;
> 
> can you RELEASE_ASSERT_NOT_REACHED here?

Nice, fixed.
Comment 18 Yusuke Suzuki 2022-04-04 19:14:14 PDT 
Created attachment 456661 [details]
Patch
Comment 19 Yusuke Suzuki 2022-04-04 20:19:49 PDT 
Created attachment 456664 [details]
Patch
Comment 20 Yusuke Suzuki 2022-04-04 21:10:49 PDT 
Committed r292372 (249237@trunk): <https://commits.webkit.org/249237@trunk>
Comment 21 Radar WebKit Bug Importer 2022-04-04 21:11:18 PDT 
<rdar://problem/91276034>
Comment 22 Michael Catanzaro 2022-04-07 13:20:18 PDT 
This broke s390x, see bug #238956