Unreviewed, reverting r263443@main.

https://bugs.webkit.org/show_bug.cgi?id=256050 Speedometer2.1 is failing Reverted changeset: "[JSC] Skip ProxyObject's ICs trap result validation in the common case" https://bugs.webkit.org/show_bug.cgi?id=255661 https://commits.webkit.org/263443@main Canonical link: https://commits.webkit.org/263461@main
WebKit · Apr 27, 2023 · feeb214 · feeb214
1 parent 6723dcf
commit feeb214
Show file tree

Hide file tree

Showing 53 changed files with 65 additions and 308 deletions.
diff --git a/JSTests/microbenchmarks/proxy-get-miss-handler.js b/JSTests/microbenchmarks/proxy-get-miss-handler.js
@@ -1,6 +1,4 @@
-(function() {
 var proxy = new Proxy({}, {});
 
 for (var i = 0; i < 1e7; ++i)
     proxy.test;
-})();
diff --git a/JSTests/microbenchmarks/proxy-get.js b/JSTests/microbenchmarks/proxy-get.js
@@ -1,4 +1,3 @@
-(function() {
 var proxy = new Proxy({}, {
     get(target, propertyName, receiver) {
         return 42;
@@ -7,4 +6,3 @@ var proxy = new Proxy({}, {
 
 for (var i = 0; i < 1e7; ++i)
     proxy.test;
-})();
diff --git a/JSTests/microbenchmarks/proxy-has-hit.js b/JSTests/microbenchmarks/proxy-has-hit.js
@@ -6,6 +6,6 @@
     });
 
     var has;
-    for (var i = 0; i < 1e7; ++i)
+    for (var i = 0; i < 1e6; ++i)
         has = "test" in proxy;
 })();
diff --git a/JSTests/microbenchmarks/proxy-has-miss-handler.js b/JSTests/microbenchmarks/proxy-has-miss-handler.js
@@ -2,6 +2,6 @@
     var proxy = new Proxy({}, {});
 
     var has;
-    for (var i = 0; i < 1e7; ++i)
+    for (var i = 0; i < 1e6; ++i)
         has = "test" in proxy;
 })();
diff --git a/JSTests/microbenchmarks/proxy-has-miss.js b/JSTests/microbenchmarks/proxy-has-miss.js
@@ -6,6 +6,6 @@
     });
 
     var has;
-    for (var i = 0; i < 1e7; ++i)
+    for (var i = 0; i < 1e6; ++i)
         has = "test" in proxy;
 })();
diff --git a/JSTests/microbenchmarks/proxy-set-miss-handler.js b/JSTests/microbenchmarks/proxy-set-miss-handler.js
@@ -1,6 +1,6 @@
 (function() {
     var proxy = new Proxy({}, {});
 
-    for (var i = 0; i < 1e7; ++i)
+    for (var i = 0; i < 1e6; ++i)
         proxy.test = i;
 })();
diff --git a/JSTests/microbenchmarks/proxy-set.js b/JSTests/microbenchmarks/proxy-set.js
@@ -5,6 +5,6 @@
         }
     });
 
-    for (var i = 0; i < 1e7; ++i)
+    for (var i = 0; i < 1e6; ++i)
         proxy.test = i;
 })();
diff --git a/Source/JavaScriptCore/builtins/BuiltinNames.h b/Source/JavaScriptCore/builtins/BuiltinNames.h
@@ -182,8 +182,9 @@ namespace JSC {
     macro(stringSplitFast) \
     macro(stringSubstring) \
     macro(handleNegativeProxyHasTrapResult) \
-    macro(handlePositiveProxySetTrapResult) \
     macro(handleProxyGetTrapResult) \
+    macro(handleProxySetTrapResultSloppy) \
+    macro(handleProxySetTrapResultStrict) \
     macro(importModule) \
     macro(copyDataProperties) \
     macro(meta) \

diff --git a/Source/JavaScriptCore/builtins/ProxyHelpers.js b/Source/JavaScriptCore/builtins/ProxyHelpers.js
@@ -44,9 +44,7 @@ function performProxyObjectHas(propertyName)
     if (trap.@call(handler, target, propertyName))
         return true;
 
-    if (@mustValidateResultOfProxyTrapsExceptGetAndSet(target))
-        @handleNegativeProxyHasTrapResult(target, propertyName);
-
+    @handleNegativeProxyHasTrapResult(target, propertyName);
     return false;
 }
 
@@ -70,8 +68,9 @@ function performProxyObjectGet(propertyName, receiver)
 
     var trapResult = trap.@call(handler, target, propertyName, receiver);
 
-    if (@mustValidateResultOfProxyGetAndSetTraps(target))
-        @handleProxyGetTrapResult(trapResult, target, propertyName);
+    // FIXME: Add op_get_own_property bytecode and IC, which returns two values, value and attributes.
+    // Then we can implement it fully in JS.
+    @handleProxyGetTrapResult(trapResult, target, propertyName);
 
     return trapResult;
 }
@@ -96,11 +95,9 @@ function performProxyObjectSetSloppy(propertyName, receiver, value)
     if (!@isCallable(trap))
         @throwTypeError("'set' property of a Proxy's handler should be callable");
 
-    if (!trap.@call(handler, target, propertyName, value, receiver))
-        return;
+    var trapResult = trap.@call(handler, target, propertyName, value, receiver);
 
-    if (@mustValidateResultOfProxyGetAndSetTraps(target))
-        @handlePositiveProxySetTrapResult(target, propertyName, value);
+    @handleProxySetTrapResultSloppy(trapResult, target, propertyName, value);
 }
 
 @linkTimeConstant
@@ -123,9 +120,7 @@ function performProxyObjectSetStrict(propertyName, receiver, value)
     if (!@isCallable(trap))
         @throwTypeError("'set' property of a Proxy's handler should be callable");
 
-    if (!trap.@call(handler, target, propertyName, value, receiver))
-        @throwTypeError("Proxy object's 'set' trap returned falsy value for property '" + @String(propertyName) + "'");
+    var trapResult = trap.@call(handler, target, propertyName, value, receiver);
 
-    if (@mustValidateResultOfProxyGetAndSetTraps(target))
-        @handlePositiveProxySetTrapResult(target, propertyName, value);
+    @handleProxySetTrapResultStrict(trapResult, target, propertyName, value);
 }
diff --git a/Source/JavaScriptCore/bytecode/BytecodeIntrinsicRegistry.h b/Source/JavaScriptCore/bytecode/BytecodeIntrinsicRegistry.h
@@ -98,8 +98,6 @@ enum class LinkTimeConstant : int32_t;
     macro(toString) \
     macro(toPropertyKey) \
     macro(toObject) \
-    macro(mustValidateResultOfProxyGetAndSetTraps) \
-    macro(mustValidateResultOfProxyTrapsExceptGetAndSet) \
     macro(newArrayWithSize) \
     macro(newArrayWithSpecies) \
     macro(newPromise) \

diff --git a/Source/JavaScriptCore/bytecode/BytecodeList.rb b/Source/JavaScriptCore/bytecode/BytecodeList.rb
@@ -1383,13 +1383,6 @@
         type: JSType,
     }
 
-op :has_structure_with_flags,
-    args: {
-        dst: VirtualRegister,
-        operand: VirtualRegister,
-        flags: unsigned,
-    }
-
 end_section :Bytecode
 
 begin_section :CLoopHelpers,

diff --git a/Source/JavaScriptCore/bytecode/BytecodeUseDef.cpp b/Source/JavaScriptCore/bytecode/BytecodeUseDef.cpp
@@ -227,7 +227,6 @@ void computeUsesForBytecodeIndexImpl(const JSInstruction* instruction, Checkpoin
     USES(OpInByVal, base, property)
     USES(OpHasPrivateName, base, property)
     USES(OpHasPrivateBrand, base, brand)
-    USES(OpHasStructureWithFlags, operand)
     USES(OpOverridesHasInstance, constructor, hasInstanceValue)
     USES(OpInstanceof, value, prototype)
     USES(OpAdd, lhs, rhs)
@@ -506,7 +505,6 @@ void computeDefsForBytecodeIndexImpl(unsigned numVars, const JSInstruction* inst
     DEFS(OpInByVal, dst)
     DEFS(OpHasPrivateName, dst)
     DEFS(OpHasPrivateBrand, dst)
-    DEFS(OpHasStructureWithFlags, dst)
     DEFS(OpToNumber, dst)
     DEFS(OpToNumeric, dst)
     DEFS(OpToString, dst)

diff --git a/Source/JavaScriptCore/bytecode/LinkTimeConstant.h b/Source/JavaScriptCore/bytecode/LinkTimeConstant.h
@@ -109,8 +109,9 @@ class JSGlobalObject;
     v(stringSplitFast, nullptr) \
     v(stringSubstring, nullptr) \
     v(handleNegativeProxyHasTrapResult, nullptr) \
-    v(handlePositiveProxySetTrapResult, nullptr) \
     v(handleProxyGetTrapResult, nullptr) \
+    v(handleProxySetTrapResultSloppy, nullptr) \
+    v(handleProxySetTrapResultStrict, nullptr) \
     v(dateTimeFormat, nullptr) \
     v(webAssemblyCompileStreamingInternal, nullptr) \
     v(webAssemblyInstantiateStreamingInternal, nullptr) \

diff --git a/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp b/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp
@@ -2835,12 +2835,6 @@ RegisterID* BytecodeGenerator::emitHasPrivateName(RegisterID* dst, RegisterID* b
     return dst;
 }
 
-RegisterID* BytecodeGenerator::emitHasStructureWithFlags(RegisterID* dst, RegisterID* src, unsigned flags)
-{
-    OpHasStructureWithFlags::emit(this, dst, src, flags);
-    return dst;
-}
-
 RegisterID* BytecodeGenerator::emitDirectPutByVal(RegisterID* base, RegisterID* property, RegisterID* value)
 {
     OpPutByValDirect::emit(this, base, property, value, ecmaMode());

diff --git a/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h b/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h
@@ -780,7 +780,6 @@ namespace JSC {
         RegisterID* emitPrivateFieldPut(RegisterID* base, RegisterID* property, RegisterID* value);
         RegisterID* emitGetPrivateName(RegisterID* dst, RegisterID* base, RegisterID* property);
         RegisterID* emitHasPrivateName(RegisterID* dst, RegisterID* base, RegisterID* property);
-        RegisterID* emitHasStructureWithFlags(RegisterID* dst, RegisterID* src, unsigned flags);
 
         void emitCreatePrivateBrand(const JSTextPosition& divot, const JSTextPosition& divotStart, const JSTextPosition& divotEnd);
         void emitInstallPrivateBrand(RegisterID* target);

diff --git a/Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp b/Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp
@@ -2027,24 +2027,6 @@ CREATE_INTRINSIC_FOR_BRAND_CHECK(isUndefinedOrNull, IsUndefinedOrNull)
 
 #undef CREATE_INTRINSIC_FOR_BRAND_CHECK
 
-RegisterID* BytecodeIntrinsicNode::emit_intrinsic_mustValidateResultOfProxyGetAndSetTraps(BytecodeGenerator& generator, RegisterID* dst)
-{
-    ArgumentListNode* node = m_args->m_listNode;
-    RefPtr<RegisterID> src = generator.emitNode(node);
-    ASSERT(!node->m_next);
-
-    return generator.move(dst, generator.emitHasStructureWithFlags(generator.tempDestination(dst), src.get(), Structure::s_hasNonConfigurableReadOnlyOrGetterSetterPropertiesBits));
-}
-
-RegisterID* BytecodeIntrinsicNode::emit_intrinsic_mustValidateResultOfProxyTrapsExceptGetAndSet(BytecodeGenerator& generator, RegisterID* dst)
-{
-    ArgumentListNode* node = m_args->m_listNode;
-    RefPtr<RegisterID> src = generator.emitNode(node);
-    ASSERT(!node->m_next);
-
-    return generator.move(dst, generator.emitHasStructureWithFlags(generator.tempDestination(dst), src.get(), Structure::s_hasNonConfigurablePropertiesBits | Structure::s_didPreventExtensionsBits));
-}
-
 RegisterID* BytecodeIntrinsicNode::emit_intrinsic_newArrayWithSize(JSC::BytecodeGenerator& generator, JSC::RegisterID* dst)
 {
     ArgumentListNode* node = m_args->m_listNode;

diff --git a/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h b/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
@@ -4845,18 +4845,6 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
         break;
     }
 
-    case HasStructureWithFlags: {
-        const AbstractValue& child = forNode(node->child1());
-        unsigned flags = node->structureFlags();
-        ASSERT(flags);
-
-        if (Structure::bitFieldFlagsCantBeChangedWithoutTransition(flags) && child.m_type && !(child.m_type & ~SpecCell) && child.m_structure.isFinite())
-            m_state.setShouldTryConstantFolding(true);
-
-        setNonCellTypeForNode(node, SpecBoolean);
-        break;
-    }
-
     case ParseInt: {
         AbstractValue value = forNode(node->child1());
         if (value.m_type && !(value.m_type & ~SpecInt32Only)) {

diff --git a/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp b/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
@@ -6624,13 +6624,6 @@ void ByteCodeParser::parseBlock(unsigned limit)
             NEXT_OPCODE(op_is_cell_with_type);
         }
 
-        case op_has_structure_with_flags: {
-            auto bytecode = currentInstruction->as<OpHasStructureWithFlags>();
-            Node* object = get(bytecode.m_operand);
-            set(bytecode.m_dst, addToGraph(HasStructureWithFlags, OpInfo(bytecode.m_flags), object));
-            NEXT_OPCODE(op_has_structure_with_flags);
-        }
-
         case op_is_object: {
             auto bytecode = currentInstruction->as<OpIsObject>();
             Node* value = get(bytecode.m_operand);

diff --git a/Source/JavaScriptCore/dfg/DFGClobberize.h b/Source/JavaScriptCore/dfg/DFGClobberize.h
@@ -1252,10 +1252,6 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
         def(HeapLocation(CheckTypeInfoFlagsLoc, JSCell_typeInfoFlags, node->child1()), LazyNode(node));
         return;
 
-    case HasStructureWithFlags:
-        read(World);
-        return;
-
     case ParseInt:
         // Note: We would have eliminated a ParseInt that has just a single child as an Int32Use inside fixup.
         if (node->child1().useKind() == StringUse || node->child1().useKind() == DoubleRepUse || node->child1().useKind() == Int32Use) {

diff --git a/Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp b/Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp
@@ -1121,35 +1121,6 @@ class ConstantFoldingPhase : public Phase {
 
                 break;
             }
-
-            case HasStructureWithFlags: {
-                const AbstractValue& child = m_state.forNode(node->child1());
-                unsigned flags = node->structureFlags();
-                ASSERT(flags);
-
-                if (Structure::bitFieldFlagsCantBeChangedWithoutTransition(flags) && child.m_type && !(child.m_type & ~SpecCell) && child.m_structure.isFinite()) {
-                    bool canFoldToTrue = true;
-                    bool canFoldToFalse = true;
-
-                    child.m_structure.forEach([&] (RegisteredStructure structure) {
-                        bool notDictionary = !structure->isDictionary();
-                        bool hasAnyOfBitFieldFlags = structure->hasAnyOfBitFieldFlags(flags);
-
-                        canFoldToTrue &= notDictionary && hasAnyOfBitFieldFlags;
-                        canFoldToFalse &= notDictionary && !hasAnyOfBitFieldFlags;
-                    });
-
-                    if (canFoldToTrue) {
-                        m_graph.convertToConstant(node, jsBoolean(true));
-                        changed = true;
-                    } else if (canFoldToFalse) {
-                        m_graph.convertToConstant(node, jsBoolean(false));
-                        changed = true;
-                    }
-                }
-
-                break;
-            }
 
             case PhantomNewObject:
             case PhantomNewFunction:

diff --git a/Source/JavaScriptCore/dfg/DFGDoesGC.cpp b/Source/JavaScriptCore/dfg/DFGDoesGC.cpp
@@ -187,7 +187,6 @@ bool doesGC(Graph& graph, Node* node)
     case Check:
     case CheckVarargs:
     case CheckTypeInfoFlags:
-    case HasStructureWithFlags:
     case MultiGetByOffset:
     case MultiDeleteByOffset:
     case ValueRep:

diff --git a/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp b/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
@@ -2384,11 +2384,6 @@ class FixupPhase : public Phase {
             break;
         }
 
-        case HasStructureWithFlags: {
-            fixEdge<KnownCellUse>(node->child1());
-            break;
-        }
-
         case HasIndexedProperty: {
             node->setArrayMode(
                 node->arrayMode().refine(

diff --git a/Source/JavaScriptCore/dfg/DFGGraph.cpp b/Source/JavaScriptCore/dfg/DFGGraph.cpp
@@ -289,8 +289,6 @@ void Graph::dump(PrintStream& out, const char* prefixStr, Node* node, DumpContex
     }
     if (node->hasQueriedType())
         out.print(comma, node->queriedType());
-    if (node->hasStructureFlags())
-        out.print(comma, node->structureFlags());
     if (node->hasStorageAccessData()) {
         StorageAccessData& storageAccessData = node->storageAccessData();
         out.print(comma, "id", storageAccessData.identifierNumber, "{", identifiers()[storageAccessData.identifierNumber], "}");

diff --git a/Source/JavaScriptCore/dfg/DFGNode.h b/Source/JavaScriptCore/dfg/DFGNode.h
@@ -1487,17 +1487,6 @@ struct Node {
     {
         return speculationFromJSType(queriedType());
     }
-
-    bool hasStructureFlags()
-    {
-        return op() == HasStructureWithFlags;
-    }
-
-    uint32_t structureFlags()
-    {
-        ASSERT(hasStructureFlags());
-        return m_opInfo.as<uint32_t>();
-    }
 
     bool hasResult()
     {

diff --git a/Source/JavaScriptCore/dfg/DFGNodeType.h b/Source/JavaScriptCore/dfg/DFGNodeType.h
@@ -418,7 +418,6 @@ namespace JSC { namespace DFG {
     \
     macro(IsCellWithType, NodeResultBoolean) \
     macro(IsEmpty, NodeResultBoolean) \
-    macro(HasStructureWithFlags, NodeResultBoolean) \
     macro(TypeOfIsUndefined, NodeResultBoolean) \
     macro(TypeOfIsObject, NodeResultBoolean) \
     macro(TypeOfIsFunction, NodeResultBoolean) \

diff --git a/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp b/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
@@ -1132,7 +1132,6 @@ class PredictionPropagationPhase : public Phase {
         case IsConstructor:
         case IsCellWithType:
         case IsTypedArrayView:
-        case HasStructureWithFlags:
         case MatchStructure: {
             setPrediction(SpecBoolean);
             break;

diff --git a/Source/JavaScriptCore/dfg/DFGSafeToExecute.h b/Source/JavaScriptCore/dfg/DFGSafeToExecute.h
@@ -272,7 +272,6 @@ bool safeToExecute(AbstractStateType& state, Graph& graph, Node* node, bool igno
     case IsConstructor:
     case IsCellWithType:
     case IsTypedArrayView:
-    case HasStructureWithFlags:
     case TypeOf:
     case ToBoolean:
     case LogicalNot:

diff --git a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
@@ -5969,20 +5969,6 @@ void SpeculativeJIT::compileIsTypedArrayView(Node* node)
     blessedBooleanResult(resultGPR, node);
 }
 
-void SpeculativeJIT::compileHasStructureWithFlags(Node* node)
-{
-    SpeculateCellOperand object(this, node->child1());
-    GPRTemporary result(this, Reuse, object);
-
-    GPRReg objectGPR = object.gpr();
-    GPRReg resultGPR = result.gpr();
-
-    emitLoadStructure(vm(), objectGPR, resultGPR);
-    test32(NonZero, Address(resultGPR, Structure::bitFieldOffset()), TrustedImm32(node->structureFlags()), resultGPR);
-
-    unblessedBooleanResult(resultGPR, node);
-}
-
 void SpeculativeJIT::compileToObjectOrCallObjectConstructor(Node* node)
 {
     RELEASE_ASSERT(node->child1().useKind() == UntypedUse);

diff --git a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h
@@ -1402,7 +1402,6 @@ class SpeculativeJIT : public JITCompiler {
 
     void compileCheckTypeInfoFlags(Node*);
     void compileCheckIdent(Node*);
-    void compileHasStructureWithFlags(Node*);
 
     void compileParseInt(Node*);