WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
REOPENED
131551
Deleting properties poisons objects
https://bugs.webkit.org/show_bug.cgi?id=131551
Summary
Deleting properties poisons objects
Mark Hahnenberg
Reported
2014-04-11 11:38:05 PDT
A rather vitriolic title, but it's sort of true. Deleting a property on an object causes it to transition to an uncacheable dictionary structure. We then gather no information on that object in the baseline JIT's inline caches, which then causes the higher tier JITs to do fully generic GetById/PutById operations on these objects which is very slow. The idea behind uncacheable dictionaries is that objects whose properties are deleted are being used like a dictionary to create and remove properties. We'd like to avoid creating tons of Structures for these objects. Additionally, re-use of previously deleted property offsets would wreak havoc on our inline caches. However, for objects where a small set of properties are added or removed, we should try to avoid dooming them to an eternity of slowness. Currently the first delete transitions to the uncacheable dictionary mode, but we could increase that limit to allow objects that are only involved in a small number of deletes to continue to function normally.
Attachments
Patch
(5.36 KB, patch)
2014-04-11 11:44 PDT
,
Mark Hahnenberg
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2
(611.56 KB, application/zip)
2014-04-11 13:00 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion
(593.17 KB, application/zip)
2014-04-11 13:48 PDT
,
Build Bot
no flags
Details
Patch
(12.13 KB, patch)
2014-04-17 21:06 PDT
,
Mark Hahnenberg
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2
(660.04 KB, application/zip)
2014-04-17 22:20 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion
(668.92 KB, application/zip)
2014-04-17 22:46 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion
(697.83 KB, application/zip)
2014-04-17 23:58 PDT
,
Build Bot
no flags
Details
Patch
(14.54 KB, patch)
2014-04-18 11:44 PDT
,
Mark Hahnenberg
no flags
Details
Formatted Diff
Diff
Patch
(14.57 KB, patch)
2014-04-18 11:46 PDT
,
Mark Hahnenberg
no flags
Details
Formatted Diff
Diff
Fixed some issues
(18.98 KB, patch)
2014-04-24 17:21 PDT
,
Mark Hahnenberg
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
(492.58 KB, application/zip)
2014-04-24 19:08 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion
(492.65 KB, application/zip)
2014-04-24 20:08 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2
(463.58 KB, application/zip)
2014-04-24 21:37 PDT
,
Build Bot
no flags
Details
Patch
(18.00 KB, patch)
2014-04-25 09:30 PDT
,
Mark Hahnenberg
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Mark Hahnenberg
Comment 1
2014-04-11 11:44:27 PDT
Created
attachment 229144
[details]
Patch
Geoffrey Garen
Comment 2
2014-04-11 11:50:28 PDT
Comment on
attachment 229144
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=229144&action=review
r=me
> Source/JavaScriptCore/runtime/Structure.cpp:472 > + if (structure->m_forgivenDeletes < s_maxForgivenDeletes) { > + Structure* transition = create(vm, structure);
Another interesting way to forgive a delete would be to notice that you were deleting the last property added, and tradition backward. I believe we already have all the information needed to do that, since each Structure points to its previous, and holds the string that it added to the set of strings in the property table (m_nameInPrevious). You would also need to verify that going backward did not violate other invariants. For example, if going forward grew your backing store, it might not be valid to go backward anymore.
Geoffrey Garen
Comment 3
2014-04-11 11:50:38 PDT
Can you add a JSRegress test for this?
Build Bot
Comment 4
2014-04-11 13:00:03 PDT
Comment on
attachment 229144
[details]
Patch
Attachment 229144
[details]
did not pass mac-wk2-ews (mac-wk2): Output:
http://webkit-queues.appspot.com/results/6447102225809408
New failing tests: jquery/manipulation.html js/pic/undictionary.html js/named-function-expression.html storage/indexeddb/cursor-value.html js/preventExtensions.html js/array-functions-non-arrays.html jquery/attributes.html js/dom/constructor-attributes.html jquery/deferred.html jquery/data.html js/mozilla/strict/8.12.7.html jquery/event.html js/dictionary-prototype-caching.html
Build Bot
Comment 5
2014-04-11 13:00:05 PDT
Created
attachment 229155
[details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-13 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Mark Hahnenberg
Comment 6
2014-04-11 13:00:48 PDT
Yeah, this borked some stuff. Investigating...
Build Bot
Comment 7
2014-04-11 13:48:28 PDT
Comment on
attachment 229144
[details]
Patch
Attachment 229144
[details]
did not pass mac-ews (mac): Output:
http://webkit-queues.appspot.com/results/6682139479244800
New failing tests: jquery/manipulation.html js/pic/undictionary.html js/named-function-expression.html js/preventExtensions.html js/array-functions-non-arrays.html jquery/attributes.html js/dom/constructor-attributes.html jquery/deferred.html jquery/data.html js/mozilla/strict/8.12.7.html jquery/event.html js/dictionary-prototype-caching.html
Build Bot
Comment 8
2014-04-11 13:48:30 PDT
Created
attachment 229158
[details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-07 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Geoffrey Garen
Comment 9
2014-04-14 21:25:56 PDT
Comment on
attachment 229144
[details]
Patch You should fix these failing tests by pinning a property table when doing a delete transition. Our code only knows how to reconstruct a property table through insertions, not deletions, so we have to hang onto a property table that indicates deletions.
Mark Hahnenberg
Comment 10
2014-04-15 07:49:41 PDT
(In reply to
comment #9
)
> (From update of
attachment 229144
[details]
) > You should fix these failing tests by pinning a property table when doing a delete transition. Our code only knows how to reconstruct a property table through insertions, not deletions, so we have to hang onto a property table that indicates deletions.
Unfortunately pinning doesn't quite fix this issue. When we pin a property table we also clear its m_nameInPrevious which is used as a key in the previous Structure's m_transitionTable. If we want to be able to cache these transitions then we need to "pin" in such a way that we can't go back farther in the transition chain when doing materializePropertyMap (e.g. previousID should be null), but that we can still use in the previous Structure's transition table. Or we could just give up on caching delete transitions, but allow future transitions from the deleted Structure to be cached (which pin()-ing would give us).
Mark Hahnenberg
Comment 11
2014-04-17 21:06:07 PDT
Created
attachment 229620
[details]
Patch
Mark Hahnenberg
Comment 12
2014-04-17 21:33:03 PDT
Comment on
attachment 229620
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=229620&action=review
> Source/JavaScriptCore/ChangeLog:8 > + This is ~6% progression on Dromaeo.
Correction. This is a 6% speedup on the jslib portion of Dromaeo. I will do a full run as well.
Build Bot
Comment 13
2014-04-17 22:20:51 PDT
Comment on
attachment 229620
[details]
Patch
Attachment 229620
[details]
did not pass mac-wk2-ews (mac-wk2): Output:
http://webkit-queues.appspot.com/results/6102370081570816
New failing tests: jquery/data.html
Build Bot
Comment 14
2014-04-17 22:20:53 PDT
Created
attachment 229628
[details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-16 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Build Bot
Comment 15
2014-04-17 22:46:08 PDT
Comment on
attachment 229620
[details]
Patch
Attachment 229620
[details]
did not pass mac-ews (mac): Output:
http://webkit-queues.appspot.com/results/6178124664406016
New failing tests: jquery/data.html
Build Bot
Comment 16
2014-04-17 22:46:12 PDT
Created
attachment 229630
[details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-02 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Build Bot
Comment 17
2014-04-17 23:58:54 PDT
Comment on
attachment 229620
[details]
Patch
Attachment 229620
[details]
did not pass mac-ews (mac): Output:
http://webkit-queues.appspot.com/results/5751994165755904
New failing tests: jquery/data.html
Build Bot
Comment 18
2014-04-17 23:58:57 PDT
Created
attachment 229634
[details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-07 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Geoffrey Garen
Comment 19
2014-04-18 09:44:58 PDT
Comment on
attachment 229620
[details]
Patch EWS test failure looks real.
Mark Hahnenberg
Comment 20
2014-04-18 10:20:32 PDT
(In reply to
comment #19
)
> (From update of
attachment 229620
[details]
) > EWS test failure looks real.
Yep :-(
Mark Hahnenberg
Comment 21
2014-04-18 10:50:16 PDT
The test that's failing is the following: var obj = { foo: "bar" }; jQuery(obj).data("foo", "baz"); dataObj = jQuery.extend(true, {}, jQuery(obj).data()); // TODO: Remove this hack which was introduced for 1.5.1 delete dataObj.toJSON; deepEqual( dataObj, { foo: "baz" }, "Retrieve data object from a wrapped JS object (#7524)" ); Somehow the delete operation is screwing up dataObj to where it's no longer deepEqual to {foo: "baz"}. Still investigating why this would happen.
Mark Hahnenberg
Comment 22
2014-04-18 10:57:55 PDT
(In reply to
comment #21
)
> The test that's failing is the following: > > var obj = { foo: "bar" }; > jQuery(obj).data("foo", "baz"); > > dataObj = jQuery.extend(true, {}, jQuery(obj).data()); > > // TODO: Remove this hack which was introduced for 1.5.1 > delete dataObj.toJSON; > > deepEqual( dataObj, { foo: "baz" }, "Retrieve data object from a wrapped JS object (#7524)" ); > > Somehow the delete operation is screwing up dataObj to where it's no longer deepEqual to {foo: "baz"}. Still investigating why this would happen.
deepEqual() uses for-in, so maybe JSPropertyNameIterator is confused about things.
Mark Hahnenberg
Comment 23
2014-04-18 11:43:30 PDT
> deepEqual() uses for-in, so maybe JSPropertyNameIterator is confused about things.
Seems like that was the issue. Re-running test and benchmarks.
Mark Hahnenberg
Comment 24
2014-04-18 11:44:40 PDT
Created
attachment 229659
[details]
Patch
Mark Hahnenberg
Comment 25
2014-04-18 11:46:16 PDT
Created
attachment 229660
[details]
Patch
Geoffrey Garen
Comment 26
2014-04-18 11:49:35 PDT
Comment on
attachment 229660
[details]
Patch r=me
Geoffrey Garen
Comment 27
2014-04-18 11:50:01 PDT
Comment on
attachment 229660
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=229660&action=review
> Source/JavaScriptCore/runtime/JSPropertyNameIterator.cpp:67 > + if (o->structure()->isDictionary() || o->structure()->hasDeletedOffsets()) > return jsPropertyNameIterator;
You should adda comment explaining that the iterator doesn't know how to skip deleted buckets.
Mark Hahnenberg
Comment 28
2014-04-18 13:08:20 PDT
Committed
r167501
: <
http://trac.webkit.org/changeset/167501
>
Andreas Kling
Comment 29
2014-04-19 14:53:01 PDT
(In reply to
comment #28
)
> Committed
r167501
: <
http://trac.webkit.org/changeset/167501
>
I think this may be causing DoYouEvenBench to crash reliably on bawts:
http://build.webkit.org/builders/Apple%20MountainLion%20Release%20%28Perf%29?numbuilds=50
WebKit Commit Bot
Comment 30
2014-04-20 04:41:37 PDT
Re-opened since this is blocked by
bug 131913
mitz
Comment 31
2014-04-20 21:51:47 PDT
(In reply to
comment #28
)
> Committed
r167501
: <
http://trac.webkit.org/changeset/167501
>
This change also broke the Find My iPhone web interface at icloud.com, but since it’s already been reverted, I didn’t file a bug about that.
Mark Hahnenberg
Comment 32
2014-04-24 17:21:24 PDT
Created
attachment 230122
[details]
Fixed some issues
Build Bot
Comment 33
2014-04-24 19:08:32 PDT
Comment on
attachment 230122
[details]
Fixed some issues
Attachment 230122
[details]
did not pass mac-ews (mac): Output:
http://webkit-queues.appspot.com/results/5381299967623168
New failing tests: js/regress/delete-a-few-properties-then-get-by-id.html
Build Bot
Comment 34
2014-04-24 19:08:37 PDT
Created
attachment 230130
[details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Build Bot
Comment 35
2014-04-24 20:08:22 PDT
Comment on
attachment 230122
[details]
Fixed some issues
Attachment 230122
[details]
did not pass mac-ews (mac): Output:
http://webkit-queues.appspot.com/results/5737659846623232
New failing tests: js/regress/delete-a-few-properties-then-get-by-id.html
Build Bot
Comment 36
2014-04-24 20:08:28 PDT
Created
attachment 230132
[details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-04 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Build Bot
Comment 37
2014-04-24 21:37:43 PDT
Comment on
attachment 230122
[details]
Fixed some issues
Attachment 230122
[details]
did not pass mac-wk2-ews (mac-wk2): Output:
http://webkit-queues.appspot.com/results/5348661437399040
New failing tests: js/regress/delete-a-few-properties-then-get-by-id.html
Build Bot
Comment 38
2014-04-24 21:37:48 PDT
Created
attachment 230136
[details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-11 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Mark Hahnenberg
Comment 39
2014-04-25 09:30:29 PDT
Created
attachment 230183
[details]
Patch
Oliver Hunt
Comment 40
2014-04-25 10:48:37 PDT
Comment on
attachment 230183
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=230183&action=review
> Source/JavaScriptCore/ChangeLog:15 > + (JSC::PropertyTable::hadDeletedOffset): If we ever had deleted properties we can no longer cache offsets when > + iterating properties because we're required to iterate properties in insertion order.
yay javascript!!!
> Source/JavaScriptCore/ChangeLog:21 > + (JSC::Structure::removePropertyTransition): We allow up to 5 deletes for a particular path through the tree of > + Structure transitions. After that, we convert to an uncacheable dictionary like we used to. We don't cache > + delete transitions, but we allow transitioning from them.
how did we choose 5? Is it possible to use 2^n (±1?) so it looks deliberate? :D
Mark Hahnenberg
Comment 41
2014-04-25 10:55:46 PDT
> > Source/JavaScriptCore/ChangeLog:21 > > + (JSC::Structure::removePropertyTransition): We allow up to 5 deletes for a particular path through the tree of > > + Structure transitions. After that, we convert to an uncacheable dictionary like we used to. We don't cache > > + delete transitions, but we allow transitioning from them. > > how did we choose 5? Is it possible to use 2^n (±1?) so it looks deliberate? :D
I picked the smallest number that achieved the maximal speedup on a benchmark affected by this optimization. 1-4 produced speedups, but not as much as 5, and anything more than 5 had no effect. Super scientific!
WebKit Commit Bot
Comment 42
2014-04-28 10:27:01 PDT
Comment on
attachment 230183
[details]
Patch Clearing flags on attachment: 230183 Committed
r167889
: <
http://trac.webkit.org/changeset/167889
>
WebKit Commit Bot
Comment 43
2014-04-28 10:27:07 PDT
All reviewed patches have been landed. Closing bug.
Ryosuke Niwa
Comment 44
2014-05-02 10:59:50 PDT
This patch appears to have caused ~1.5% regression on DYEBench:
https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-mavericks%22%2C%22DoYouEvenBench%2FFull%3ATime%3ATotal%22%5D%2C%5B%22mac-mountainlion%22%2C%22DoYouEvenBench%2FFull%3ATime%3ATotal%22%5D%5D&days=17
Mark Hahnenberg
Comment 45
2014-05-06 11:57:11 PDT
This broke things. Reopening for a rollout. We're going to reevaluate how we do Structure transitions in general as part of
bug 132618
.
Mark Hahnenberg
Comment 46
2014-05-06 12:18:32 PDT
Rolled out in
http://trac.webkit.org/changeset/168373
.
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