Bug 190933 - Consider removing double load for accessing the MetadataTable from LLInt
Summary: Consider removing double load for accessing the MetadataTable from LLInt
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-10-25 16:41 PDT by Tadeu Zagallo
Modified: 2018-11-08 07:43 PST (History)
7 users (show)

See Also:


Attachments
Patch (19.63 KB, patch)
2018-11-07 04:09 PST, Yusuke Suzuki
keith_miller: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tadeu Zagallo 2018-10-25 16:41:03 PDT
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).
Comment 1 Yusuke Suzuki 2018-11-07 04:09:33 PST
Created attachment 354080 [details]
Patch
Comment 2 Keith Miller 2018-11-07 05:25:48 PST
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?
Comment 3 Yusuke Suzuki 2018-11-08 07:37:59 PST
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 :)
Comment 4 Yusuke Suzuki 2018-11-08 07:42:17 PST
Committed r237987: <https://trac.webkit.org/changeset/237987>
Comment 5 Radar WebKit Bug Importer 2018-11-08 07:43:24 PST
<rdar://problem/45910122>