Bug 197910 – [JSC] UnlinkedMetadataTable's offset table should be small

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

[Yusuke Suzuki]

Each one takes 208 bytes for now.

[Yusuke Suzuki]

Created attachment 370177 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 370185 [details]
Patch

WIP

[Yusuke Suzuki]

16bit thing can cover almost all the thing. But 8bit version cannot cover things even if we leverage 4byte alignment. So, we should use 16bit offset table.

[Yusuke Suzuki]

Created attachment 370189 [details]
Patch

[Yusuke Suzuki]

Created attachment 370190 [details]
Patch

[Tadeu Zagallo]

This looks great! I wonder what percentage of the UnlinkedMetadataTables in gmail fit in the 16-bit offset table.

[Yusuke Suzuki]

(In reply to Tadeu Zagallo from comment #6)
> This looks great! I wonder what percentage of the UnlinkedMetadataTables in
> gmail fit in the 16-bit offset table.

I don't remember the exact number of that, but when I logged the 32bit UnlinkedMetadataTable in Gmail to see it is actually used and works, it is < 100, super limited. So, it is less than 0.4% of all the UnlinkedMetadataTable in Gmail :)

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #7)
> (In reply to Tadeu Zagallo from comment #6)
> > This looks great! I wonder what percentage of the UnlinkedMetadataTables in
> > gmail fit in the 16-bit offset table.
> 
> I don't remember the exact number of that, but when I logged the 32bit
> UnlinkedMetadataTable in Gmail to see it is actually used and works, it is <
> 100, super limited. So, it is less than 0.4% of all the
> UnlinkedMetadataTable in Gmail :)

On the other hand, 8bit version does not cover the existing MetadataTable well. When I inserted the log, it seems sooooo many MetadataTable in Gmail exceeds 1024 bytes (log does not stop...) => even if we use 4byte alignment information, this exceeds 8bit range. This makes me decide picking 16bit offset table here.

[Saam Barati]

Comment on attachment 370190 [details]
Patch

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

r=me

> Source/JavaScriptCore/bytecode/UnlinkedMetadataTable.cpp:61
> +            offset = roundUpToMultipleOf(metadataAlignment(static_cast<OpcodeID>(i)), offset);

maybe also ASSERT metadataAlignment <= s_maxMetadataAlignment?

> Source/JavaScriptCore/bytecode/UnlinkedMetadataTable.cpp:69
> +    if (m_is32Bit) {

Please make sure we have tests for this code in our regression test suite.

> Source/JavaScriptCore/bytecode/UnlinkedMetadataTable.h:106
> +    Offset16* offsetTable16() const { return bitwise_cast<Offset16*>(m_rawBuffer + sizeof(LinkingData)); }

nit: ASSERT(!m_is32bit)?

> Source/JavaScriptCore/bytecode/UnlinkedMetadataTable.h:107
> +    Offset32* offsetTable32() const { return bitwise_cast<Offset32*>(m_rawBuffer + sizeof(LinkingData) + s_offset16TableSize); }

nit: ASSERT(m_is32bit)?

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:185
> +const MetadataOffsetTable16 = 0
> +const MetadataOffsetTable32 = constexpr UnlinkedMetadataTable::s_offset16TableSize

I think these names should have "Offset" at the end of them.

[Yusuke Suzuki]

Comment on attachment 370190 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/bytecode/UnlinkedMetadataTable.cpp:61
>> +            offset = roundUpToMultipleOf(metadataAlignment(static_cast<OpcodeID>(i)), offset);
> 
> maybe also ASSERT metadataAlignment <= s_maxMetadataAlignment?

Sounds nice. Fixed.

>> Source/JavaScriptCore/bytecode/UnlinkedMetadataTable.cpp:69
>> +    if (m_is32Bit) {
> 
> Please make sure we have tests for this code in our regression test suite.

Thanks, added.

>> Source/JavaScriptCore/bytecode/UnlinkedMetadataTable.h:106
>> +    Offset16* offsetTable16() const { return bitwise_cast<Offset16*>(m_rawBuffer + sizeof(LinkingData)); }
> 
> nit: ASSERT(!m_is32bit)?

Nice, added.

>> Source/JavaScriptCore/bytecode/UnlinkedMetadataTable.h:107
>> +    Offset32* offsetTable32() const { return bitwise_cast<Offset32*>(m_rawBuffer + sizeof(LinkingData) + s_offset16TableSize); }
> 
> nit: ASSERT(m_is32bit)?

Ditto.

>> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:185
>> +const MetadataOffsetTable32 = constexpr UnlinkedMetadataTable::s_offset16TableSize
> 
> I think these names should have "Offset" at the end of them.

Added. Nice.

[Yusuke Suzuki]

Committed r245669: <https://trac.webkit.org/changeset/245669>

[Radar WebKit Bug Importer]

<rdar://problem/51051682>