Bug 210019

Summary: [JSC] JSWrapperObject should use JSInternalFieldObjectImpl
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, sbarati, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch mark.lam: review+

Description Yusuke Suzuki 2020-04-05 00:39:56 PDT
[JSC] JSWrapperObject should use JSInternalFieldObjectImpl
Comment 1 Yusuke Suzuki 2020-04-05 00:42:59 PDT
Created attachment 395490 [details]
Patch
Comment 2 Yusuke Suzuki 2020-04-05 00:44:13 PDT
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
Comment 3 Yusuke Suzuki 2020-04-05 00:47:37 PDT
Created attachment 395491 [details]
Patch
Comment 4 Yusuke Suzuki 2020-04-05 00:55:34 PDT
Created attachment 395492 [details]
Patch
Comment 5 Mark Lam 2020-04-05 22:42:21 PDT
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?
Comment 6 Saam Barati 2020-04-06 15:07:05 PDT
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
Comment 7 Yusuke Suzuki 2020-04-07 10:30:59 PDT
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.
Comment 8 Yusuke Suzuki 2020-04-07 10:31:59 PDT
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.
Comment 9 Yusuke Suzuki 2020-04-07 10:39:34 PDT
Committed r259645: <https://trac.webkit.org/changeset/259645>
Comment 10 Radar WebKit Bug Importer 2020-04-07 10:40:20 PDT
<rdar://problem/61399515>