Bug 207087

Summary: [JSC] Introduce UnlinkedCodeBlockGenerator and reduce sizeof(UnlinkedCodeBlock)
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, dbates, ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch tzagallo: review+

Description Yusuke Suzuki 2020-02-01 00:19:09 PST 
UnlinkedCodeBlock has many Vectors while they are already frozen.
We should introduce UnlinkedCodeBlockWriter, and use Vector in it.
And when creating UnlinkedCodeBlock, we should use RefCountedArray for Vectors.
Comment 1 Yusuke Suzuki 2020-02-04 01:08:20 PST 
Created attachment 389635 [details]
Patch
Comment 2 Yusuke Suzuki 2020-02-04 01:11:27 PST 
I think this can offer sub-1% memory reduction in Gmail.
Comment 3 Yusuke Suzuki 2020-02-04 01:40:18 PST 
Ah,dead-lock! Fixing
Comment 4 Yusuke Suzuki 2020-02-04 01:48:48 PST 
Created attachment 389639 [details]
Patch
Comment 5 Yusuke Suzuki 2020-02-04 01:50:55 PST 
Created attachment 389640 [details]
Patch
Comment 6 Yusuke Suzuki 2020-02-04 02:29:13 PST 
Created attachment 389645 [details]
Patch
Comment 7 Tadeu Zagallo 2020-02-04 10:43:05 PST 
Comment on attachment 389645 [details]
Patch

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

Nice!

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlockGenerator.cpp:150
> +        if (!m_codeBlock->m_rareData) {

Why would the code block have rare data at this point?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:291
> +    m_codeBlock->finalize(m_writer.finalize());

Not necessarily on this patch, but ideally I think UnlinkedCodeBlockGenerator should own the UnlinkedCodeBlock and return it from finalize.

> Source/WTF/wtf/RefCountedArray.h:201
> +    T& front() { return (*this)[0]; }
> +    const T& front() const { return (*this)[0]; }

I believe this called `first` in Vector, should we just call the same here?
Comment 8 Yusuke Suzuki 2020-02-04 11:03:35 PST 
Comment on attachment 389645 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/bytecode/UnlinkedCodeBlockGenerator.cpp:150
>> +        if (!m_codeBlock->m_rareData) {
> 
> Why would the code block have rare data at this point?

If `NeedsClassFieldInitializer::Yes` is specified, we create a rareData in UnlinkedCodeBlock's constructor.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:291
>> +    m_codeBlock->finalize(m_writer.finalize());
> 
> Not necessarily on this patch, but ideally I think UnlinkedCodeBlockGenerator should own the UnlinkedCodeBlock and return it from finalize.

Right! Filed a issue here. https://bugs.webkit.org/show_bug.cgi?id=207212

>> Source/WTF/wtf/RefCountedArray.h:201
>> +    const T& front() const { return (*this)[0]; }
> 
> I believe this called `first` in Vector, should we just call the same here?

Nice, fixed.
Comment 9 Yusuke Suzuki 2020-02-04 11:05:25 PST 
Committed r255687: <https://trac.webkit.org/changeset/255687>
Comment 10 Radar WebKit Bug Importer 2020-02-04 11:06:18 PST 
<rdar://problem/59155888>