Bug 206557 – Fix small memory regression caused by r206365

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

[Justin Michaud]

Put m_giveUpOnObjectToStringValueCache into m_objectToStringValue and use 16 bit fields for m_maxOffset and m_transitionOffset, added to their non-rare counterparts.

[Justin Michaud]

Put StructureRareData::m_giveUpOnObjectToStringValueCache into m_objectToStringValue to prevent increasing StructureRareData's size.

[Justin Michaud]

Created attachment 388370 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 388370 [details]
Patch

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

I think this direction is correct. But I found several bugs.

> Source/JavaScriptCore/runtime/StructureRareData.cpp:74
> +    visitor.appendUnbarriered(thisObject->objectToStringValue());

`appendUnbarriered` assumes that whether the cell pointer is valid or nullptr.
Let's do the similar thing done for `m_cachedOwnKeys` below.

> Source/JavaScriptCore/runtime/StructureRareData.cpp:97
> +    if (objectToStringValue() == giveUpOnObjectToStringValueCacheValue())

This never happens since `objectToStringValue()` returns nullptr if the stored value is `giveUpOnObjectToStringValueCacheValue`.
Let's directly read m_objectToStringValue here, and check it carefully.

> Source/JavaScriptCore/runtime/StructureRareData.cpp:159
> +    if (objectToStringValue() != giveUpOnObjectToStringValueCacheValue())

Ditto.

[Justin Michaud]

Created attachment 388384 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 388384 [details]
Patch

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

r=me with nits.

> Source/JavaScriptCore/runtime/StructureRareData.h:71
> +    bool doGiveUpOnObjectToStringValueCache() { return m_objectToStringValue.unvalidatedGet() == giveUpOnObjectToStringValueCacheValue(); }

`doGiveUpOnObjectToStringValueCache` sounds like we give up caching by this function. Can you rename it to `canCacheObjectToStringValue()`?

> Source/JavaScriptCore/runtime/StructureRareData.h:72
> +    static JSString* giveUpOnObjectToStringValueCacheValue() { return bitwise_cast<JSString*>(static_cast<uintptr_t>(1)); }

Let's rename it something like "objectToStringCacheGiveUpMarker()", otherwise, this function name sounds like we are giving up caching by this function.

[Justin Michaud]

Created attachment 389174 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 389174 [details]
Patch

Clearing flags on attachment: 389174

Committed r255380: <https://trac.webkit.org/changeset/255380>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59004377>

[Maciej Stachowiak]

The cited regression revision here is probably wrong, since <https://trac.webkit.org/changeset/206365/webkit> only changes TestExpectations.

[Maciej Stachowiak]

Pretty sure this was meant to reference https://trac.webkit.org/changeset/254760/webkit (which is bug 206365)