RESOLVED FIXED 206430
Deleting a property should not turn structures into uncacheable dictionaries
https://bugs.webkit.org/show_bug.cgi?id=206430
Summary Deleting a property should not turn structures into uncacheable dictionaries
Justin Michaud
Reported 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.
Attachments
Patch (34.90 KB, patch)
2020-01-24 17:23 PST, Justin Michaud
no flags
Patch (40.85 KB, patch)
2020-01-28 11:02 PST, Justin Michaud
no flags
Patch (43.96 KB, patch)
2020-01-28 16:00 PST, Justin Michaud
no flags
Patch (51.14 KB, patch)
2020-01-30 17:11 PST, Justin Michaud
no flags
Patch (51.38 KB, patch)
2020-01-31 13:19 PST, Justin Michaud
no flags
Patch (51.54 KB, patch)
2020-02-03 16:30 PST, Justin Michaud
no flags
Patch (53.04 KB, patch)
2020-02-04 14:08 PST, Justin Michaud
no flags
Patch (52.46 KB, patch)
2020-02-05 11:22 PST, Justin Michaud
no flags
Radar WebKit Bug Importer
Comment 1 2020-01-17 23:22:44 PST
Justin Michaud
Comment 2 2020-01-24 17:23:13 PST
Justin Michaud
Comment 3 2020-01-24 17:24:42 PST
WIP patch to see test results.
Justin Michaud
Comment 4 2020-01-24 17:26:59 PST Comment hidden (obsolete)
Saam Barati
Comment 5 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
Justin Michaud
Comment 6 2020-01-28 11:02:33 PST
Justin Michaud
Comment 7 2020-01-28 16:00:43 PST
Justin Michaud
Comment 8 2020-01-30 17:11:15 PST
Justin Michaud
Comment 9 2020-01-31 13:19:28 PST
Justin Michaud
Comment 10 2020-02-03 16:30:52 PST
Yusuke Suzuki
Comment 11 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.
Justin Michaud
Comment 12 2020-02-04 14:08:01 PST
Justin Michaud
Comment 13 2020-02-05 11:22:47 PST
WebKit Commit Bot
Comment 14 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>
WebKit Commit Bot
Comment 15 2020-02-05 19:48:41 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.