[webkit-reviews] review denied: [Bug 213372] [JSC][ESNext] Create a new opcode to handle private fields store/define : [Attachment 407475] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has denied Caio Lima <ticaiolima at gmail.com>'s
request for review:
Bug 213372: [JSC][ESNext] Create a new opcode to handle private fields
store/define
https://bugs.webkit.org/show_bug.cgi?id=213372

Attachment 407475: Patch

https://bugs.webkit.org/attachment.cgi?id=407475&action=review




--- Comment #31 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 407475
  --> https://bugs.webkit.org/attachment.cgi?id=407475
Patch

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:4050
>	       PutByIdStatus status = PutByIdStatus::computeFor(

Inside PutByIdStatus::computeFor, it can find transitioned-structure by adding
a new property. And this transition can be constructed in different "define"
type PutPrivateName.
But even if this PutPrivateNameById is "set" version, this structure transition
will be populated here, and I think this is not correct.

> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:1406
> +	   PutByIdStatus status = PutByIdStatus::computeFor(
> +	       m_graph.globalObjectFor(origin.semantic),
> +	       baseValue.m_structure.toStructureSet(),
> +	       node->cacheableIdentifier().uid(),
> +	       isDirect);

Ditto. I think this can populate adding-property-structure-transition case even
if this is "set" PutPrivateNameById.

> Source/JavaScriptCore/jit/JIT.h:172
> +	   ByValCompilationInfo(ByValInfo* byValInfo, BytecodeIndex
bytecodeIndex, MacroAssembler::PatchableJump notIndexJump,
MacroAssembler::Label doneTarget, MacroAssembler::Label nextHotPathTarget)
> +	       : byValInfo(byValInfo)
> +	       , bytecodeIndex(bytecodeIndex)
> +	       , notIndexJump(notIndexJump)
> +	       , doneTarget(doneTarget)
> +	       , nextHotPathTarget(nextHotPathTarget)
> +	   {
> +	   }

Need to update this to setUp function which stores these fields to avoid
overriding bytecodeIndex again.

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:378
> +    ByValInfo* byValInfo = m_codeBlock->addByValInfo();

Need to update it to pass bytecodeIndex.

> Source/JavaScriptCore/jit/Repatch.cpp:545
>	       return operationPutByIdDirectStrict;
> -	   if (putKind == DirectPrivateFieldCreate)
> +	   if (putKind == DirectPrivateFieldDefine)
>	       return operationPutByIdDefinePrivateFieldStrict;
> -	   if (putKind == DirectPrivateFieldPut)
> -	       return operationPutByIdPutPrivateFieldStrict;
> +	   if (putKind == DirectPrivateFieldSet)
> +	       return operationPutByIdSetPrivateFieldStrict;
>	   return operationPutByIdStrict;

Let's convert it to switch to avoid missing some kinds.

> Source/JavaScriptCore/jit/Repatch.cpp:561
>	   if (putKind == Direct)
>	       return operationPutByIdDirectStrictOptimize;
> -	   if (putKind == DirectPrivateFieldCreate)
> +	   if (putKind == DirectPrivateFieldDefine)
>	       return operationPutByIdDefinePrivateFieldStrictOptimize;
> -	   if (putKind == DirectPrivateFieldPut)
> -	       return operationPutByIdPutPrivateFieldStrictOptimize;
> +	   if (putKind == DirectPrivateFieldSet)
> +	       return operationPutByIdSetPrivateFieldStrictOptimize;
>	   return operationPutByIdStrictOptimize;

Let's convert it to switch to avoid missing some kinds.

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:-1196
> -    if (subscript.isDouble()) {
> -	   ASSERT(!isPrivateFieldAccess);
> -	   double subscriptAsDouble = subscript.asDouble();
> -	   uint32_t subscriptAsUInt32 =
static_cast<uint32_t>(subscriptAsDouble);
> -	   if (subscriptAsDouble == subscriptAsUInt32 &&
isIndex(subscriptAsUInt32)) {
> -	       baseObject->putDirectIndex(globalObject, subscriptAsUInt32,
value, 0, isStrictMode ? PutDirectIndexShouldThrow :
PutDirectIndexShouldNotThrow);
> -	       LLINT_END();
> -	   }
> -    }

Why is it removed?