Bug 203276 – BytecodeIndex should be a proper C++ class

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

BytecodeIndex should be a proper C++ class

[Keith Miller]

Created attachment 381615 [details]
Patch

[Keith Miller]

Created attachment 381616 [details]
Patch

[Keith Miller]

Created attachment 381617 [details]
Patch

[Keith Miller]

Created attachment 381618 [details]
Patch

[Mark Lam]

Comment on attachment 381618 [details]
Patch

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

r=me with fixes.

> Source/JavaScriptCore/bytecode/BytecodeIndex.h:53
> +    explicit operator bool() const { return m_offset != std::numeric_limits<uint32_t>::max(); }

Can you use a "static constexpr InvalidOffset = std::numeric_limits<uint32_t>::max();" and use InvalidOffset here instead?  I think it will read better.

> Source/JavaScriptCore/bytecode/BytecodeIndex.h:66
> +    uint32_t m_offset { std::numeric_limits<uint32_t>::max() };

Ditto.  Use InvalidOffset here.

> Source/JavaScriptCore/bytecode/InstructionStream.h:80
> +        inline Offset offset() const { return m_index; }

Rename m_index to m_offset?  This cross-mixed naming is going to be a source of confusion.  You can do this in a follow up patch.

> Source/JavaScriptCore/bytecode/LazyOperandValueProfile.h:48
> -        : m_bytecodeOffset(0) // 0 = empty value
> +        : m_bytecodeIndex(0) // 0 = empty value
>          , m_operand(VirtualRegister()) // not a valid operand index in our current scheme
>      {
>      }
>      
>      LazyOperandValueProfileKey(WTF::HashTableDeletedValueType)
> -        : m_bytecodeOffset(1) // 1 = deleted value
> +        : m_bytecodeIndex(1) // 1 = deleted value

These feels like they deserve a comment as to why you can't use the default empty and deleted value instead of 0 and 1.  I presume the default values will cause problems?  The comment will prevent someone naive (like myself) coming along in the future and changing them thus.

> Source/JavaScriptCore/dfg/DFGOSREntrypointCreationPhase.cpp:55
> +        RELEASE_ASSERT(bytecodeIndex.offset());

Shouldn't this be RELEASE_ASSERT(bytecodeIndex)?

[Keith Miller]

Created attachment 381622 [details]
Patch for landing

[Keith Miller]

Comment on attachment 381618 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/BytecodeIndex.h:53
>> +    explicit operator bool() const { return m_offset != std::numeric_limits<uint32_t>::max(); }
> 
> Can you use a "static constexpr InvalidOffset = std::numeric_limits<uint32_t>::max();" and use InvalidOffset here instead?  I think it will read better.

Done.

>> Source/JavaScriptCore/bytecode/InstructionStream.h:80
>> +        inline Offset offset() const { return m_index; }
> 
> Rename m_index to m_offset?  This cross-mixed naming is going to be a source of confusion.  You can do this in a follow up patch.

Sure.

>> Source/JavaScriptCore/bytecode/LazyOperandValueProfile.h:48
>> +        : m_bytecodeIndex(1) // 1 = deleted value
> 
> These feels like they deserve a comment as to why you can't use the default empty and deleted value instead of 0 and 1.  I presume the default values will cause problems?  The comment will prevent someone naive (like myself) coming along in the future and changing them thus.

I think I just left it because that's how it was before. I don't think it will cause any problems. I can change it.

>> Source/JavaScriptCore/dfg/DFGOSREntrypointCreationPhase.cpp:55
>> +        RELEASE_ASSERT(bytecodeIndex.offset());
> 
> Shouldn't this be RELEASE_ASSERT(bytecodeIndex)?

No, the old line above now covers the != UINT_MAX case. This covers the bytecodeIndex.offset() == 0 case.

[Mark Lam]

Comment on attachment 381618 [details]
Patch

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

>>> Source/JavaScriptCore/dfg/DFGOSREntrypointCreationPhase.cpp:55
>>> +        RELEASE_ASSERT(bytecodeIndex.offset());
>> 
>> Shouldn't this be RELEASE_ASSERT(bytecodeIndex)?
> 
> No, the old line above now covers the != UINT_MAX case. This covers the bytecodeIndex.offset() == 0 case.

Oh, I see.

[Keith Miller]

Committed r251468: <https://trac.webkit.org/changeset/251468>

[Radar WebKit Bug Importer]

<rdar://problem/56523782>

[Yusuke Suzuki]

Committed r251471: <https://trac.webkit.org/changeset/251471>

[Keith Miller]

(In reply to Yusuke Suzuki from comment #11)
> Committed r251471: <https://trac.webkit.org/changeset/251471>

Ah, thanks. Unfortunately, the WinCairo EWS wasn't working.

[Yusuke Suzuki]

(In reply to Keith Miller from comment #12)
> (In reply to Yusuke Suzuki from comment #11)
> > Committed r251471: <https://trac.webkit.org/changeset/251471>
> 
> Ah, thanks. Unfortunately, the WinCairo EWS wasn't working.

And, maybe, we also need to update 32bit things (but EWS is not working, sad :().
32bit had different implementation of CallSiteIndex. We should remove this difference I think.

[Jonathan Bedard]

Just an FYI, this change broke a bunch of our Windows testers, Zan fixed it in <https://trac.webkit.org/changeset/251534>