Bug 190933 – Consider removing double load for accessing the MetadataTable from LLInt

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Tadeu Zagallo]

https://bugs.webkit.org/show_bug.cgi?id=187373 introduces a MetadataTable to store mutable data for bytecodes. Right now, fetching the table requires two loads (codeBlock->m_metadataTable->m_buffer).

[Yusuke Suzuki]

Created attachment 354080 [details]
Patch

[Keith Miller]

Comment on attachment 354080 [details]
Patch

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

r=me with some nits.

> Source/JavaScriptCore/bytecode/UnlinkedMetadataTableInlines.h:112
> +    void* buffer;

Nit: Can't this just be a char*? Then you wouldn't need the reinterpret_casts below.

> Source/JavaScriptCore/bytecode/UnlinkedMetadataTableInlines.h:121
> +    memset(reinterpret_cast<uint8_t*>(buffer) + sizeof(LinkingData) + s_offsetTableSize, 0, totalSize - s_offsetTableSize);
> +    return adoptRef(*new (reinterpret_cast<uint8_t*>(buffer) + sizeof(LinkingData)) MetadataTable(*this));

It seems like we are going to fill a lot of the buffer twice between here and the MetadataTable constructor? Should we file a bug to move the zeroing into the constructor?

[Yusuke Suzuki]

Comment on attachment 354080 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/UnlinkedMetadataTableInlines.h:112
>> +    void* buffer;
> 
> Nit: Can't this just be a char*? Then you wouldn't need the reinterpret_casts below.

Sounds nice! Fixed.

>> Source/JavaScriptCore/bytecode/UnlinkedMetadataTableInlines.h:121
>> +    return adoptRef(*new (reinterpret_cast<uint8_t*>(buffer) + sizeof(LinkingData)) MetadataTable(*this));
> 
> It seems like we are going to fill a lot of the buffer twice between here and the MetadataTable constructor? Should we file a bug to move the zeroing into the constructor?

MetadataTable's constructor does nothing except for filling LinkingData :)

[Yusuke Suzuki]

Committed r237987: <https://trac.webkit.org/changeset/237987>

[Radar WebKit Bug Importer]

<rdar://problem/45910122>