WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2020-01-17 23:22:44 PST
<
rdar://problem/58706877
>
Justin Michaud
Comment 2
2020-01-24 17:23:13 PST
Created
attachment 388747
[details]
Patch
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)
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 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
Created
attachment 389033
[details]
Patch
Justin Michaud
Comment 7
2020-01-28 16:00:43 PST
Created
attachment 389083
[details]
Patch
Justin Michaud
Comment 8
2020-01-30 17:11:15 PST
Created
attachment 389307
[details]
Patch
Justin Michaud
Comment 9
2020-01-31 13:19:28 PST
Created
attachment 389399
[details]
Patch
Justin Michaud
Comment 10
2020-02-03 16:30:52 PST
Created
attachment 389597
[details]
Patch
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
Created
attachment 389706
[details]
Patch
Justin Michaud
Comment 13
2020-02-05 11:22:47 PST
Created
attachment 389832
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug