Bug 217373 – [ESNext][JIT] Add support for UntypedUse on PutPrivateName's base operand

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

[Caio Lima]

Right now we are always speculating cell, but it is very costly when we need to throw an exception due to invalid receivers (e.g. primitives).

[Caio Lima]

Created attachment 410666 [details]
Patch

[Caitlin Potter (:caitp)]

Comment on attachment 410666 [details]
Patch

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

looks okay here 👍

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1917
> +            fixEdge<UntypedUse>(node->child1());

Are we not UntypedUse by default? Are there any assertions failing in debug tests without this? (Asking because I haven't done this in the GetPrivateName patch)

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3530
> +    case CellUse: {

Here, the approach I went with was to let `speculate(...)` do the speculation of base, and conditionally emit a cell check and slow path in the case where we aren't fairly sure it's a cell. Would that make sense here too? (I don't have a strong opinion on this, this is fine)

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3976
> +        LValue base = m_node->child1().useKind() == CellUse ? lowCell(m_node->child1()) : lowJSValue(m_node->child1());

We're just calling a slow case function, so I'm not sure there's any reason to not just lowJSValue() in all cases

[Yusuke Suzuki]

Comment on attachment 410666 [details]
Patch

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

Can you also ensure whether PutPrivateNameById's CellUse is correct? Does it work correctly even if it gets Symbol / JSString? If it is not tested, can you add tests too?

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1915
> +                fixEdge<CellUse>(node->child1());
> +                break;
> +            }

Let's remove CellUse handling since it does not add optimizations.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3530
>> +    case CellUse: {
> 
> Here, the approach I went with was to let `speculate(...)` do the speculation of base, and conditionally emit a cell check and slow path in the case where we aren't fairly sure it's a cell. Would that make sense here too? (I don't have a strong opinion on this, this is fine)

I think this style is OK since we can avoid adding a new XXXUse here without adding an implementation.
For example, if we add ObjectUse here, then `speculate(...)` can work, but it is possible that following code is not assuming that it is ObjectUse speculated one.
Anyway, for this path, I think we do not need to have CellUse. And I think CellUse speculation kind would be wrong since it can allow JSString / Symbol etc.
Let's just have UntypedUse code.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3976
>> +        LValue base = m_node->child1().useKind() == CellUse ? lowCell(m_node->child1()) : lowJSValue(m_node->child1());
> 
> We're just calling a slow case function, so I'm not sure there's any reason to not just lowJSValue() in all cases

Agreed. I think for now, just speculating it with UntypedUse is OK. We are speculating CellUse, but we are not using this information. And I think CellUse speculation is not correct: it should be ObjectUse. Let's just call lowJSValue().

> Source/JavaScriptCore/jit/JITOperations.cpp:-684
> -    JSObject* baseObject = asObject(baseValue);

Previously, we are using `asObject`. But speculation was CellUse. This means that cells like JSString, Symbol etc. can pass that speculation.
Was that a bug? If it is, can you add a test to cover this case?

> Source/JavaScriptCore/jit/JITOperations.cpp:-703
> -    JSObject* baseObject = asObject(baseValue);

Ditto. Please add a test which flows String / Symbol into this code.

> Source/JavaScriptCore/jit/JITOperations.cpp:-739
> -    JSObject* baseObject = asObject(JSValue::decode(encodedBase));

Ditto.

> Source/JavaScriptCore/jit/JITOperations.cpp:-774
> -    JSObject* baseObject = asObject(JSValue::decode(encodedBase));

Ditto.

> Source/JavaScriptCore/jit/JITOperations.cpp:-1158
> -    auto baseObject = asObject(baseValue);

Ditto.

> Source/JavaScriptCore/jit/JITOperations.cpp:-1187
> -    auto baseObject = asObject(baseValue);

Ditto.

[Radar WebKit Bug Importer]

<rdar://problem/70246622>

[Caio Lima]

Comment on attachment 410666 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1917
>> +            fixEdge<UntypedUse>(node->child1());
> 
> Are we not UntypedUse by default? Are there any assertions failing in debug tests without this? (Asking because I haven't done this in the GetPrivateName patch)

IIUC, we are `UntypedUse` by default and this fixEdge would be redundant. I think we can remove it.

>> Source/JavaScriptCore/jit/JITOperations.cpp:-684
>> -    JSObject* baseObject = asObject(baseValue);
> 
> Previously, we are using `asObject`. But speculation was CellUse. This means that cells like JSString, Symbol etc. can pass that speculation.
> Was that a bug? If it is, can you add a test to cover this case?

It was a bug indeed. It was also a bug into LLInt slow paths. I'll make it clearer in the ChangeLog.  I added `JSTests/stress/get-private-name-with-primitive.js` to catch this issue.

>> Source/JavaScriptCore/jit/JITOperations.cpp:-703
>> -    JSObject* baseObject = asObject(baseValue);
> 
> Ditto. Please add a test which flows String / Symbol into this code.

I'll include String and Symbol on `JSTests/stress/put-private-name-untyped-use.js`.

[Caio Lima]

Created attachment 411583 [details]
Patch

[EWS]

Committed r268656: <https://trac.webkit.org/changeset/268656>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411583 [details].

[Alexey Shvayka]

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