Make inline caching work for deletes in the baseline jit.
Created attachment 390324 [details] Patch
Created attachment 390410 [details] Patch
Created attachment 390473 [details] Patch
Created attachment 390477 [details] Patch
Comment on attachment 390477 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390477&action=review > Source/JavaScriptCore/bytecode/AccessCase.cpp:1962 > + valueRegs, What’s in valueRegs? Why isn’t this just a store of the empty JSValue (zero)? > Source/JavaScriptCore/jit/JITOperations.cpp:2175 > + repatchDelBy(globalObject, codeBlock, baseValue, baseValue.structureOrNull(), propertyName, *stubInfo, DelByKind::Normal); Don’t we need something like PropertySlot/PutPropertySlot that tells us it’s ok to cache this? Custom implementations of delete might intercept this access > Source/JavaScriptCore/jit/Repatch.cpp:757 > + if (oldStructure->isDictionary()) Do we want to flatten and try again like other ICs? (If we haven’t flattened before) > Source/JavaScriptCore/jit/Repatch.cpp:760 > + if (!oldStructure->isObject()) What does delete do on strings/symbols/bigints? > Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:-143 > - RELEASE_AND_RETURN(scope, Base::put(thisObject, globalObject, ident, value, slot)); Why is this change required? > Source/JavaScriptCore/runtime/StructureInlines.h:231 > +inline bool Structure::mayHaveIndexingHeader(bool& mustCheckCell) const What does mustCheckCell mean here? What must be checked? > JSTests/ChangeLog:8 > + * microbenchmarks/delete-property-inline-cache-polymorphic.js: Added. Nit: All your tests are still not using WebKit style for spacing. “x<y”, “x=y” instead of “x < y”, “x = y” It would be nice to apply this to future patches too without me commenting.
Created attachment 390597 [details] Patch
Checking for test failures
Created attachment 390659 [details] Patch
Created attachment 390670 [details] Patch
(In reply to Saam Barati from comment #5) > Comment on attachment 390477 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=390477&action=review > > > Source/JavaScriptCore/bytecode/AccessCase.cpp:1962 > > + valueRegs, > > What’s in valueRegs? > Why isn’t this just a store of the empty JSValue (zero)? It is. We load it above. It seems to me that there is no option for storeValue that takes a constant value, so it has to get loaded first. > > > Source/JavaScriptCore/jit/Repatch.cpp:760 > > + if (!oldStructure->isObject()) > > What does delete do on strings/symbols/bigints? Seems like it calls the overridden implementation to me: https://tc39.es/ecma262/#sec-delete-operator bool StringObject::deleteProperty(JSCell* cell, JSGlobalObject* globalObject, PropertyName propertyName, DeletePropertySlot& slot) { VM& vm = globalObject->vm(); StringObject* thisObject = jsCast<StringObject*>(cell); if (propertyName == vm.propertyNames->length) return false; Optional<uint32_t> index = parseIndex(propertyName); if (index && thisObject->internalValue()->canGetIndex(index.value())) return false; return JSObject::deleteProperty(thisObject, globalObject, propertyName, slot); } > > > Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:-143 > > - RELEASE_AND_RETURN(scope, Base::put(thisObject, globalObject, ident, value, slot)); > > Why is this change required? My test uncovered this bug. We were missing an exception check. > > > Source/JavaScriptCore/runtime/StructureInlines.h:231 > > +inline bool Structure::mayHaveIndexingHeader(bool& mustCheckCell) const > > What does mustCheckCell mean here? What must be checked? > The cell itself must be used, not just the structure. In this case, it depends on some typed array indexing mode. inline bool Structure::hasIndexingHeader(const JSCell* cell) const { if (hasIndexedProperties(indexingType())) return true; if (!isTypedView(typedArrayTypeForType(m_blob.type()))) return false; return jsCast<const JSArrayBufferView*>(cell)->mode() == WastefulTypedArray; }
Created attachment 390690 [details] Patch
Created attachment 390837 [details] Patch
Comment on attachment 390837 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390837&action=review > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5176 > + CallSiteIndex callSiteIndex = Not sure if this is correct
Created attachment 390841 [details] Patch
Created attachment 390842 [details] Patch
Created attachment 390934 [details] Patch
Created attachment 390968 [details] Patch
Created attachment 391075 [details] Patch
Created attachment 391080 [details] Patch
Comment on attachment 391080 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391080&action=review > Source/JavaScriptCore/bytecode/AccessCase.h:151 > // This create method should be used for transitions. Can you delete this comment? > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1164 > + if (!m_state.forNode(node->child1()).isType(SpecCell)) > + slowCases.append(m_jit.branchIfNotCell(valueRegs)); We usually do this with a UseKind on the edge. That way nodes after the DelBy* can see that this node already did the speculation and not do it themselves. I also think you want to check if it's an Object since you don't have to do the check in the fast path. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1202 > + if (!m_state.forNode(node->child1()).isType(SpecCell)) > + slowCases.append(m_jit.branchIfNotCell(baseRegs)); > + if (!m_state.forNode(node->child2()).isType(SpecCell)) > + slowCases.append(m_jit.branchIfNotCell(keyRegs)); Ditto on UseKinds. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5151 > + auto base = lowJSValue(m_node->child1()); Ditto on UseKinds.
Created attachment 391349 [details] Patch
Created attachment 391350 [details] Patch
Created attachment 391360 [details] Patch
Comment on attachment 391360 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391360&action=review > JSTests/stress/delete-property-inline-cache.js:537 > +//for (let i = 0; i < 1000; ++i) { > +// testCacheableDeleteById() > +// testCacheableDeleteByVal() > +// testCacheableEmptyDeleteById() > +// testCacheableDeleteByIdMiss() > +// testDeleteIndex() > +// testPolymorphicDelByVal(i) > +// testBigintDeleteByVal() > +// testSymbolDeleteByVal() > +// testStrict() > +// testOverride() > +// testNonObject() > +// testNonObjectStrict() > +//} why is this commented out?
Regarding the armv7 crash. I ran stress/delete-by-id.js with --dumpDisassembly=true under gdb. I get a crash when trying to execute address 0x7196e388, which only contains 0s. We get there from this code: Generated JIT code for Access stub for test1#BOPmDU:[0x705b4340->0x705b40d0->0x705d0b90, DFGFunctionCall, 13 (NeverInline) (DidTryToEnterInLoop)] bc#7 with return point CodePtr(executable = 0x718fe325, dataLocation = 0x718fe324): Delete:(Generated, ident = 'cocoa', offset = 0, structure = 0x705fac60:[0x705fac60, Object, {cocoa:0, cappuccino:1}, NonArray, Proto:0x705ec000] -> 0x705facb0:[0x705facb0, Object, {cappuccino:1}, NonArray, Proto:0x705ec000, Leaf]): Code at [0x718fe561, 0x718fe5c1): 0x718fe560: ldr r6, [r0] 0x718fe562: movw ip, #0xac60 0x718fe566: movt ip, #0x705f 0x718fe56a: cmp.w r6, ip 0x718fe56e: ittt ne 0x718fe570: nopne 0x718fe572: nopne.w 0x718fe576: bne.w #0x718fe432 0x718fe57a: bl #0x7196e388 ^^^^^^^^^^^ That's probably not the address we want to branch to. Hope this helps tracking the issue.
Created attachment 391427 [details] Patch
Comment on attachment 391427 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391427&action=review > Source/JavaScriptCore/bytecode/AccessCase.cpp:131 > + RELEASE_ASSERT(oldStructure == newStructure->previousID()); Not related to your patch but why does Structure::previousID return the Structure*? Seems poorly named... > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4309 > + base.use(); > + key.use(); I don't think you need this if you don't have UseChildrenCalledExplicitly below. > Source/JavaScriptCore/jit/Repatch.cpp:748 > + if (!baseValue.isCell() || !oldStructure || !oldStructure->isObject() || !oldStructure->propertyAccessesAreCacheable()) Nit: I think this would be clearer as: ASSERT(oldStructure); if (!baseValue.isObject() || !oldStructure->propertyAccessesAreCachebale()) > Source/JavaScriptCore/runtime/JSONObject.cpp:764 > + DeletePropertySlot slot; > + object->methodTable(vm)->deleteProperty(object, m_globalObject, prop, slot); Nit: maybe we should have a version of deleteProperty that doesn't require a DeletePropertySlot similar to put and get. > Source/JavaScriptCore/runtime/JSObject.cpp:2040 > + return thisObject->methodTable(vm)->deleteProperty(thisObject, globalObject, Identifier::from(vm, i), slot); ditto. > Source/JavaScriptCore/runtime/JSObject.cpp:3636 > + object->methodTable(vm)->deleteProperty(object, globalObject, propertyName, slot); ditto. > Source/JavaScriptCore/runtime/JSObject.cpp:3653 > + object->methodTable(vm)->deleteProperty(object, globalObject, propertyName, slot); ditto. > Source/JavaScriptCore/runtime/JSObject.cpp:3678 > + object->methodTable(vm)->deleteProperty(object, globalObject, propertyName, slot); ditto. > Source/JavaScriptCore/runtime/JSObject.cpp:3732 > + object->methodTable(vm)->deleteProperty(object, globalObject, propertyName, slot); ditto. > Source/JavaScriptCore/runtime/Structure.cpp:-554 > - ASSERT(!isCompilationThread()); Why get rid of this?
Comment on attachment 391427 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391427&action=review r=me with some comments and a question: What happens if you have something like: let obj = {}; obj.foo = 1 obj.bar = 2; delete obj.foo; obj.baz = 3; delete obj.bar; delete obj.baz; Is obj's structure now the same as when it was first created? > Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:193 > + fastPath, slowPath, fastPath.locationOf<JITStubRoutinePtrTag>(m_start)); I think you can just replace m_start with `m_slowPathJump.m_jump.label()`? That way you don't need m_start at all. > Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:224 > + fastPath, slowPath, fastPath.locationOf<JITStubRoutinePtrTag>(m_start)); Ditto.
(In reply to Keith Miller from comment #28) > Comment on attachment 391427 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=391427&action=review > > r=me with some comments and a question: > > What happens if you have something like: > > let obj = {}; > obj.foo = 1 > obj.bar = 2; > delete obj.foo; > obj.baz = 3; > delete obj.bar; > delete obj.baz; > > > Is obj's structure now the same as when it was first created? > No. Deleting creates a new structure.
Comment on attachment 391427 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391427&action=review Nice work! I left some comments about 32-bits build failure. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4283 > + JSValueOperand value(this, node->child1()); Since we are fixing edges of `node->child1` during fixup, this will cause assertion failures. We have a some options to solve this: 1. Do not fixup to `CellUse` on 32-bits. 2. Speculate children here, but always fallback to slow path add a FIXME to add fast path to 32-bits later. 3. Implement fast path for 32-bits (The best option IMO). I understand that option 3 can not be viable given it is very hard to test things without real build. So we could go with both "1" or "2". Option 1 sounds more aligned with what we have in some cases of `DFGFixupPhase`, since speculation without fast path sounds wrong to me. If we decide to do so, do you mind open a bug and adding a FIXME to implement fast path for 32-bits? > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4292 > + callOperation(operationDeleteById, resultGPR, TrustedImmPtr::weakPointer(m_graph, m_graph.globalObjectFor(node->origin.semantic)), valueRegs, identifierUID(node->identifierNumber())); This operation doesn't exist anymore. I think it should call `operationDeleteByIdGeneric`. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4312 > + callOperation(operationDeleteByVal, resultGPR, TrustedImmPtr::weakPointer(m_graph, m_graph.globalObjectFor(node->origin.semantic)), baseRegs, keyRegs); This operation doesn't exist anymore. I think it should call `operationDeleteByValGeneric` instead.
Created attachment 391568 [details] Patch
Created attachment 391594 [details] Patch
Comment on attachment 391427 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391427&action=review >>> Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:193 >>> + fastPath, slowPath, fastPath.locationOf<JITStubRoutinePtrTag>(m_start)); >> >> I think you can just replace m_start with `m_slowPathJump.m_jump.label()`? That way you don't need m_start at all. > > I think you can just replace m_start with `m_slowPathJump.m_jump.label()`? That way you don't need m_start at all. I tried this, and it doesn't work. It causes an infinite loop. Any idea why?
Created attachment 391680 [details] Patch
Created attachment 391684 [details] Patch
Created attachment 391690 [details] Patch
Created attachment 391704 [details] Patch
Comment on attachment 391704 [details] Patch Clearing flags on attachment: 391704 Committed r257399: <https://trac.webkit.org/changeset/257399>
All reviewed patches have been landed. Closing bug.
<rdar://problem/59788843>