Bug 233723 – Remove StructureIDBlob

• Status: REOPENED
• Component: New Bugs

[Keith Miller]

Remove StructureIDBlob

[Keith Miller]

Created attachment 445608 [details]
Patch

[Keith Miller]

Created attachment 445609 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 445609 [details]
Patch

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

r=me

> Source/JavaScriptCore/runtime/Structure.h:861
> +    // part of the object. And need to match the order of the equivalent properties in
> +    // JSCell.
> +    IndexingType m_indexingModeIncludingHistory;
> +    JSType m_type;
> +    TypeInfo::InlineTypeFlags m_inlineTypeFlags;
> +    CellState m_defaultCellState { CellState::DefinitelyWhite };

I think we should rename them to explicitly state that they are information of JSCell allocated with this Structure.
Since Structure itself is a JSCell, it has Structure::m_type etc. in JSCell parent class. So using the same name here is confusing.
e.g. m_instanceType, m_instanceIndexingModeIncludingHistory etc.

[Mark Lam]

Comment on attachment 445609 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:11
> +        can be a single load platforms that allow (and don't penalize) misaligned loads.

/load platforms/load on platforms/

>> Source/JavaScriptCore/runtime/Structure.h:861
>> +    IndexingType m_indexingModeIncludingHistory;
>> +    JSType m_type;
>> +    TypeInfo::InlineTypeFlags m_inlineTypeFlags;
>> +    CellState m_defaultCellState { CellState::DefinitelyWhite };
> 
> I think we should rename them to explicitly state that they are information of JSCell allocated with this Structure.
> Since Structure itself is a JSCell, it has Structure::m_type etc. in JSCell parent class. So using the same name here is confusing.
> e.g. m_instanceType, m_instanceIndexingModeIncludingHistory etc.

I suggest adding static_asserts to guarantee that the order of these fields match those in JSCell to ensure that anyone messing this up will not go unnoticed.  Alternatively, define a JSCellHeader macro / struct and use that both here and in JSCell to ensure that we never mess up.  That would be a much stronger guarantee than the comment above.  I think the static_asserts would probably be least intrusive for now.

[Mark Lam]

Comment on attachment 445609 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/Structure.h:861
>>> +    CellState m_defaultCellState { CellState::DefinitelyWhite };
>> 
>> I think we should rename them to explicitly state that they are information of JSCell allocated with this Structure.
>> Since Structure itself is a JSCell, it has Structure::m_type etc. in JSCell parent class. So using the same name here is confusing.
>> e.g. m_instanceType, m_instanceIndexingModeIncludingHistory etc.
> 
> I suggest adding static_asserts to guarantee that the order of these fields match those in JSCell to ensure that anyone messing this up will not go unnoticed.  Alternatively, define a JSCellHeader macro / struct and use that both here and in JSCell to ensure that we never mess up.  That would be a much stronger guarantee than the comment above.  I think the static_asserts would probably be least intrusive for now.

In response to Yusuke's comment, one alternative id to to put these in an embedded struct here e.g.

    struct {
        IndexingType m_indexingModeIncludingHistory;
        JSType m_type;
        TypeInfo::InlineTypeFlags m_inlineTypeFlags;
        CellState m_defaultCellState { CellState::DefinitelyWhite };
    } m_cellHeader;

[Saam Barati]

Comment on attachment 445609 [details]
Patch

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

Nice

> Source/JavaScriptCore/runtime/Structure.h:168
> +    int32_t objectInitializationBlob() const { return *reinterpret_cast_ptr<const uint32_t*>(&m_indexingModeIncludingHistory); }

nit: bitwise_cast

[Keith Miller]

Comment on attachment 445609 [details]
Patch

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

>> Source/JavaScriptCore/runtime/Structure.h:168
>> +    int32_t objectInitializationBlob() const { return *reinterpret_cast_ptr<const uint32_t*>(&m_indexingModeIncludingHistory); }
> 
> nit: bitwise_cast

Why bitwise_cast? It's casting a pointer to a different pointer, I usually only use bitwise_cast for integral->integral.

[Keith Miller]

Created attachment 445867 [details]
Patch for landing

[EWS]

Committed r286502 (244841@main): <https://commits.webkit.org/244841@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 445867 [details].

[Radar WebKit Bug Importer]

<rdar://problem/86024479>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 233930