Bug 226107 – Reduce Baseline JIT emitted code size for op_jfalse, op_jtrue, op_get_from_scope, op_resolve_scope.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

Using more JIT thunks.

[Mark Lam]

Created attachment 429340 [details]
proposed patch.

[Radar WebKit Bug Importer]

<rdar://problem/78328476>

[Mark Lam]

Created attachment 429342 [details]
proposed patch.

[Saam Barati]

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

[Mark Lam]

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.

[Mark Lam]

I'm going to invert thunkNotUsedForGetFromScope => thunkIsUsedForGetFromScope to avoid a double negative with the new assert I have to add.

Ditto for thunkNotUsedForResolveScope.

[Mark Lam]

Created attachment 429390 [details]
patch for landing.

[Mark Lam]

The JSC EWS bot failures appear to be real.  However, they only reproduce on x86_64.  I will investigate.

[Mark Lam]

(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.

[Mark Lam]

Created attachment 429618 [details]
patch for landing.

[Mark Lam]

Landed in r278029: <http://trac.webkit.org/r278029>.