Bug 207522 – Inline Cache delete by id/val

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Justin Michaud]

Make inline caching work for deletes in the baseline jit.

[Justin Michaud]

Created attachment 390324 [details]
Patch

[Justin Michaud]

Created attachment 390410 [details]
Patch

[Justin Michaud]

Created attachment 390473 [details]
Patch

[Justin Michaud]

Created attachment 390477 [details]
Patch

[Saam Barati]

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.

[Justin Michaud]

Created attachment 390597 [details]
Patch

[Justin Michaud]

Checking for test failures

[Justin Michaud]

Created attachment 390659 [details]
Patch

[Justin Michaud]

Created attachment 390670 [details]
Patch

[Justin Michaud]

(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;
}

[Justin Michaud]

Created attachment 390690 [details]
Patch

[Justin Michaud]

Created attachment 390837 [details]
Patch

[Justin Michaud]

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

[Justin Michaud]

Created attachment 390841 [details]
Patch

[Justin Michaud]

Created attachment 390842 [details]
Patch

[Justin Michaud]

Created attachment 390934 [details]
Patch

[Justin Michaud]

Created attachment 390968 [details]
Patch

[Justin Michaud]

Created attachment 391075 [details]
Patch

[Justin Michaud]

Created attachment 391080 [details]
Patch

[Keith Miller]

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.

[Justin Michaud]

Created attachment 391349 [details]
Patch

[Justin Michaud]

Created attachment 391350 [details]
Patch

[Justin Michaud]

Created attachment 391360 [details]
Patch

[Saam Barati]

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?

[Guillaume Emont]

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.

[Justin Michaud]

Created attachment 391427 [details]
Patch

[Keith Miller]

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?

[Keith Miller]

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.

[Keith Miller]

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.

[Justin Michaud]

(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.

[Caio Lima]

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.

[Justin Michaud]

Created attachment 391568 [details]
Patch

[Justin Michaud]

Created attachment 391594 [details]
Patch

[Justin Michaud]

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?

[Justin Michaud]

Created attachment 391680 [details]
Patch

[Justin Michaud]

Created attachment 391684 [details]
Patch

[Justin Michaud]

Created attachment 391690 [details]
Patch

[Justin Michaud]

Created attachment 391704 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 391704 [details]
Patch

Clearing flags on attachment: 391704

Committed r257399: <https://trac.webkit.org/changeset/257399>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59788843>