Bug 197910

Created attachment 370177 [details] Patch WIP

Created attachment 370185 [details] Patch WIP

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.

Created attachment 370189 [details] Patch

Created attachment 370190 [details] Patch

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

(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 :)

(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.

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.

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.

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

<rdar://problem/51051682>