Changeset 251088 in webkit


Ignore:
Timestamp:
Oct 14, 2019 12:54:28 PM (4 years ago)
Author:
ysuzuki@apple.com
Message:

[JSC] GetterSetter should be JSCell, not JSObject
https://bugs.webkit.org/show_bug.cgi?id=202656

Reviewed by Tadeu Zagallo and Saam Barati.

JSTests:

  • stress/getter-setter-should-be-cell.js: Added.

(foo.with.):
(foo.with.get for):
(foo.with.bar):
(foo):

Source/JavaScriptCore:

Essentially, GetterSetter is not a JSObject. It is like a JSCell. But we made GetterSetter JSObject
to leverage existing strict-eq implementations for JSObject: pointer-comparison. But given the following
conditions,

  1. GetterSetter strict-eq comparison only happens in builtin code when using @tryGetById.
  2. RHS of that comparison is always folded into constant in DFG.
  3. We already use pointer-comparison for cells that are neither JSString nor JSBigInt.
  4. DFG strength reduction already has a rule which makes CompareStrictEq(Cell-not-JSString/JSBigInt, Constant) ComparePtrEq.

So we already support non-JSString/JSBigInt cell comparison in JSC JS code. We should use it instead of making GetterSetter JSObject.
This patch makes GetterSetter JSCell, and makes getterSetterStructure per-VM structure.

The attached test reported AI validation failure. AI assumed that GetterSetter's realm should be the same to the base object. But
this is incorrect in our runtime code: we are creating GetterSetter with lexical realm (JSGlobalObject). But the fundamental problem
is that GetterSetter is JSObject and tied to JSGlobalObject while it is not necessary.

  • dfg/DFGAbstractInterpreterInlines.h:

(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):

  • dfg/DFGFixupPhase.cpp:
  • runtime/GetterSetter.cpp:
  • runtime/GetterSetter.h:
  • runtime/JSGlobalObject.cpp:

(JSC::getGetterById):
(JSC::JSGlobalObject::init):
(JSC::JSGlobalObject::visitChildren):

  • runtime/JSGlobalObject.h:

(JSC::JSGlobalObject::regExpProtoGlobalGetter const):
(JSC::JSGlobalObject::regExpProtoUnicodeGetter const):
(JSC::JSGlobalObject::customGetterSetterFunctionStructure const):
(JSC::JSGlobalObject::getterSetterStructure const): Deleted.

  • runtime/JSType.h:
  • runtime/VM.cpp:

(JSC::VM::VM):

  • runtime/VM.h:
Location:
trunk
Files:
1 added
11 edited

Legend:

Unmodified
Added
Removed
  • trunk/JSTests/ChangeLog

    r251085 r251088  
     12019-10-14  Yusuke Suzuki  <ysuzuki@apple.com>
     2
     3        [JSC] GetterSetter should be JSCell, not JSObject
     4        https://bugs.webkit.org/show_bug.cgi?id=202656
     5
     6        Reviewed by Tadeu Zagallo and Saam Barati.
     7
     8        * stress/getter-setter-should-be-cell.js: Added.
     9        (foo.with.):
     10        (foo.with.get for):
     11        (foo.with.bar):
     12        (foo):
     13
    1142019-10-14  Saam Barati  <sbarati@apple.com>
    215
  • trunk/Source/JavaScriptCore/ChangeLog

    r251085 r251088  
     12019-10-14  Yusuke Suzuki  <ysuzuki@apple.com>
     2
     3        [JSC] GetterSetter should be JSCell, not JSObject
     4        https://bugs.webkit.org/show_bug.cgi?id=202656
     5
     6        Reviewed by Tadeu Zagallo and Saam Barati.
     7
     8        Essentially, GetterSetter is not a JSObject. It is like a JSCell. But we made GetterSetter JSObject
     9        to leverage existing strict-eq implementations for JSObject: pointer-comparison. But given the following
     10        conditions,
     11
     12        1. GetterSetter strict-eq comparison only happens in builtin code when using @tryGetById.
     13        2. RHS of that comparison is always folded into constant in DFG.
     14        3. We already use pointer-comparison for cells that are neither JSString nor JSBigInt.
     15        4. DFG strength reduction already has a rule which makes `CompareStrictEq(Cell-not-JSString/JSBigInt, Constant)` `ComparePtrEq`.
     16
     17        So we already support non-JSString/JSBigInt cell comparison in JSC JS code. We should use it instead of making GetterSetter JSObject.
     18        This patch makes GetterSetter JSCell, and makes getterSetterStructure per-VM structure.
     19
     20        The attached test reported AI validation failure. AI assumed that GetterSetter's realm should be the same to the base object. But
     21        this is incorrect in our runtime code: we are creating GetterSetter with lexical realm (JSGlobalObject). But the fundamental problem
     22        is that GetterSetter is JSObject and tied to JSGlobalObject while it is not necessary.
     23
     24        * dfg/DFGAbstractInterpreterInlines.h:
     25        (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
     26        * dfg/DFGFixupPhase.cpp:
     27        * runtime/GetterSetter.cpp:
     28        * runtime/GetterSetter.h:
     29        * runtime/JSGlobalObject.cpp:
     30        (JSC::getGetterById):
     31        (JSC::JSGlobalObject::init):
     32        (JSC::JSGlobalObject::visitChildren):
     33        * runtime/JSGlobalObject.h:
     34        (JSC::JSGlobalObject::regExpProtoGlobalGetter const):
     35        (JSC::JSGlobalObject::regExpProtoUnicodeGetter const):
     36        (JSC::JSGlobalObject::customGetterSetterFunctionStructure const):
     37        (JSC::JSGlobalObject::getterSetterStructure const): Deleted.
     38        * runtime/JSType.h:
     39        * runtime/VM.cpp:
     40        (JSC::VM::VM):
     41        * runtime/VM.h:
     42
    1432019-10-14  Saam Barati  <sbarati@apple.com>
    244
  • trunk/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h

    r250932 r251088  
    34673467        }
    34683468       
    3469         if (base.value() && base.value().isObject()) {
    3470             setForNode(node, asObject(base.value())->globalObject()->getterSetterStructure());
    3471             break;
    3472         }
    3473 
    3474         setTypeForNode(node, SpecObjectOther);
     3469        setForNode(node, m_vm.getterSetterStructure.get());
    34753470        break;
    34763471    }
  • trunk/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp

    r250932 r251088  
    3535#include "DFGPredictionPropagationPhase.h"
    3636#include "DFGVariableAccessDataDump.h"
     37#include "GetterSetter.h"
    3738#include "JSCInlines.h"
    3839#include "TypeLocation.h"
  • trunk/Source/JavaScriptCore/runtime/GetterSetter.cpp

    r250932 r251088  
    3434STATIC_ASSERT_IS_TRIVIALLY_DESTRUCTIBLE(GetterSetter);
    3535
    36 const ClassInfo GetterSetter::s_info = { "GetterSetter", &Base::s_info, nullptr, nullptr, CREATE_METHOD_TABLE(GetterSetter) };
     36const ClassInfo GetterSetter::s_info = { "GetterSetter", nullptr, nullptr, nullptr, CREATE_METHOD_TABLE(GetterSetter) };
    3737
    3838void GetterSetter::visitChildren(JSCell* cell, SlotVisitor& visitor)
  • trunk/Source/JavaScriptCore/runtime/GetterSetter.h

    r250932 r251088  
    4141// that constant is observed to have a non-null setter (or getter) then we can
    4242// constant fold that setter (or getter).
    43 class GetterSetter final : public JSNonFinalObject {
     43class GetterSetter final : public JSCell {
    4444    friend class JIT;
    45     typedef JSNonFinalObject Base;
     45    using Base = JSCell;
    4646private:
    4747    GetterSetter(VM& vm, JSGlobalObject* globalObject, JSObject* getter, JSObject* setter)
    48         : Base(vm, globalObject->getterSetterStructure())
     48        : Base(vm, vm.getterSetterStructure.get())
    4949    {
    5050        WTF::storeStoreFence();
  • trunk/Source/JavaScriptCore/runtime/JSGlobalObject.cpp

    r250932 r251088  
    451451}
    452452
    453 static JSObject* getGetterById(ExecState* exec, JSObject* base, const Identifier& ident)
     453static GetterSetter* getGetterById(ExecState* exec, JSObject* base, const Identifier& ident)
    454454{
    455455    JSValue baseValue = JSValue(base);
    456456    PropertySlot slot(baseValue, PropertySlot::InternalMethodType::VMInquiry);
    457457    baseValue.getPropertySlot(exec, ident, slot);
    458     return slot.getPureResult().toObject(exec);
     458    return jsCast<GetterSetter*>(slot.getPureResult());
    459459}
    460460
     
    515515            init.set(JSBoundFunction::createStructure(init.vm, init.owner, init.owner->m_functionPrototype.get()));
    516516        });
    517     m_getterSetterStructure.set(vm, this, GetterSetter::createStructure(vm, this, jsNull()));
    518517    m_nativeStdFunctionStructure.initLater(
    519518        [] (const Initializer<Structure>& init) {
     
    940939    JSFunction* privateFuncSetBucketKey = JSFunction::create(vm, this, 0, String(), setPrivateFuncSetBucketKey, JSSetBucketKeyIntrinsic);
    941940
    942     JSObject* regExpProtoFlagsGetterObject = getGetterById(exec, m_regExpPrototype.get(), vm.propertyNames->flags);
     941    GetterSetter* regExpProtoFlagsGetter = getGetterById(exec, m_regExpPrototype.get(), vm.propertyNames->flags);
    943942    catchScope.assertNoException();
    944     JSObject* regExpProtoGlobalGetterObject = getGetterById(exec, m_regExpPrototype.get(), vm.propertyNames->global);
     943    GetterSetter* regExpProtoGlobalGetter = getGetterById(exec, m_regExpPrototype.get(), vm.propertyNames->global);
    945944    catchScope.assertNoException();
    946     m_regExpProtoGlobalGetter.set(vm, this, regExpProtoGlobalGetterObject);
    947     JSObject* regExpProtoIgnoreCaseGetterObject = getGetterById(exec, m_regExpPrototype.get(), vm.propertyNames->ignoreCase);
     945    m_regExpProtoGlobalGetter.set(vm, this, regExpProtoGlobalGetter);
     946    GetterSetter* regExpProtoIgnoreCaseGetter = getGetterById(exec, m_regExpPrototype.get(), vm.propertyNames->ignoreCase);
    948947    catchScope.assertNoException();
    949     JSObject* regExpProtoMultilineGetterObject = getGetterById(exec, m_regExpPrototype.get(), vm.propertyNames->multiline);
     948    GetterSetter* regExpProtoMultilineGetter = getGetterById(exec, m_regExpPrototype.get(), vm.propertyNames->multiline);
    950949    catchScope.assertNoException();
    951     JSObject* regExpProtoSourceGetterObject = getGetterById(exec, m_regExpPrototype.get(), vm.propertyNames->source);
     950    GetterSetter* regExpProtoSourceGetter = getGetterById(exec, m_regExpPrototype.get(), vm.propertyNames->source);
    952951    catchScope.assertNoException();
    953     JSObject* regExpProtoStickyGetterObject = getGetterById(exec, m_regExpPrototype.get(), vm.propertyNames->sticky);
     952    GetterSetter* regExpProtoStickyGetter = getGetterById(exec, m_regExpPrototype.get(), vm.propertyNames->sticky);
    954953    catchScope.assertNoException();
    955     JSObject* regExpProtoUnicodeGetterObject = getGetterById(exec, m_regExpPrototype.get(), vm.propertyNames->unicode);
     954    GetterSetter* regExpProtoUnicodeGetter = getGetterById(exec, m_regExpPrototype.get(), vm.propertyNames->unicode);
    956955    catchScope.assertNoException();
    957     m_regExpProtoUnicodeGetter.set(vm, this, regExpProtoUnicodeGetterObject);
     956    m_regExpProtoUnicodeGetter.set(vm, this, regExpProtoUnicodeGetter);
    958957    JSObject* builtinRegExpExec = asObject(m_regExpPrototype->getDirect(vm, vm.propertyNames->exec).asCell());
    959958    m_regExpProtoExec.set(vm, this, builtinRegExpExec);
     
    10251024        GlobalPropertyInfo(vm.propertyNames->builtinNames().isConstructorPrivateName(), JSFunction::create(vm, this, 1, String(), esSpecIsConstructor, NoIntrinsic), PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
    10261025
    1027         GlobalPropertyInfo(vm.propertyNames->builtinNames().regExpProtoFlagsGetterPrivateName(), regExpProtoFlagsGetterObject, PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
    1028         GlobalPropertyInfo(vm.propertyNames->builtinNames().regExpProtoGlobalGetterPrivateName(), regExpProtoGlobalGetterObject, PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
    1029         GlobalPropertyInfo(vm.propertyNames->builtinNames().regExpProtoIgnoreCaseGetterPrivateName(), regExpProtoIgnoreCaseGetterObject, PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
    1030         GlobalPropertyInfo(vm.propertyNames->builtinNames().regExpProtoMultilineGetterPrivateName(), regExpProtoMultilineGetterObject, PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
    1031         GlobalPropertyInfo(vm.propertyNames->builtinNames().regExpProtoSourceGetterPrivateName(), regExpProtoSourceGetterObject, PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
    1032         GlobalPropertyInfo(vm.propertyNames->builtinNames().regExpProtoStickyGetterPrivateName(), regExpProtoStickyGetterObject, PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
    1033         GlobalPropertyInfo(vm.propertyNames->builtinNames().regExpProtoUnicodeGetterPrivateName(), regExpProtoUnicodeGetterObject, PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
     1026        GlobalPropertyInfo(vm.propertyNames->builtinNames().regExpProtoFlagsGetterPrivateName(), regExpProtoFlagsGetter, PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
     1027        GlobalPropertyInfo(vm.propertyNames->builtinNames().regExpProtoGlobalGetterPrivateName(), regExpProtoGlobalGetter, PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
     1028        GlobalPropertyInfo(vm.propertyNames->builtinNames().regExpProtoIgnoreCaseGetterPrivateName(), regExpProtoIgnoreCaseGetter, PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
     1029        GlobalPropertyInfo(vm.propertyNames->builtinNames().regExpProtoMultilineGetterPrivateName(), regExpProtoMultilineGetter, PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
     1030        GlobalPropertyInfo(vm.propertyNames->builtinNames().regExpProtoSourceGetterPrivateName(), regExpProtoSourceGetter, PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
     1031        GlobalPropertyInfo(vm.propertyNames->builtinNames().regExpProtoStickyGetterPrivateName(), regExpProtoStickyGetter, PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
     1032        GlobalPropertyInfo(vm.propertyNames->builtinNames().regExpProtoUnicodeGetterPrivateName(), regExpProtoUnicodeGetter, PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
    10341033
    10351034        // RegExp.prototype helpers.
     
    17611760    thisObject->m_customGetterSetterFunctionStructure.visit(visitor);
    17621761    thisObject->m_boundFunctionStructure.visit(visitor);
    1763     visitor.append(thisObject->m_getterSetterStructure);
    17641762    thisObject->m_nativeStdFunctionStructure.visit(visitor);
    17651763    visitor.append(thisObject->m_regExpStructure);
  • trunk/Source/JavaScriptCore/runtime/JSGlobalObject.h

    r250932 r251088  
    307307    WriteBarrier<JSObject> m_regExpProtoExec;
    308308    WriteBarrier<JSObject> m_regExpProtoSymbolReplace;
    309     WriteBarrier<JSObject> m_regExpProtoGlobalGetter;
    310     WriteBarrier<JSObject> m_regExpProtoUnicodeGetter;
     309    WriteBarrier<GetterSetter> m_regExpProtoGlobalGetter;
     310    WriteBarrier<GetterSetter> m_regExpProtoUnicodeGetter;
    311311    WriteBarrier<GetterSetter> m_throwTypeErrorArgumentsCalleeAndCallerGetterSetter;
    312312
     
    366366    LazyProperty<JSGlobalObject, Structure> m_boundFunctionStructure;
    367367    LazyProperty<JSGlobalObject, Structure> m_customGetterSetterFunctionStructure;
    368     WriteBarrier<Structure> m_getterSetterStructure;
    369368    LazyProperty<JSGlobalObject, Structure> m_nativeStdFunctionStructure;
    370369    PropertyOffset m_functionNameOffset;
     
    623622    JSObject* regExpProtoExecFunction() const { return m_regExpProtoExec.get(); }
    624623    JSObject* regExpProtoSymbolReplaceFunction() const { return m_regExpProtoSymbolReplace.get(); }
    625     JSObject* regExpProtoGlobalGetter() const { return m_regExpProtoGlobalGetter.get(); }
    626     JSObject* regExpProtoUnicodeGetter() const { return m_regExpProtoUnicodeGetter.get(); }
     624    GetterSetter* regExpProtoGlobalGetter() const { return m_regExpProtoGlobalGetter.get(); }
     625    GetterSetter* regExpProtoUnicodeGetter() const { return m_regExpProtoUnicodeGetter.get(); }
    627626    GetterSetter* throwTypeErrorArgumentsCalleeAndCallerGetterSetter()
    628627    {
     
    749748    Structure* boundFunctionStructure() const { return m_boundFunctionStructure.get(this); }
    750749    Structure* customGetterSetterFunctionStructure() const { return m_customGetterSetterFunctionStructure.get(this); }
    751     Structure* getterSetterStructure() const { return m_getterSetterStructure.get(); }
    752750    Structure* nativeStdFunctionStructure() const { return m_nativeStdFunctionStructure.get(this); }
    753751    PropertyOffset functionNameOffset() const { return m_functionNameOffset; }
  • trunk/Source/JavaScriptCore/runtime/JSType.h

    r250932 r251088  
    3030    BigIntType,
    3131
     32    GetterSetterType,
    3233    CustomGetterSetterType,
    3334    APIValueWrapperType,
     
    8889    DataViewType,
    8990    // End JSArrayBufferView types.
    90 
    91     GetterSetterType,
    9291
    9392    // JSScope <- JSWithScope
  • trunk/Source/JavaScriptCore/runtime/VM.cpp

    r250932 r251088  
    343343    terminatedExecutionErrorStructure.set(*this, TerminatedExecutionError::createStructure(*this, 0, jsNull()));
    344344    propertyNameEnumeratorStructure.set(*this, JSPropertyNameEnumerator::createStructure(*this, 0, jsNull()));
     345    getterSetterStructure.set(*this, GetterSetter::createStructure(*this, 0, jsNull()));
    345346    customGetterSetterStructure.set(*this, CustomGetterSetter::createStructure(*this, 0, jsNull()));
    346347    domAttributeGetterSetterStructure.set(*this, DOMAttributeGetterSetter::createStructure(*this, 0, jsNull()));
  • trunk/Source/JavaScriptCore/runtime/VM.h

    r250932 r251088  
    505505    Strong<Structure> stringStructure;
    506506    Strong<Structure> propertyNameEnumeratorStructure;
     507    Strong<Structure> getterSetterStructure;
    507508    Strong<Structure> customGetterSetterStructure;
    508509    Strong<Structure> domAttributeGetterSetterStructure;
Note: See TracChangeset for help on using the changeset viewer.