Bug 206557

Summary:

Fix small memory regression caused by r206365

Product:

WebKit

Reporter:

Justin Michaud <justin_michaud>

Component:

JavaScriptCore

Assignee:

Justin Michaud <justin_michaud>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, ews-watchlist, keith_miller, mark.lam, mjs, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Description Justin Michaud 2020-01-21 14:47:13 PST Comment hidden (obsolete)

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

Comment 1 Justin Michaud 2020-01-21 16:11:31 PST

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

Comment 2 Justin Michaud 2020-01-21 16:28:54 PST

Created attachment 388370 [details]
Patch

Comment 3 Yusuke Suzuki 2020-01-21 16:40:56 PST

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.

Comment 4 Justin Michaud 2020-01-21 17:58:59 PST

Created attachment 388384 [details]
Patch

Comment 5 Yusuke Suzuki 2020-01-21 19:22:14 PST

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.

Comment 6 Justin Michaud 2020-01-29 12:45:36 PST

Created attachment 389174 [details]
Patch

Comment 7 WebKit Commit Bot 2020-01-29 13:30:33 PST

Comment on attachment 389174 [details]
Patch

Clearing flags on attachment: 389174

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

Comment 8 WebKit Commit Bot 2020-01-29 13:30:35 PST

All reviewed patches have been landed.  Closing bug.

Comment 9 Radar WebKit Bug Importer 2020-01-29 13:31:17 PST

<rdar://problem/59004377>

Comment 10 Maciej Stachowiak 2020-01-30 22:54:20 PST

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

Comment 11 Maciej Stachowiak 2020-01-30 22:56:33 PST

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