Due to the way that numCacheableSlots is currently calculated, static properties on the prototype cause us not to cache any properties at all. We should cache as many properties as possible in the prototype chain until we encounter an object with static properties. This fix will undo a ~2% SunSpider regression caused by http://trac.webkit.org/changeset/147570.
Created attachment 196986 [details] Patch
<rdar://problem/13603937>
Created attachment 196989 [details] Benchmark results ~2% progression, ~41% progression on string-fasta.
Comment on attachment 196986 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=196986&action=review > Source/JavaScriptCore/runtime/JSObject.cpp:1539 > + size_t preStaticPropertyCacheableSlotsCount = propertyNames.numCacheableSlots(); > getClassPropertyNames(exec, object->classInfo(), propertyNames, mode, object->staticFunctionsReified()); > - size_t preStructurePropertyNamesCount = propertyNames.size(); > + size_t preStructurePropertyNamesCount = propertyNames.size() - preStaticPropertyCacheableSlotsCount; > object->structure()->getPropertyNamesFromStructure(exec->globalData(), propertyNames, mode); > - size_t numCacheableSlots = preStructurePropertyNamesCount ? 0 : propertyNames.size(); > + size_t numCacheableSlots = preStructurePropertyNamesCount ? preStaticPropertyCacheableSlotsCount : propertyNames.size(); > propertyNames.setNumCacheableSlots(numCacheableSlots); I buy this but I find it hard to follow. I don't have suggestions for how to clean this logic up, and have no objection to you landing it as-is. But if you could find somewhat of simplifying these conditionals and this arithmetic, it would be super coool.
> I buy this but I find it hard to follow. I don't have suggestions for how to clean this logic up, and have no objection to you landing it as-is. But if you could find somewhat of simplifying these conditionals and this arithmetic, it would be super coool. Yeah, I thought it might be a little convoluted. I'll work on it.
Comment on attachment 196986 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=196986&action=review > Source/JavaScriptCore/ChangeLog:8 > + Due to the way that numCacheableSlots is currently calculated, static properties on the prototype cause us not You must be talking about static *enumerable* properties, right? Under what conditions is this a problem? For example, all properties on the object prototype are not enumerable.
(In reply to comment #6) > (From update of attachment 196986 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=196986&action=review > > > Source/JavaScriptCore/ChangeLog:8 > > + Due to the way that numCacheableSlots is currently calculated, static properties on the prototype cause us not > > You must be talking about static *enumerable* properties, right? Under what conditions is this a problem? > > For example, all properties on the object prototype are not enumerable. Actually, my statement is wrong. It was the fact that we were trying to enumerate properties on the prototype *at all* that was causing us to incorrectly reset numCacheableSlots to 0. Calling getOwnNonIndexPropertyNames on an object would correctly set numCacheableSlots, and then when it was also called on the object's prototype, we would see that we already had a non-empty list of property names and incorrectly reset numCacheableProperties to 0.
Created attachment 196995 [details] Patch
Here's a cleaned up version that explains what's going on a little better. I double-checked performance and verified that this change doesn't break the test I added in the original patch.
Comment on attachment 196995 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=196995&action=review r=me > Source/JavaScriptCore/runtime/JSObject.cpp:1537 > + size_t numCacheableSlotsBeforeStaticProperties = propertyNames.numCacheableSlots(); > getClassPropertyNames(exec, object->classInfo(), propertyNames, mode, object->staticFunctionsReified()); > - size_t preStructurePropertyNamesCount = propertyNames.size(); > + > + bool canCachePropertiesFromStructure = propertyNames.size() == numCacheableSlotsBeforeStaticProperties; I think it would be simpler, and slightly more correct, just to test !propertyNames.size() at this point. The assumption behind cacheable slots is that they start indexing from zero.
Committed r148036: <http://trac.webkit.org/changeset/148036>
Reopening for a follow up fix. If the object doesn't have any properties but the prototype does, we'll assume those prototype properties are accessible in the base object's backing store, which is bad. (<rdar://problem/13613932>)
Created attachment 197357 [details] Patch
I've been unable to reproduce this with a reduced test case, although the site on which the bug manifests (chase.com) no longer crashes.
Created attachment 197393 [details] Patch
Created attachment 197405 [details] Patch
Committed r148142: <http://trac.webkit.org/changeset/148142>
*** Bug 114360 has been marked as a duplicate of this bug. ***