Bug 226107 - Reduce Baseline JIT emitted code size for op_jfalse, op_jtrue, op_get_from_scope, op_resolve_scope.
Summary: Reduce Baseline JIT emitted code size for op_jfalse, op_jtrue, op_get_from_sc...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-21 13:29 PDT by Mark Lam
Modified: 2021-05-25 11:04 PDT (History)
6 users (show)

See Also:


Attachments
proposed patch. (56.74 KB, patch)
2021-05-21 14:20 PDT, Mark Lam
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
proposed patch. (56.58 KB, patch)
2021-05-21 14:27 PDT, Mark Lam
sbarati: review+
Details | Formatted Diff | Diff
patch for landing. (56.70 KB, patch)
2021-05-21 19:35 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
patch for landing. (49.68 KB, patch)
2021-05-24 20:06 PDT, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2021-05-21 13:29:35 PDT
Using more JIT thunks.
Comment 1 Mark Lam 2021-05-21 14:20:26 PDT
Created attachment 429340 [details]
proposed patch.
Comment 2 Radar WebKit Bug Importer 2021-05-21 14:21:11 PDT
<rdar://problem/78328476>
Comment 3 Mark Lam 2021-05-21 14:27:06 PDT
Created attachment 429342 [details]
proposed patch.
Comment 4 Saam Barati 2021-05-21 16:53:33 PDT
Comment on attachment 429342 [details]
proposed patch.

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

r=me

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1890
> +        if constexpr (thunkNotUsedForResolveScope(resolveType)) { \
> +            RELEASE_ASSERT_NOT_REACHED(); \
> +            return { }; \
> +        } \

Why not just RELEASE_ASSERT in generateOpResolveScopeThunk

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1910
> +#if CPU(X86_64)
> +    jit.push(X86Registers::ebp);
> +#elif CPU(ARM64)
> +    jit.pushPair(framePointerRegister, linkRegister);
> +#endif

not just emit a prologue?

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1924
> +    jit.setupArguments<decltype(operationGetFromScope)>(globalObjectGPR, instructionGPR);

operationResolveScopeForBaseline, not operationGetFromScope

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1933
> +#if CPU(X86_64)
> +    jit.pop(X86Registers::ebp);
> +#elif CPU(ARM64)
> +    jit.popPair(CCallHelpers::framePointerRegister, CCallHelpers::linkRegister);
> +#endif

ditto for epilogue

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:2353
> +    jit.move(metadataGPR, GPRInfo::numberTagRegister); // Preserve metadata in a callee saved register.

I'd assert numberTagRegister is a CSR

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:2360
> +    jit.move(TrustedImm64(JSValue::NumberTag), GPRInfo::numberTagRegister);

I'd do this before the exception check for sanity
Comment 5 Mark Lam 2021-05-21 19:17:18 PDT
Comment on attachment 429342 [details]
proposed patch.

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

Thanks for the review.

>> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1890
>> +        } \
> 
> Why not just RELEASE_ASSERT in generateOpResolveScopeThunk

I will do that.

>> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1910
>> +#endif
> 
> not just emit a prologue?

We can't use emitFunctionPrologue() because we don't want to modify the framePointerRegister.  emitFunctionPrologue() will move the sp into fp.  The thunk should preserve the frame of the JS function that called it.  The only reason for pushing here is to preserve the return address because this thunk can make a call, and hence, trash the linkRegister.  For both ARM64 and x86_64, we push the framePointerRegister here just to keep the stack aligned.  We could have pushed any other register, but fp seems natural.

>> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1924
>> +    jit.setupArguments<decltype(operationGetFromScope)>(globalObjectGPR, instructionGPR);
> 
> operationResolveScopeForBaseline, not operationGetFromScope

Will fix.

>> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1933
>> +#endif
> 
> ditto for epilogue

Ditto: emitFunctionEpilogue() modifies sp with fp.  We also don't want to do that.

>> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:2353
>> +    jit.move(metadataGPR, GPRInfo::numberTagRegister); // Preserve metadata in a callee saved register.
> 
> I'd assert numberTagRegister is a CSR

Will assert.

>> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:2360
>> +    jit.move(TrustedImm64(JSValue::NumberTag), GPRInfo::numberTagRegister);
> 
> I'd do this before the exception check for sanity

Can't do that.  Before we restore the numberTagRegister, we need to store the result in the profile.  However, we only want to store the result in the profile if the result is valid because we don't want to pollute the profile with junk.  In the presence of an exception, it's not valid.
Comment 6 Mark Lam 2021-05-21 19:23:23 PDT
I'm going to invert thunkNotUsedForGetFromScope => thunkIsUsedForGetFromScope to avoid a double negative with the new assert I have to add.

Ditto for thunkNotUsedForResolveScope.
Comment 7 Mark Lam 2021-05-21 19:35:38 PDT
Created attachment 429390 [details]
patch for landing.
Comment 8 Mark Lam 2021-05-21 19:58:51 PDT
The JSC EWS bot failures appear to be real.  However, they only reproduce on x86_64.  I will investigate.
Comment 9 Mark Lam 2021-05-23 14:11:21 PDT
(In reply to Mark Lam from comment #8)
> The JSC EWS bot failures appear to be real.  However, they only reproduce on
> x86_64.  I will investigate.

The jsc bot failures are due to flaky test failures.  I'll try to address that in another bug before landing this one.
Comment 10 Mark Lam 2021-05-24 20:06:21 PDT
Created attachment 429618 [details]
patch for landing.
Comment 11 Mark Lam 2021-05-25 11:04:16 PDT
Landed in r278029: <http://trac.webkit.org/r278029>.