Bug 206430 - Deleting a property should not turn structures into uncacheable dictionaries
Summary: Deleting a property should not turn structures into uncacheable dictionaries
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Justin Michaud
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-01-17 10:56 PST by Justin Michaud
Modified: 2020-02-05 19:48 PST (History)
10 users (show)

See Also:


Attachments
Patch (34.90 KB, patch)
2020-01-24 17:23 PST, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (40.85 KB, patch)
2020-01-28 11:02 PST, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (43.96 KB, patch)
2020-01-28 16:00 PST, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (51.14 KB, patch)
2020-01-30 17:11 PST, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (51.38 KB, patch)
2020-01-31 13:19 PST, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (51.54 KB, patch)
2020-02-03 16:30 PST, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (53.04 KB, patch)
2020-02-04 14:08 PST, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (52.46 KB, patch)
2020-02-05 11:22 PST, Justin Michaud
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.