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).
Created attachment 410666 [details] Patch
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
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.
<rdar://problem/70246622>
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`.
Created attachment 411583 [details] Patch
Committed r268656: <https://trac.webkit.org/changeset/268656> All reviewed patches have been landed. Closing bug and clearing flags on attachment 411583 [details].
*** Bug 217844 has been marked as a duplicate of this bug. ***