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

Justin Michaud

Reported 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.

Attachments
Patch (7.45 KB, patch) 2020-01-21 16:28 PST, Justin Michaud	no flags	Details Formatted Diff Diff
Patch (7.49 KB, patch) 2020-01-21 17:58 PST, Justin Michaud	no flags	Details Formatted Diff Diff
Patch (7.46 KB, patch) 2020-01-29 12:45 PST, Justin Michaud	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Justin Michaud

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

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

Justin Michaud

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

Created attachment 388370 [details] Patch

Yusuke Suzuki

Comment 3 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.

Justin Michaud

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

Created attachment 388384 [details] Patch

Yusuke Suzuki

Comment 5 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.

Justin Michaud

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

Created attachment 389174 [details] Patch

WebKit Commit Bot

Comment 7 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>

WebKit Commit Bot

Comment 8 2020-01-29 13:30:35 PST

All reviewed patches have been landed. Closing bug.

Radar WebKit Bug Importer

Comment 9 2020-01-29 13:31:17 PST

<rdar://problem/59004377>