Consider the following code: ```js class C extends Object {} delete Object.prototype.__proto__ let c = new C ``` WebKit Nightly throws `TypeError: undefined is not a constructor`. However, `super` should call internal `[[GetPrototypeOf]]`, not the `__proto__` getter. This code works fine both in Chrome Canary and Firefox.
Created attachment 400002 [details] Patch
(In reply to Alexey Shvayka from comment #1) > Created attachment 400002 [details] > Patch With warm-up runs, --outer 128: r262029 patch super-get-by-id-with-this-monomorphic 22.6679+-0.4237 ^ 20.5501+-0.3550 ^ definitely 1.1031x faster super-get-by-id-with-this-polymorphic 17.9731+-0.2928 17.5325+-0.2612 might be 1.0251x faster super-get-by-val-with-this-monomorphic 24.4027+-1.1266 ^ 22.5436+-0.7088 ^ definitely 1.0825x faster super-get-by-val-with-this-polymorphic 21.3870+-1.2635 20.6429+-0.6245 might be 1.0360x faster super-getter 16.4953+-0.2982 ? 16.6465+-0.3252 ?
Created attachment 400048 [details] Patch Fix 32-bit build and handle JSFunctionType by JIT fast path.
Created attachment 400051 [details] Patch Fix 32-bit build.
Created attachment 400063 [details] Patch Drop emitLoadStructure() and use OBJECT_OFFSETOF for 32-bit targets.
Created attachment 400464 [details] Patch Drop baseline JIT fast path for 32-bit targets.
Comment on attachment 400464 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400464&action=review > Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:1307 > + addSlowCase(jump()); If we compile this op properly (see prev. patch), stress/arrowfunction-lexical-bind-supercall-2.js segfaults on ARM & MIPS EWS. I couldn't reproduce segfaults locally on both 32-bit Intel and ARMv7 (via qemu), yet I think it is fine to slow-case it unconditionally.
Comment on attachment 400464 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400464&action=review > Source/JavaScriptCore/ChangeLog:12 > + This patch defines OpGetPrototypeOf for LLInt and baseline JIT, ensuring makes sense > Source/JavaScriptCore/jit/JITOpcodes.cpp:1534 > + addSlowCase(branchIfNotType(regT0, FinalObjectType, JSFunctionType)); This feels like a precarious implementation without having some kind of assert this is true. Also, couldn't we just inline the fast path case here by looking at the class info function ptr? Like the JSObject implementation: ``` MethodTable::GetPrototypeFunctionPtr defaultGetPrototype = JSObject::getPrototype; if (LIKELY(getPrototypeMethod == defaultGetPrototype)) return getPrototypeDirect(vm); ``` This can be done in a followup. We should probably also inline it in the DFG/FTL Maybe we could add an out of line type info bit to make it even faster? >> Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:1307 >> + addSlowCase(jump()); > > If we compile this op properly (see prev. patch), stress/arrowfunction-lexical-bind-supercall-2.js segfaults on ARM & MIPS EWS. > I couldn't reproduce segfaults locally on both 32-bit Intel and ARMv7 (via qemu), yet I think it is fine to slow-case it unconditionally. you can just make this a SLOW_CASE_OP (or whatever the macro is) in the switch statement for 32-bit and then you don't need to implement this. > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1766 > +slowPathOp(get_prototype_of) let's implement a fast path, at least for 64-bit
Created attachment 400749 [details] Patch Add LLInt fast path (64-bit only), add bytecode intrinsic & test, and introduce OverridesGetPrototype out of line flag.
(In reply to Saam Barati from comment #8) Thank you for detailed comments, Saam. > Also, couldn't we just inline the fast path case here by looking at the > class info function ptr? Comparing GetPrototypeFunctionPtr led to 70% regression (w/o DFG) for microbenchmarks/super-getter.js and ~10% for other `super` microbenchmarks. Code I was testing: ``` addSlowCase(branchIfNotCell(regT0)); emitLoadStructure(vm(), regT0, regT2, regT1); static constexpr MethodTable::GetPrototypeFunctionPtr defaultGetPrototype = JSObject::getPrototype; static ptrdiff_t getPrototypeOffset = Structure::classInfoOffset() + ClassInfo::offsetOfMethodTable() + MethodTable::offsetOfGetPrototype(); addSlowCase(branchPtr(NotEqual, Address(regT2, getPrototypeOffset), TrustedImmPtr(defaultGetPrototype))); ``` So I went with out of line flag, which is performance-neutral and a nice refactor. > We should probably also inline it in the DFG/FTL Looks like DFG already inlines GetPrototypeOf, please see the changed bit in DFGAbstractInterpreterInlines.h.
Created attachment 400794 [details] Patch Fix tests and 32-bit build, use JSValue::getPrototype() in objectConstructorGetPrototypeOf().
Comment on attachment 400794 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400794&action=review With this change, Object.getPrototypeOf() uses synthesizePrototype() for primitives instead of allocating wrapper object. --outer 128: trunk patch object-get-prototype-of-primitive 111.1243+-1.0766 ^ 79.5175+-0.7592 ^ definitely 1.3975x faster > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1492 > + const OverridesGetPrototype = 0x400 # (1 << 18) >> 8 We can improve this by tweaking LLInt parser to support bitwise shifts in a follow-up.
Comment on attachment 400794 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400794&action=review r=me with a few comments > Source/JavaScriptCore/jit/JITOpcodes.cpp:1528 > +void JIT::emit_op_get_prototype_of(const Instruction* currentInstruction) Can we also add fast paths for DFG/FTL that look like this? Maybe in a follow-up? > Source/JavaScriptCore/jit/JITOpcodes.cpp:1537 > + addSlowCase(branchTest32(NonZero, Address(regT2, Structure::outOfLineTypeFlagsOffset()), TrustedImm32(OverridesGetPrototype >> 8))); nit: can we define 8 somewhere in JSTypeInfo like "numberOfInlineBits"? >> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1492 >> + const OverridesGetPrototype = 0x400 # (1 << 18) >> 8 > > We can improve this by tweaking LLInt parser to support bitwise shifts in a follow-up. constexpr expressions in LLInt doesn't work here? If not, I'd prefer to define this in the JSTypeInfo header and just use the value here, and we can static assert it's proper there? Could also be used in baseline JIT code for this. > Source/JavaScriptCore/runtime/JSTypeInfo.h:60 > +static constexpr unsigned OverridesGetPrototype = 1 << 18; Can you add validation for this into the new Structure::validateFlags that was added after you uploaded this change.
Created attachment 401781 [details] Patch Define numberOfInlineBits and OverridesGetPrototypeOutOfLine, add overridesGetPrototype() validation.
(In reply to Saam Barati from comment #13) > Can we also add fast paths for DFG/FTL that look like this? Maybe in a follow-up? Looks like we already have those: please see DFGSpeculativeJIT.cpp/compileGetPrototypeOf() and FTLLowerDFGToB3.cpp/compileGetPrototypeOf(). LLInt and baseline JIT fast paths are inspired by DFG one. > constexpr expressions in LLInt doesn't work here? constexpr does work, but no the bit shifts.
Comment on attachment 401781 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401781&action=review > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1511 > + btqnz t0, notCellMask, .opGetPrototypeOfSlow > + bbb JSCell::m_type[t0], ObjectType, .opGetPrototypeOfSlow > + > + loadStructureWithScratch(t0, t2, t1, t3) > + btinz Structure::m_outOfLineTypeFlags[t2], OverridesGetPrototypeOutOfLine, .opGetPrototypeOfSlow > + > + loadq Structure::m_prototype[t2], t2 > + btqz t2, .opGetPrototypeOfPolyProto > + return(t2) > + Ah, no. I think what Saam says is adding these fast path for void SpeculativeJIT::compileGetPrototypeOf(Node* node)'s ObjectUse and generic cases. 13945 case ObjectUse: { 13946 SpeculateCellOperand value(this, node->child1()); 13947 JSValueRegsTemporary result(this); 13948 13949 GPRReg valueGPR = value.gpr(); 13950 JSValueRegs resultRegs = result.regs(); 13951 13952 speculateObject(node->child1(), valueGPR); 13953 13954 flushRegisters(); 13955 callOperation(operationGetPrototypeOfObject, resultRegs, TrustedImmPtr::weakPointer(m_graph, m_graph.globalObjectFor(node->origin.semantic)), valueGPR); 13956 m_jit.exceptionCheck(); 13957 jsValueResult(resultRegs, node); 13958 return; 13959 } 13960 default: { 13961 JSValueOperand value(this, node->child1()); 13962 JSValueRegsTemporary result(this); 13963 13964 JSValueRegs valueRegs = value.jsValueRegs(); 13965 JSValueRegs resultRegs = result.regs(); 13966 13967 flushRegisters(); 13968 callOperation(operationGetPrototypeOf, resultRegs, TrustedImmPtr::weakPointer(m_graph, m_graph.globalObjectFor(node->origin.semantic)), valueRegs); 13969 m_jit.exceptionCheck(); 13970 jsValueResult(resultRegs, node); 13971 return; 13972 } We do not have this. And please add it to FTL too.
(In reply to Yusuke Suzuki from comment #16) > Comment on attachment 401781 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=401781&action=review > > > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1511 > > + btqnz t0, notCellMask, .opGetPrototypeOfSlow > > + bbb JSCell::m_type[t0], ObjectType, .opGetPrototypeOfSlow > > + > > + loadStructureWithScratch(t0, t2, t1, t3) > > + btinz Structure::m_outOfLineTypeFlags[t2], OverridesGetPrototypeOutOfLine, .opGetPrototypeOfSlow > > + > > + loadq Structure::m_prototype[t2], t2 > > + btqz t2, .opGetPrototypeOfPolyProto > > + return(t2) > > + > > Ah, no. I think what Saam says is adding these fast path for void > SpeculativeJIT::compileGetPrototypeOf(Node* node)'s ObjectUse and generic > cases. > > > 13945 case ObjectUse: { > 13946 SpeculateCellOperand value(this, node->child1()); > 13947 JSValueRegsTemporary result(this); > 13948 > 13949 GPRReg valueGPR = value.gpr(); > 13950 JSValueRegs resultRegs = result.regs(); > 13951 > 13952 speculateObject(node->child1(), valueGPR); > 13953 > 13954 flushRegisters(); > 13955 callOperation(operationGetPrototypeOfObject, resultRegs, > TrustedImmPtr::weakPointer(m_graph, > m_graph.globalObjectFor(node->origin.semantic)), valueGPR); > 13956 m_jit.exceptionCheck(); > 13957 jsValueResult(resultRegs, node); > 13958 return; > 13959 } > 13960 default: { > 13961 JSValueOperand value(this, node->child1()); > 13962 JSValueRegsTemporary result(this); > 13963 > 13964 JSValueRegs valueRegs = value.jsValueRegs(); > 13965 JSValueRegs resultRegs = result.regs(); > 13966 > 13967 flushRegisters(); > 13968 callOperation(operationGetPrototypeOf, resultRegs, > TrustedImmPtr::weakPointer(m_graph, > m_graph.globalObjectFor(node->origin.semantic)), valueRegs); > 13969 m_jit.exceptionCheck(); > 13970 jsValueResult(resultRegs, node); > 13971 return; > 13972 } > > We do not have this. And please add it to FTL too. Yeah this is what I meant
I’m ok if it’s a follow up as well.
Comment on attachment 401781 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401781&action=review >>> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1511 >>> + >> >> Ah, no. I think what Saam says is adding these fast path for void SpeculativeJIT::compileGetPrototypeOf(Node* node)'s ObjectUse and generic cases. >> >> >> 13945 case ObjectUse: { >> 13946 SpeculateCellOperand value(this, node->child1()); >> 13947 JSValueRegsTemporary result(this); >> 13948 >> 13949 GPRReg valueGPR = value.gpr(); >> 13950 JSValueRegs resultRegs = result.regs(); >> 13951 >> 13952 speculateObject(node->child1(), valueGPR); >> 13953 >> 13954 flushRegisters(); >> 13955 callOperation(operationGetPrototypeOfObject, resultRegs, TrustedImmPtr::weakPointer(m_graph, m_graph.globalObjectFor(node->origin.semantic)), valueGPR); >> 13956 m_jit.exceptionCheck(); >> 13957 jsValueResult(resultRegs, node); >> 13958 return; >> 13959 } >> 13960 default: { >> 13961 JSValueOperand value(this, node->child1()); >> 13962 JSValueRegsTemporary result(this); >> 13963 >> 13964 JSValueRegs valueRegs = value.jsValueRegs(); >> 13965 JSValueRegs resultRegs = result.regs(); >> 13966 >> 13967 flushRegisters(); >> 13968 callOperation(operationGetPrototypeOf, resultRegs, TrustedImmPtr::weakPointer(m_graph, m_graph.globalObjectFor(node->origin.semantic)), valueRegs); >> 13969 m_jit.exceptionCheck(); >> 13970 jsValueResult(resultRegs, node); >> 13971 return; >> 13972 } >> >> We do not have this. And please add it to FTL too. > > Yeah this is what I meant So can you add a FIXME with bugzilla URL?
Committed r263035: <https://trac.webkit.org/changeset/263035>
<rdar://problem/64362505>
Comment on attachment 401781 [details] Patch (In reply to Yusuke Suzuki from comment #19) > So can you add a FIXME with bugzilla URL? Asked Yusuke offline: I will add DFG/FTL fast paths in https://bugs.webkit.org/show_bug.cgi?id=213191 since it requires additional tests and microbenchmarks. Landed with a FIXME.
(In reply to Alexey Shvayka from comment #20) > Committed r263035: <https://trac.webkit.org/changeset/263035> This change appears to have broken the CLoop build: /Volumes/Data/slave/catalina-cloop-debug/build/Source/JavaScriptCore/offlineasm/cloop.rb:649:in `lowerC_LOOP': undefined method `uint32MemRef' for #<Immediate:0x00007f8fb1bc5fa0> (due to JavaScriptCore/llint/LowLevelInterpreter64.asm:1517) (LoweringError) https://build.webkit.org/builders/Apple-Catalina-LLINT-CLoop-BuildAndTest/builds/4871/steps/compile-webkit/logs/stdio
Comment on attachment 401781 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401781&action=review > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1517 > + loadi knownPolyProtoOffset, t1 This is wrong. It should be mov.
Committed r263043: <https://trac.webkit.org/changeset/263043>
(In reply to Yusuke Suzuki from comment #25) > Committed r263043: <https://trac.webkit.org/changeset/263043> This code in general is a little silly that it's using a macro that branches on a known offset. We should just load directly from the object. Maybe we can split a new macro called "loadInlineOffset" or something macro loadPropertyAtVariableOffset(propertyOffsetAsInt, objectAndStorage, value) bilt propertyOffsetAsInt, firstOutOfLineOffset, .isInline loadp JSObject::m_butterfly[objectAndStorage], objectAndStorage negi propertyOffsetAsInt sxi2q propertyOffsetAsInt, propertyOffsetAsInt jmp .ready .isInline: addp sizeof JSObject - (firstOutOfLineOffset - 2) * 8, objectAndStorage .ready: loadq (firstOutOfLineOffset - 2) * 8[objectAndStorage, propertyOffsetAsInt, 8], value end
This change regressed JSC stress tests on s390x. I'm not sure why. See bug #213307.