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.
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.
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
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
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.
(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).
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
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
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
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.
(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.
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.
(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.
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
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
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
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
> > 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!
2014-04-11 11:44 PDT, Mark Hahnenberg
2014-04-11 13:00 PDT, Build Bot
2014-04-11 13:48 PDT, Build Bot
2014-04-17 21:06 PDT, Mark Hahnenberg
2014-04-17 22:20 PDT, Build Bot
2014-04-17 22:46 PDT, Build Bot
2014-04-17 23:58 PDT, Build Bot
2014-04-18 11:44 PDT, Mark Hahnenberg
2014-04-18 11:46 PDT, Mark Hahnenberg
2014-04-24 17:21 PDT, Mark Hahnenberg
2014-04-24 19:08 PDT, Build Bot
2014-04-24 20:08 PDT, Build Bot
2014-04-24 21:37 PDT, Build Bot
2014-04-25 09:30 PDT, Mark Hahnenberg