Bug 206430 – Deleting a property should not turn structures into uncacheable dictionaries

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Justin Michaud]

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.

[Radar WebKit Bug Importer]

<rdar://problem/58706877>

[Justin Michaud]

Created attachment 388747 [details]
Patch

[Justin Michaud]

WIP patch to see test results.

[Justin Michaud]

Known omissions:
- the concurrent materialization thing
- tests aren't complete: flatten, testing with inline cache

Preliminary review, plus extra cases to test would be great.

[Saam Barati]

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

[Justin Michaud]

Created attachment 389033 [details]
Patch

[Justin Michaud]

Created attachment 389083 [details]
Patch

[Justin Michaud]

Created attachment 389307 [details]
Patch

[Justin Michaud]

Created attachment 389399 [details]
Patch

[Justin Michaud]

Created attachment 389597 [details]
Patch

[Yusuke Suzuki]

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.

[Justin Michaud]

Created attachment 389706 [details]
Patch

[Justin Michaud]

Created attachment 389832 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 389832 [details]
Patch

Clearing flags on attachment: 389832

Committed r255897: <https://trac.webkit.org/changeset/255897>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.