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.
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.
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
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.
Created attachment 403988 [details] WIP - Patch
Created attachment 404260 [details] WIP - Patch It adds DFG and FTL support for 32-bits and some tests.
Created attachment 404394 [details] WIP - Patch Patch is almost done, let's see EWS results
Created attachment 404459 [details] WIP - Patch
Created attachment 404563 [details] Patch
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`.
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
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.
Created attachment 404951 [details] WIP - Patch
Created attachment 405073 [details] Patch
Created attachment 405381 [details] Patch
Created attachment 405391 [details] Patch
Created attachment 405459 [details] Patch
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?
(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
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.
Created attachment 405922 [details] Patch
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
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?
(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).
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.
Created attachment 406381 [details] Patch
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.
Created attachment 406597 [details] Patch
Created attachment 406725 [details] Patch
Created attachment 407222 [details] Patch
Created attachment 407475 [details] Patch
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?
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
Created attachment 407884 [details] Patch
Created attachment 407964 [details] Patch
Created attachment 407972 [details] Patch
Created attachment 407977 [details] Patch
Created attachment 408180 [details] Patch
(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.
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?
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.
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.
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 ?
Created attachment 409045 [details] Patch
Created attachment 409053 [details] Patch
Created attachment 409281 [details] Patch
Created attachment 409344 [details] Patch
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.
Created attachment 409429 [details] Patch
Created attachment 409441 [details] Patch
Created attachment 409450 [details] Patch
Created attachment 409480 [details] Patch
Comment on attachment 409480 [details] Patch Thank you very much for the review!
Committed r267489: <https://trac.webkit.org/changeset/267489> All reviewed patches have been landed. Closing bug and clearing flags on attachment 409480 [details].
<rdar://problem/69443132>
*** Bug 214421 has been marked as a duplicate of this bug. ***
*** Bug 212784 has been marked as a duplicate of this bug. ***