Bug 206430

Summary: Deleting a property should not turn structures into uncacheable dictionaries
Product: WebKit Reporter: Justin Michaud <justin_michaud>
Component: JavaScriptCoreAssignee: Justin Michaud <justin_michaud>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, fpizlo, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Justin Michaud 2020-01-17 10:56:54 PST 
Right now, deleteProperty/removePropertyTransition causes a structure transition to uncacheable dictionary. Instead, we should allow it to transition to a new regular structure like adding a property does. This means that we have to:

1) Break the assumption that structure transition offsets increase monotonically
2) Add a new transition map and transition kind for deletes
3) Find some heuristic for when we should actually transition to uncacheable dictionary.
Comment 1 Radar WebKit Bug Importer 2020-01-17 23:22:44 PST 
<rdar://problem/58706877>
Comment 2 Justin Michaud 2020-01-24 17:23:13 PST 
Created attachment 388747 [details]
Patch
Comment 3 Justin Michaud 2020-01-24 17:24:42 PST 
WIP patch to see test results.
Comment 4 Justin Michaud 2020-01-24 17:26:59 PST Comment hidden (obsolete)
Comment 5 Saam Barati 2020-01-27 18:10:25 PST 
Comment on attachment 388747 [details]
Patch

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

> Source/JavaScriptCore/runtime/Structure.cpp:577
> +    return 0;

nullptr
Comment 6 Justin Michaud 2020-01-28 11:02:33 PST 
Created attachment 389033 [details]
Patch
Comment 7 Justin Michaud 2020-01-28 16:00:43 PST 
Created attachment 389083 [details]
Patch
Comment 8 Justin Michaud 2020-01-30 17:11:15 PST 
Created attachment 389307 [details]
Patch
Comment 9 Justin Michaud 2020-01-31 13:19:28 PST 
Created attachment 389399 [details]
Patch
Comment 10 Justin Michaud 2020-02-03 16:30:52 PST 
Created attachment 389597 [details]
Patch
Comment 11 Yusuke Suzuki 2020-02-04 13:49:49 PST 
Comment on attachment 389597 [details]
Patch

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

r=me

> Source/JavaScriptCore/runtime/PropertyMapHashTable.h:360
> +#ifndef NDEBUG

Use #if ASSERT_ENABLED

> Source/JavaScriptCore/runtime/PropertyMapHashTable.h:497
> +#ifndef NDEBUG

Ditto.

> Source/JavaScriptCore/runtime/Structure.cpp:564
> +    if (Structure* existingTransition = structure->m_transitionTable.get(propertyName.uid(), attributes, false)) {

Instead of passing `false`, let's pass,

constexpr bool isAddition = false;
if (Structure* existingTransition = structure->m_transitionTable.get(propertyName.uid(), attributes, isAddition)) {

> Source/JavaScriptCore/runtime/Structure.cpp:581
> +    for (auto* s = structure; s; s = s->previousID())
> +        ++transitionCount;

Let's stop this iteration when transitionCount exceeds s_maxTransitionLength

> Source/JavaScriptCore/runtime/StructureTransitionTable.h:185
> +    bool contains(UniquedStringImpl*, unsigned attributes, bool isAddition = true) const;
> +    Structure* get(UniquedStringImpl*, unsigned attributes, bool isAddition = true) const;

I think specifying `isAddition` boolean in the caller side is easier to read instead of using default parameter.
Comment 12 Justin Michaud 2020-02-04 14:08:01 PST 
Created attachment 389706 [details]
Patch
Comment 13 Justin Michaud 2020-02-05 11:22:47 PST 
Created attachment 389832 [details]
Patch
Comment 14 WebKit Commit Bot 2020-02-05 19:48:39 PST 
Comment on attachment 389832 [details]
Patch

Clearing flags on attachment: 389832

Committed r255897: <https://trac.webkit.org/changeset/255897>
Comment 15 WebKit Commit Bot 2020-02-05 19:48:41 PST 
All reviewed patches have been landed.  Closing bug.