[webkit-reviews] review granted: [Bug 197910] [JSC] UnlinkedMetadataTable's offset table should be small : [Attachment 370190] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 197910: [JSC] UnlinkedMetadataTable's offset table should be small
https://bugs.webkit.org/show_bug.cgi?id=197910

Attachment 370190: Patch

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




--- Comment #9 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 370190
  --> https://bugs.webkit.org/attachment.cgi?id=370190
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.