Bug 210019 – [JSC] JSWrapperObject should use JSInternalFieldObjectImpl

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] JSWrapperObject should use JSInternalFieldObjectImpl

[Yusuke Suzuki]

Created attachment 395490 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 395490 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395490&action=review

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:-9866
> -    m_jit.storePtr(
> -        TrustedImmPtr(StringObject::info()),
> -        JITCompiler::Address(resultGPR, JSDestructibleObject::classInfoOffset()));

This was wrong :P

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:-1533
> -    template<typename ClassType>
> -    void emitAllocateDestructibleObject(GPRReg resultGPR, RegisteredStructure structure, 
> -        GPRReg scratchGPR1, GPRReg scratchGPR2, MacroAssembler::JumpList& slowPath)
> -    {
> -        m_jit.emitAllocateDestructibleObject<ClassType>(vm(), resultGPR, structure.get(), scratchGPR1, scratchGPR2, slowPath);
> -    }

We are no longer using this.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:-6404
> -        m_out.storePtr(m_out.constIntPtr(StringObject::info()), fastResultValue, m_heaps.JSDestructibleObject_classInfo);

Ditto :P

[Yusuke Suzuki]

Created attachment 395491 [details]
Patch

[Yusuke Suzuki]

Created attachment 395492 [details]
Patch

[Mark Lam]

Comment on attachment 395492 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395492&action=review

r=me with suggestion.

> Source/JavaScriptCore/runtime/JSWrapperObject.cpp:37
>  void JSWrapperObject::visitChildren(JSCell* cell, SlotVisitor& visitor)
>  {
> -    JSWrapperObject* thisObject = jsCast<JSWrapperObject*>(cell);
> +    auto* thisObject = jsCast<JSWrapperObject*>(cell);
>      ASSERT_GC_OBJECT_INHERITS(thisObject, info());
> -    JSObject::visitChildren(thisObject, visitor);
> -    visitor.append(thisObject->m_internalValue);
> +    Base::visitChildren(thisObject, visitor);
>  }

This no longer adds much value.  Can we just get rid of it and use the Base visitChildren()?  

The only thing done here that is not done by the Base is the ASSERT_GC_OBJECT_INHERITS.  How much value is there in keeping that?  If we want to keep this assert, can / should we make this function conditional on ASSERT_ENABLED?

[Saam Barati]

Comment on attachment 395492 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395492&action=review

> Source/JavaScriptCore/ChangeLog:16
> +        We remove this wrong store.

I feel like all of the c++ code emitting these stores should come along with a static_assert

[Yusuke Suzuki]

Comment on attachment 395492 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395492&action=review

>> Source/JavaScriptCore/runtime/JSWrapperObject.cpp:37
>>  }
> 
> This no longer adds much value.  Can we just get rid of it and use the Base visitChildren()?  
> 
> The only thing done here that is not done by the Base is the ASSERT_GC_OBJECT_INHERITS.  How much value is there in keeping that?  If we want to keep this assert, can / should we make this function conditional on ASSERT_ENABLED?

To avoid bothering about template-weak-symbol related linking problems (template weak symbols which is only referenced as a pointer), I think defining this is harmless and easy to make it manageable. For now, I'll just use this.

[Yusuke Suzuki]

Comment on attachment 395492 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395492&action=review

>> Source/JavaScriptCore/ChangeLog:16
>> +        We remove this wrong store.
> 
> I feel like all of the c++ code emitting these stores should come along with a static_assert

I think we should eventually put more and more static_assert in FTL / DFG side to ensure that FTL / DFG's class hierarchy assumption is kept.

[Yusuke Suzuki]

Committed r259645: <https://trac.webkit.org/changeset/259645>

[Radar WebKit Bug Importer]

<rdar://problem/61399515>