Bug 229229

Summary: [JSC] Polymorphic PutByVal
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, pvollan, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
saam: review+, ews-feeder: commit-queue-
Patch none

Description Yusuke Suzuki 2021-08-17 23:31:57 PDT
[JSC] Polymorphic PutByVal
Comment 1 Yusuke Suzuki 2021-08-17 23:32:33 PDT
Created attachment 435749 [details]
Patch
Comment 2 Yusuke Suzuki 2021-08-18 12:26:32 PDT
Created attachment 435789 [details]
Patch
Comment 3 Yusuke Suzuki 2021-08-18 15:15:38 PDT
Created attachment 435804 [details]
Patch
Comment 4 Yusuke Suzuki 2021-08-18 16:33:38 PDT
Created attachment 435816 [details]
Patch
Comment 5 Yusuke Suzuki 2021-08-18 19:27:50 PDT
Created attachment 435827 [details]
Patch
Comment 6 Yusuke Suzuki 2021-08-18 19:31:38 PDT
Created attachment 435828 [details]
Patch
Comment 7 Yusuke Suzuki 2021-08-18 22:54:42 PDT
Created attachment 435846 [details]
Patch
Comment 8 Yusuke Suzuki 2021-08-19 01:48:42 PDT
Created attachment 435856 [details]
Patch
Comment 9 Yusuke Suzuki 2021-08-20 18:15:41 PDT
Created attachment 436051 [details]
Patch
Comment 10 Yusuke Suzuki 2021-08-21 04:59:46 PDT
Created attachment 436069 [details]
Patch
Comment 11 Yusuke Suzuki 2021-08-21 05:09:49 PDT
Created attachment 436070 [details]
Patch
Comment 12 Yusuke Suzuki 2021-08-21 05:15:48 PDT
Created attachment 436071 [details]
Patch
Comment 13 Yusuke Suzuki 2021-08-23 00:14:02 PDT
Created attachment 436154 [details]
Patch
Comment 14 Yusuke Suzuki 2021-08-23 01:04:17 PDT
Created attachment 436158 [details]
Patch
Comment 15 Yusuke Suzuki 2021-08-23 01:33:02 PDT
Created attachment 436163 [details]
Patch
Comment 16 Yusuke Suzuki 2021-08-23 02:57:55 PDT
Created attachment 436174 [details]
Patch
Comment 17 Yusuke Suzuki 2021-08-23 03:09:42 PDT
Created attachment 436176 [details]
Patch
Comment 18 Yusuke Suzuki 2021-08-23 03:19:12 PDT
Created attachment 436177 [details]
Patch
Comment 19 Yusuke Suzuki 2021-08-23 20:02:26 PDT
Created attachment 436260 [details]
Patch
Comment 20 Yusuke Suzuki 2021-08-24 09:18:20 PDT
Created attachment 436294 [details]
Patch
Comment 21 Yusuke Suzuki 2021-08-24 09:40:59 PDT
Created attachment 436296 [details]
Patch
Comment 22 Yusuke Suzuki 2021-08-24 15:00:51 PDT
Created attachment 436337 [details]
Patch
Comment 23 Yusuke Suzuki 2021-08-24 15:13:39 PDT
Created attachment 436338 [details]
Patch
Comment 24 Yusuke Suzuki 2021-08-24 15:25:01 PDT
Created attachment 436341 [details]
Patch
Comment 25 Radar WebKit Bug Importer 2021-08-24 23:32:19 PDT
<rdar://problem/82327133>
Comment 26 Yusuke Suzuki 2021-08-25 14:05:33 PDT
Created attachment 436423 [details]
Patch
Comment 27 Yusuke Suzuki 2021-08-25 14:47:44 PDT
Created attachment 436429 [details]
Patch
Comment 28 Saam Barati 2021-08-25 19:45:15 PDT
Comment on attachment 436429 [details]
Patch

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

Nice. r=me with a few comments.

> Source/JavaScriptCore/bytecode/AccessCase.cpp:1068
> +        if (stubInfo.m_arrayProfileGPR != InvalidGPRReg)

assert it's invalidGPR?

> Source/JavaScriptCore/bytecode/AccessCase.cpp:1170
> +        if (stubInfo.m_arrayProfileGPR != InvalidGPRReg)
> +            allocator.lock(stubInfo.m_arrayProfileGPR);

assert it's invalidGPR?

> Source/JavaScriptCore/bytecode/AccessCase.cpp:1257
> +        if (stubInfo.m_arrayProfileGPR != InvalidGPRReg)
> +            allocator.lock(stubInfo.m_arrayProfileGPR);

assert it's invalidGPR?

> Source/JavaScriptCore/bytecode/AccessCase.cpp:1492
> +                failAndIgnore.append(jit.branchIfNaN(state.scratchFPR));

Why ignore? Shouldn't this be a case to repatch, since we're not storing int32? Or maybe we expect this particular array to get transitioned into something more general, so we can kind of ignore the likelihood of this?

I guess I'm just worried about the case where we see a lot of non numbers in practice? Like, say we started off with profiling a single number case, but then we ended up with many things that are never numbers, we'll never end up repatching. Not sure what the right approach is, but it's worth considering if we should do something else here.

> Source/JavaScriptCore/bytecode/AccessCase.cpp:1505
> +                failAndIgnore.append(jit.branchIfNotInt32(valueRegs));

ditto regarding repatching vs not repatching.

> Source/JavaScriptCore/bytecode/AccessCase.cpp:1573
> +            state.failAndIgnore.append(jit.branchIfNotInt32(valueRegs));

ditto about repatching or ignoring.

> Source/JavaScriptCore/bytecode/AccessCase.cpp:1582
> +            state.failAndIgnore.append(jit.branchIfNotNumber(valueRegs.payloadGPR()));

ditto

> Source/JavaScriptCore/bytecode/AccessCase.cpp:1594
> +        state.failAndRepatch.append(jit.branch32(CCallHelpers::AboveOrEqual, propertyGPR, scratchGPR));

we fail and repatch here, but ignore the case for normal arrays when we go OOB past vector length. Is that intentional?

> Source/JavaScriptCore/bytecode/AccessCase.cpp:2434
> +        if (stubInfo.m_arrayProfileGPR != InvalidGPRReg)
> +            allocator.lock(stubInfo.m_arrayProfileGPR);

Assert invalidGPR?

> Source/JavaScriptCore/bytecode/GetterSetterAccessCase.cpp:145
> +    if (stubInfo.m_arrayProfileGPR != InvalidGPRReg)
> +        allocator.lock(stubInfo.m_arrayProfileGPR);

assert is invalidGPR?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6475
> +                    tryCompileAsPutByOffset = true;

let's rename this to "compiledAsPutPrivateNameById"

> Source/JavaScriptCore/jit/Repatch.cpp:899
> +        GCSafeConcurrentJSLocker locker(codeBlock->m_lock, globalObject->vm().heap);

small nit: Maybe grab lock below the if and switches, since some cases return?
Comment 29 Yusuke Suzuki 2021-08-25 21:26:37 PDT
Created attachment 436464 [details]
Patch

Patch for landing
Comment 30 Yusuke Suzuki 2021-08-25 22:58:38 PDT
Comment on attachment 436429 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/AccessCase.cpp:1068
>> +        if (stubInfo.m_arrayProfileGPR != InvalidGPRReg)
> 
> assert it's invalidGPR?

Changed.

>> Source/JavaScriptCore/bytecode/AccessCase.cpp:1170
>> +            allocator.lock(stubInfo.m_arrayProfileGPR);
> 
> assert it's invalidGPR?

Changed.

>> Source/JavaScriptCore/bytecode/AccessCase.cpp:1257
>> +            allocator.lock(stubInfo.m_arrayProfileGPR);
> 
> assert it's invalidGPR?

Changed.

>> Source/JavaScriptCore/bytecode/AccessCase.cpp:1492
>> +                failAndIgnore.append(jit.branchIfNaN(state.scratchFPR));
> 
> Why ignore? Shouldn't this be a case to repatch, since we're not storing int32? Or maybe we expect this particular array to get transitioned into something more general, so we can kind of ignore the likelihood of this?
> 
> I guess I'm just worried about the case where we see a lot of non numbers in practice? Like, say we started off with profiling a single number case, but then we ended up with many things that are never numbers, we'll never end up repatching. Not sure what the right approach is, but it's worth considering if we should do something else here.

Changed to failAndRepatch.

>> Source/JavaScriptCore/bytecode/AccessCase.cpp:1505
>> +                failAndIgnore.append(jit.branchIfNotInt32(valueRegs));
> 
> ditto regarding repatching vs not repatching.

Changed to failAndRepatch.

>> Source/JavaScriptCore/bytecode/AccessCase.cpp:1573
>> +            state.failAndIgnore.append(jit.branchIfNotInt32(valueRegs));
> 
> ditto about repatching or ignoring.

Changed to failAndRepatch.

>> Source/JavaScriptCore/bytecode/AccessCase.cpp:1582
>> +            state.failAndIgnore.append(jit.branchIfNotNumber(valueRegs.payloadGPR()));
> 
> ditto

Changed to failAndRepatch.

>> Source/JavaScriptCore/bytecode/AccessCase.cpp:1594
>> +        state.failAndRepatch.append(jit.branch32(CCallHelpers::AboveOrEqual, propertyGPR, scratchGPR));
> 
> we fail and repatch here, but ignore the case for normal arrays when we go OOB past vector length. Is that intentional?

Discussed with Saam at Slack. We will continue using failAndRepatch since OOB TypedArray access case should soon get the slow path.

>> Source/JavaScriptCore/bytecode/AccessCase.cpp:2434
>> +            allocator.lock(stubInfo.m_arrayProfileGPR);
> 
> Assert invalidGPR?

Changed.

>> Source/JavaScriptCore/bytecode/GetterSetterAccessCase.cpp:145
>> +        allocator.lock(stubInfo.m_arrayProfileGPR);
> 
> assert is invalidGPR?

Changed.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6475
>> +                    tryCompileAsPutByOffset = true;
> 
> let's rename this to "compiledAsPutPrivateNameById"

Changed.

>> Source/JavaScriptCore/jit/Repatch.cpp:899
>> +        GCSafeConcurrentJSLocker locker(codeBlock->m_lock, globalObject->vm().heap);
> 
> small nit: Maybe grab lock below the if and switches, since some cases return?

Fixed!
Comment 31 Yusuke Suzuki 2021-08-26 00:39:11 PDT
Committed r281615 (240971@main): <https://commits.webkit.org/240971@main>
Comment 32 Yusuke Suzuki 2021-08-26 22:48:27 PDT
*** Bug 203866 has been marked as a duplicate of this bug. ***