Bug 213372 – [JSC][ESNext] Create a new opcode to handle private fields store/define

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

[Caio Lima]

Now we are using `put_by_val_direct` with a `PutByValFlags` to indicate such operation, but while there are some similarities of `put_by_val_direct`, the semantics of set/define operation of private fields are different enough to be profitable adding a new opcode for it.

[Caio Lima]

Created attachment 402297 [details]
WIP - Patch

It starts the implementation. There is a lot of work to do yet like refactoring (there is a lot of repeated code) and rolling back the usage of private fields from `put_by_val_direct`. DFG/FTL support is not done as well.

[Caitlin Potter (:caitp)]

Comment on attachment 402297 [details]
WIP - Patch

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

> Source/JavaScriptCore/bytecode/BytecodeList.rb:581
> +        flags: PutByValFlags,

Do we want to keep PutByValFlags around? Since they only existed to distinguish put-by-val from put-private-field operations, I think we could replace it with a simpler flag here that just distinguishes between the define and update operations. It would free up a few bits for PutByVal's own flags, if it ever needs to add new ones. We don't necessarily need to remove PutByValFlags entirely, but it might make sense to remove the private-specific parts of it

> Source/JavaScriptCore/jit/JITInlines.h:708
> +ALWAYS_INLINE ECMAMode JIT::ecmaMode<OpPutPrivateName>(OpPutPrivateName)

this seems to be unused now in this patch, is it needed by JITInlineCacheGenerator?

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1354
> +    // and class methods are always in strict mode

nit: if we're going to have this informational comment, lets add `const bool isStrictMode = true;` to make it obvious what it's talking about --- A lot of us don't have PutPropertySlot parameter names memorized

[Caio Lima]

Comment on attachment 402297 [details]
WIP - Patch

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

>> Source/JavaScriptCore/bytecode/BytecodeList.rb:581
>> +        flags: PutByValFlags,
> 
> Do we want to keep PutByValFlags around? Since they only existed to distinguish put-by-val from put-private-field operations, I think we could replace it with a simpler flag here that just distinguishes between the define and update operations. It would free up a few bits for PutByVal's own flags, if it ever needs to add new ones. We don't necessarily need to remove PutByValFlags entirely, but it might make sense to remove the private-specific parts of it

After the refactoring I applied, I don't think `PutByValFlags` makes much sense now and I removed it. I'm using a `PrivateFieldPutKind` instead.

>> Source/JavaScriptCore/jit/JITInlines.h:708
>> +ALWAYS_INLINE ECMAMode JIT::ecmaMode<OpPutPrivateName>(OpPutPrivateName)
> 
> this seems to be unused now in this patch, is it needed by JITInlineCacheGenerator?

I don't think this is would be necessary anymore. I'll remove it.

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1354
>> +    // and class methods are always in strict mode
> 
> nit: if we're going to have this informational comment, lets add `const bool isStrictMode = true;` to make it obvious what it's talking about --- A lot of us don't have PutPropertySlot parameter names memorized

Agreed. Done.

[Caio Lima]

Created attachment 403988 [details]
WIP - Patch

[Caio Lima]

Created attachment 404260 [details]
WIP - Patch

It adds DFG and FTL support for 32-bits and some tests.

[Caio Lima]

Created attachment 404394 [details]
WIP - Patch

Patch is almost done, let's see EWS results

[Caio Lima]

Created attachment 404459 [details]
WIP - Patch

[Caio Lima]

Created attachment 404563 [details]
Patch

[Caio Lima]

Comment on attachment 404563 [details]
Patch

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

> Source/JavaScriptCore/runtime/OptionsList.h:568
> +    v(usePrivateClassFields, SupportsDFG | SupportsFTL, "https://bugs.webkit.org/show_bug.cgi?id=212781", "https://bugs.webkit.org/show_bug.cgi?id=212784")

I changed it and also added `runNoDFG` mode into some tests that emit `get_private_name` to be able to see EWS run with JIT configurations of `put_private_name`. This should be changed back `LLIntAndBaselineOnly` until we get proper JIT support of both `put_private_name` and `get_private_name`.

[Caitlin Potter (:caitp)]

Comment on attachment 404563 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6060
> +                        compiledAsPutById = true;

nit: I would write this as `didCompileAsPutById`, to make the past tense more obvious

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6068
> +                        if (identifier.isSymbolCell())

lets just `ASSERT(identifier.isSymbolCell())`, since it has to be, and will make the code a bit simpler

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6083
> +                    handlePutById(base, identifier, identifierNumber, value, putByIdStatus, isDirect, nextOpcodeIndex(), ECMAMode::strict());

nit: there's no reason we can't do this in the block above where `compiledAsPutById` is set to true

[Caio Lima]

Comment on attachment 404563 [details]
Patch

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

I've found a bug

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6083
>> +                    handlePutById(base, identifier, identifierNumber, value, putByIdStatus, isDirect, nextOpcodeIndex(), ECMAMode::strict());
> 
> nit: there's no reason we can't do this in the block above where `compiledAsPutById` is set to true

The way we are compiling here is wrong. In a case where we have a megamorphic callsite, this will compile as PutByID but slow path won't have the proper semantics for private fields. A test case for it is also needed.

[Caio Lima]

Created attachment 404951 [details]
WIP - Patch

[Caio Lima]

Created attachment 405073 [details]
Patch

[Caio Lima]

Created attachment 405381 [details]
Patch

[Caio Lima]

Created attachment 405391 [details]
Patch

[Caio Lima]

Created attachment 405459 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 405459 [details]
Patch

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

Continue reviewing, early comments.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:253
> +        bool isDirect, BytecodeIndex osrExitIndex, ECMAMode, PrivateFieldPutKind = PrivateFieldPutKind::none());

We should not use default parameter here since it is hard to follow what is passed.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6078
> +                handlePutById(base, identifier, identifierNumber, value, putByIdStatus, isDirect, nextOpcodeIndex(), ECMAMode::strict(), bytecode.m_putKind);

If we separate the bytecode from put_by_val_direct, then I think still using PutByIdDirect for op_put_private_name looks strange here. IIUC, there is semantics difference, so can we use it?

[Caio Lima]

(In reply to Yusuke Suzuki from comment #17)
> Comment on attachment 405459 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=405459&action=review
> 
> Continue reviewing, early comments.
> 
> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:253
> > +        bool isDirect, BytecodeIndex osrExitIndex, ECMAMode, PrivateFieldPutKind = PrivateFieldPutKind::none());
> 
> We should not use default parameter here since it is hard to follow what is
> passed.
> 
> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6078
> > +                handlePutById(base, identifier, identifierNumber, value, putByIdStatus, isDirect, nextOpcodeIndex(), ECMAMode::strict(), bytecode.m_putKind);
> 
> If we separate the bytecode from put_by_val_direct, then I think still using
> PutByIdDirect for op_put_private_name looks strange here. IIUC, there is
> semantics difference, so can we use it?

We are using `bytecode.m_putKind` into the JITPutByIdGenerator and this will make we properly set slow path function for it when there is an IC miss (like it is done on Baseline), keeping the semantics correct (see `JITPutByIdGenerator::slowPathFunction`). We are storing `bytecode.m_putKind` into `PutByIdDirect`'s OpInfo and using it on `SpeculativeJIT::compilePutByIdDirect` and `FTLLowerDFGToB3::compilePutById()`. The test case dedicated to validate this is `JSTests/stress/dfg-put-private-name-compiled-as-put-by-id-direct.js`. The usage of `PutByIdDirect` here is to be able to re-use all optimizations we already apply to it, including the constant folding and IC for generic cases.

One alternative we have is to avoid using `handlePutById` and only try to inline the IC on `PutByOffset` or `MultiPutByOffset`, while implementing all optimizations to `PutPrivateName`, like what's being proposed for `get_private_name`[1]. I'm liking this alternative more than current patch, but I'd rather implement it in a followup patch, though.

[1] - https://bugs.webkit.org/show_bug.cgi?id=214861

[Yusuke Suzuki]

Comment on attachment 405459 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:14
> +        This patch is adding a new opcode to handle private field storage.
> +        Before this change, we were using `put_by_val_direct` and including
> +        the information of `PutKind` into `PutByValFlags`. We initially decided
> +        to use `put_by_val_direct` to take advantage of all IC mechanism already
> +        implemented for this instruction, however the semantics of private field
> +        is different enough to complicate the understanding of
> +        `put_by_val_direct`.

I agree. So I think we should not complicate DFG PutByIdDirect too.

>>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:253
>>> +        bool isDirect, BytecodeIndex osrExitIndex, ECMAMode, PrivateFieldPutKind = PrivateFieldPutKind::none());
>> 
>> We should not use default parameter here since it is hard to follow what is passed.
> 
> We are using `bytecode.m_putKind` into the JITPutByIdGenerator and this will make we properly set slow path function for it when there is an IC miss (like it is done on Baseline), keeping the semantics correct (see `JITPutByIdGenerator::slowPathFunction`). We are storing `bytecode.m_putKind` into `PutByIdDirect`'s OpInfo and using it on `SpeculativeJIT::compilePutByIdDirect` and `FTLLowerDFGToB3::compilePutById()`. The test case dedicated to validate this is `JSTests/stress/dfg-put-private-name-compiled-as-put-by-id-direct.js`. The usage of `PutByIdDirect` here is to be able to re-use all optimizations we already apply to it, including the constant folding and IC for generic cases.
> 
> One alternative we have is to avoid using `handlePutById` and only try to inline the IC on `PutByOffset` or `MultiPutByOffset`, while implementing all optimizations to `PutPrivateName`, like what's being proposed for `get_private_name`[1]. I'm liking this alternative more than current patch, but I'd rather implement it in a followup patch, though.
> 
> [1] - https://bugs.webkit.org/show_bug.cgi?id=214861

If we share the DFG nodes, why do we need to introduce new bytecode in this patch?
Introducing new bytecodes while using shared DFG nodes is tricky.
You can add new DFG nodes (e.g. PutPrivateNameDirect), and wire existing optimizations to that DFG opcode.
I think we can use the same IC mechanism for different DFG nodes. (PutByIdDirect and PutPrivateNameDirect).
I agree to reusing cachePutById etc. We should reuse them.
But I don't think extending DFG PutByIdDirect is a good idea. We have a lot of strong assumption about PutByIdDirect.
If the behavior is sufficiently different, we should have different DFG nodes.

[Caio Lima]

Created attachment 405922 [details]
Patch

[Caio Lima]

Comment on attachment 405459 [details]
Patch

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

>>>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:253
>>>> +        bool isDirect, BytecodeIndex osrExitIndex, ECMAMode, PrivateFieldPutKind = PrivateFieldPutKind::none());
>>> 
>>> We should not use default parameter here since it is hard to follow what is passed.
>> 
>> We are using `bytecode.m_putKind` into the JITPutByIdGenerator and this will make we properly set slow path function for it when there is an IC miss (like it is done on Baseline), keeping the semantics correct (see `JITPutByIdGenerator::slowPathFunction`). We are storing `bytecode.m_putKind` into `PutByIdDirect`'s OpInfo and using it on `SpeculativeJIT::compilePutByIdDirect` and `FTLLowerDFGToB3::compilePutById()`. The test case dedicated to validate this is `JSTests/stress/dfg-put-private-name-compiled-as-put-by-id-direct.js`. The usage of `PutByIdDirect` here is to be able to re-use all optimizations we already apply to it, including the constant folding and IC for generic cases.
>> 
>> One alternative we have is to avoid using `handlePutById` and only try to inline the IC on `PutByOffset` or `MultiPutByOffset`, while implementing all optimizations to `PutPrivateName`, like what's being proposed for `get_private_name`[1]. I'm liking this alternative more than current patch, but I'd rather implement it in a followup patch, though.
>> 
>> [1] - https://bugs.webkit.org/show_bug.cgi?id=214861
> 
> If we share the DFG nodes, why do we need to introduce new bytecode in this patch?
> Introducing new bytecodes while using shared DFG nodes is tricky.
> You can add new DFG nodes (e.g. PutPrivateNameDirect), and wire existing optimizations to that DFG opcode.
> I think we can use the same IC mechanism for different DFG nodes. (PutByIdDirect and PutPrivateNameDirect).
> I agree to reusing cachePutById etc. We should reuse them.
> But I don't think extending DFG PutByIdDirect is a good idea. We have a lot of strong assumption about PutByIdDirect.
> If the behavior is sufficiently different, we should have different DFG nodes.

I agree that sharing them is a recipe for headaches. I uploaded a new version that is not emitting `PutByIdDirect` when compiling `put_private_name` and the idea is to optimize `PutPrivateName` on https://bugs.webkit.org/show_bug.cgi?id=215126

[Yusuke Suzuki]

Comment on attachment 405922 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4841
>          if (!isDirect) {
> +            ASSERT(privateFieldPutKind.isNone());
>              for (unsigned variantIndex = putByIdStatus.numVariants(); variantIndex--;) {
>                  if (putByIdStatus[variantIndex].kind() != PutByIdVariant::Transition)
>                      continue;

We are using PutById / Direct path for private names. However privateFieldPutKind has Define / Set modes.
If Define and Set has different semantics, why can we use the same PutByOffset etc.? Can you explain it?

[Caio Lima]

(In reply to Yusuke Suzuki from comment #22)
> Comment on attachment 405922 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=405922&action=review
> 
> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4841
> >          if (!isDirect) {
> > +            ASSERT(privateFieldPutKind.isNone());
> >              for (unsigned variantIndex = putByIdStatus.numVariants(); variantIndex--;) {
> >                  if (putByIdStatus[variantIndex].kind() != PutByIdVariant::Transition)
> >                      continue;
> 
> We are using PutById / Direct path for private names. However
> privateFieldPutKind has Define / Set modes.
> If Define and Set has different semantics, why can we use the same
> PutByOffset etc.? Can you explain it?

For now, we are only compiling `put_private_name` as `[Multi]PutByOffset` from information we get from stubInfo. This info is populated during Baseline IC slow path execution of private name on `operationPutByIdDefinePrivateFieldStrictOptimize` and `operationPutByIdPutPrivateFieldStrictOptimize` (this was already added by https://trac.webkit.org/changeset/262613). Once we have a stub for any given structure set, we can infer that put private field operations always succeed for this set, and we don't need to check for `Set|Define` semantics for those cases.

Since we emit a `CheckStructure` before `PutByOffset`, in case of a structure check miss, we will OSR and then perform the proper `Set|Define` semantics in whatever tier we OSR to. For cases where we decide to not emit `PutByOffset`, we then emit `PutPrivateName` and this node checks for `Set|Define` private field semantics.

We are testing OSR cases in `JSTests/stress/put-private-name-osr.js` and `JSTests/stress/put-private-name-set-before-define.js`. It is important to notice that a `put_private_name` with `Define` put kind will only have `PutByIdVariant::Transition` while the one with `Set` will only have `PutByIdVariant::Replace` (this should be added as an ASSERT).

[Caio Lima]

Comment on attachment 405922 [details]
Patch

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

> JSTests/stress/put-private-name-set-before-define.js:1
> +//@ requireOptions("--allowUnsupportedTiers=true", "--usePrivateClassFields=true")

This test is a duplicate of JSTests/stress/put-private-name-invalid-store.js. Also, this test is probably wrong on its purpose, since the idea here is to trigger an OSR due to `this.#foo` `BadCache`, put it is potentially getting invalidate when we execute `shouldStore = true;` within the for loop.

[Caio Lima]

Created attachment 406381 [details]
Patch

[Caitlin Potter (:caitp)]

Comment on attachment 406381 [details]
Patch

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

> Source/JavaScriptCore/jit/JITOperations.cpp:1168
> +        if (++byValInfo->slowPathCount >= 10)

Would it make sense to define this in an JSC::Options field instead of just hardcoding `10` here?

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1551
> +        genericFunction = bytecode.m_putKind.isDefine() ? operationDefinePrivateNameGeneric : operationSetPrivateNameGeneric;

It might be better to give PutPrivateName its own JIT and DFG handling. There are downsides, but it seems less fragile than sharing with PutByID.

[Caio Lima]

Created attachment 406597 [details]
Patch

[Caio Lima]

Created attachment 406725 [details]
Patch

[Caio Lima]

Created attachment 407222 [details]
Patch

[Caio Lima]

Created attachment 407475 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 407475 [details]
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?

[Caio Lima]

Comment on attachment 407475 [details]
Patch

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

Thank you very much for the 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.

This is definitely not correct. I added a test that is wrongly optimized by this bug. Thanks for the catch.

>> Source/JavaScriptCore/jit/JIT.h:172
>> +        }
> 
> Need to update this to setUp function which stores these fields to avoid overriding bytecodeIndex again.

I see a `setUp` function into `ByValInfo`, but not into `ByValCompilationInfo`.  I added this new constructor since there's no `badTypeJump`, `arrayMode` and `arrayProfile` for `put_private_name` case.

>> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:-1196
>> -    }
> 
> Why is it removed?

This was introduced by https://trac.webkit.org/changeset/262613 and I'm reverting all changes of `put_by_val_direct` from this bug. TBH, I don't know was this was introduced back in that revision

[Caio Lima]

Created attachment 407884 [details]
Patch

[Caio Lima]

Created attachment 407964 [details]
Patch

[Caio Lima]

Created attachment 407972 [details]
Patch

[Caio Lima]

Created attachment 407977 [details]
Patch

[Caio Lima]

Created attachment 408180 [details]
Patch

[Caio Lima]

(In reply to Caio Lima from comment #37)
> Created attachment 408180 [details]
> Patch

This fixed some nits and added microbenchmarks. Here are results for reference:

```
Collected 10 samples per benchmark/VM, with 10 VM invocations per benchmark. Emitted a call to gc() between sample
measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used the jsc-specific preciseTime()
function to get microsecond-level timing. Reporting benchmark execution times with 95% confidence intervals in
milliseconds.

                                           baseline                  changes                                      
Microbenchmarks:
   put-public-field                    21.2727+-0.4655           21.0484+-0.5071          might be 1.0107x faster
   polymorphic-put-public-field        36.0990+-1.2683     ?     36.8361+-1.6222        ? might be 1.0204x slower

   <geometric>                         27.7048+-0.6616     ?     27.8283+-0.6683        ? might be 1.0045x slower

                                           baseline                  changes                                      
PrivateFieldsBench:
   put-private-field                   51.0347+-2.2051     ^     21.5075+-0.3837        ^ definitely 2.3729x faster
   polymorphic-put-private-field      148.8444+-0.9087     ^     36.8156+-0.8545        ^ definitely 4.0430x faster

   <geometric>                         87.1192+-1.8639     ^     28.1339+-0.4085        ^ definitely 3.0966x faster

                                           baseline                  changes                                      
Geomean of preferred means:
   <scaled-result>                     49.1189+-0.8266     ^     27.9757+-0.3906        ^ definitely 1.7558x faster
```

We can see that it makes private fields put operations significantly faster and considering DFG/FTL, we have same performance numbers than common properties. It's also important to notice that PrivateFields bench on "baseline" version is not running with JIT enabled.

[Yusuke Suzuki]

Comment on attachment 408180 [details]
Patch

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

> Source/JavaScriptCore/bytecode/PutByIdStatus.cpp:316
>                  return PutByIdStatus(TakesSlowPath);

Is this

if (isValidOffset(offset)) {
    ...
}

correct for define case?

[Yusuke Suzuki]

Comment on attachment 408180 [details]
Patch

Discussed with Caio at Slack. My suggestion is that we should have branches and ASSERTs in code that is newly exercised by PutPrivate to ensure that this new semantic is correctly handled.

[Yusuke Suzuki]

Comment on attachment 408180 [details]
Patch

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

Commented, I think the change is being really solid. I think we should make PutByIdStatus / PutByIdVariant / ComplexPutById etc. robust against private name things, and after that the change looks good :)

> Source/JavaScriptCore/dfg/DFGNode.h:2298
> +        if (op() == PutPrivateName || op() == PutPrivateNameById)

I think you can just remove this and return `return PrivateFieldPutKind::fromByte(m_opInfo2.as<uint8_t>());` since we have `ASSERT(hasPrivateFieldPutKind());`.

> Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:160
> +        return operationPutByIdDefinePrivateFieldStrictOptimize;

Let's make this function switch. This will prevent us from missing new m_putKind handling. m_putKind did not have many kinds. But now it has. If the enum has various kinds, switch is better for readability & avoids the bug missing handling some values.

switch (m_putKind) {
case DirectPrivateFieldSet: {
    ASSERT(m_ecmaMode.isStrict());
    return operationPutByIdSetPrivateFieldStrictOptimize;
}
case DirectPrivateFieldDefine: {
    ASSERT(m_ecmaMode.isStrict());
    return operationPutByIdDefinePrivateFieldStrictOptimize;
}
case Direct: {
    if (m_ecmaMode.isStrict())
        return operationPutByIdDirectStrictOptimize;
    return operationPutByIdDirectNonStrictOptimize;
}
case NonDirect: {
    if (m_ecmaMode.isStrict())
        return operationPutByIdStrictOptimize;
    return operationPutByIdNonStrictOptimize;
}
}

> Source/JavaScriptCore/jit/JITOperations.cpp:1188
> +        baseObject->putPrivateField(globalObject, propertyName, value, slot);

I suggest renaming putPrivateField to setPrivateField.

> Source/JavaScriptCore/jit/JITOperations.cpp:1213
> +    baseObject->putPrivateField(globalObject, propertyName, value, slot);

I suggest renaming putPrivateField to setPrivateField.

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:-1197
> -    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();
> -        }
> -    }
> -

I think it is worth keeping it. Let's keep it for now.

[Caio Lima]

Comment on attachment 408180 [details]
Patch

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

>> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:-1197
>> -
> 
> I think it is worth keeping it. Let's keep it for now.

I don't see how this path adds anything that previous `subscript.tryGetAsUint32Index()` is covering. From `subscript.tryGetAsUint32Index` we already check if it `isNumber` and also checks if it's a valid `uint32_t` and is index. @caitp do you remember why you added it on https://trac.webkit.org/changeset/262613 ?

[Caio Lima]

Created attachment 409045 [details]
Patch

[Caio Lima]

Created attachment 409053 [details]
Patch

[Caio Lima]

Created attachment 409281 [details]
Patch

[Caio Lima]

Created attachment 409344 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 409344 [details]
Patch

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

r=me

> Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:170
> +    default:
> +        RELEASE_ASSERT_NOT_REACHED();
> +        return nullptr;

Let's remove this default clause. To make it possible, let's make PutKind enum class. Then, we can purge this default clause. This make it possible that future extension of PutKind will warn at compile time.

> Source/JavaScriptCore/jit/Repatch.cpp:555
> +    default:
> +        RELEASE_ASSERT_NOT_REACHED();
> +        return nullptr;

Ditto.

> Source/JavaScriptCore/jit/Repatch.cpp:578
> +    default:
> +        RELEASE_ASSERT_NOT_REACHED(); 
> +        return nullptr;

Ditto.

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1189
>      if (subscript.isDouble()) {
> -        ASSERT(!isPrivateFieldAccess);
>          double subscriptAsDouble = subscript.asDouble();
>          uint32_t subscriptAsUInt32 = static_cast<uint32_t>(subscriptAsDouble);
>          if (subscriptAsDouble == subscriptAsUInt32 && isIndex(subscriptAsUInt32)) {

OK, now I understand. `subscript.tryGetAsUint32Index` subsumes this branch. So, this is dead code. Then we can remove it.

> Source/JavaScriptCore/runtime/PrivateFieldPutKind.cpp:39
> +    if (isSet())
> +        out.print("Set");
> +    else
> +        out.print("Define");

Let's support None too.

[Caio Lima]

Created attachment 409429 [details]
Patch

[Caio Lima]

Created attachment 409441 [details]
Patch

[Caio Lima]

Created attachment 409450 [details]
Patch

[Caio Lima]

Created attachment 409480 [details]
Patch

[Caio Lima]

Comment on attachment 409480 [details]
Patch

Thank you very much for the review!

[EWS]

Committed r267489: <https://trac.webkit.org/changeset/267489>

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

[Radar WebKit Bug Importer]

<rdar://problem/69443132>

[Caio Lima]

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

[Caio Lima]

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