Bug 229229 – [JSC] Polymorphic PutByVal

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Polymorphic PutByVal

[Yusuke Suzuki]

Created attachment 435749 [details]
Patch

[Yusuke Suzuki]

Created attachment 435789 [details]
Patch

[Yusuke Suzuki]

Created attachment 435804 [details]
Patch

[Yusuke Suzuki]

Created attachment 435816 [details]
Patch

[Yusuke Suzuki]

Created attachment 435827 [details]
Patch

[Yusuke Suzuki]

Created attachment 435828 [details]
Patch

[Yusuke Suzuki]

Created attachment 435846 [details]
Patch

[Yusuke Suzuki]

Created attachment 435856 [details]
Patch

[Yusuke Suzuki]

Created attachment 436051 [details]
Patch

[Yusuke Suzuki]

Created attachment 436069 [details]
Patch

[Yusuke Suzuki]

Created attachment 436070 [details]
Patch

[Yusuke Suzuki]

Created attachment 436071 [details]
Patch

[Yusuke Suzuki]

Created attachment 436154 [details]
Patch

[Yusuke Suzuki]

Created attachment 436158 [details]
Patch

[Yusuke Suzuki]

Created attachment 436163 [details]
Patch

[Yusuke Suzuki]

Created attachment 436174 [details]
Patch

[Yusuke Suzuki]

Created attachment 436176 [details]
Patch

[Yusuke Suzuki]

Created attachment 436177 [details]
Patch

[Yusuke Suzuki]

Created attachment 436260 [details]
Patch

[Yusuke Suzuki]

Created attachment 436294 [details]
Patch

[Yusuke Suzuki]

Created attachment 436296 [details]
Patch

[Yusuke Suzuki]

Created attachment 436337 [details]
Patch

[Yusuke Suzuki]

Created attachment 436338 [details]
Patch

[Yusuke Suzuki]

Created attachment 436341 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/82327133>

[Yusuke Suzuki]

Created attachment 436423 [details]
Patch

[Yusuke Suzuki]

Created attachment 436429 [details]
Patch

[Saam Barati]

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?

[Yusuke Suzuki]

Created attachment 436464 [details]
Patch

Patch for landing

[Yusuke Suzuki]

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!

[Yusuke Suzuki]

Committed r281615 (240971@main): <https://commits.webkit.org/240971@main>

[Yusuke Suzuki]

*** Bug 203866 has been marked as a duplicate of this bug. ***