Bug 114235

Summary: JSObject::getOwnNonIndexPropertyNames calculates numCacheableSlots incorrectly
Product: WebKit Reporter: Mark Hahnenberg <mhahnenberg>
Component: JavaScriptCoreAssignee: Mark Hahnenberg <mhahnenberg>
Status: RESOLVED FIXED    
Severity: Normal CC: sanford-webkit-bugzilla
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Benchmark results
none
Patch
none
Patch
none
Patch
none
Patch fpizlo: review+

Description Mark Hahnenberg 2013-04-08 20:39:56 PDT
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.
Comment 1 Mark Hahnenberg 2013-04-08 20:42:33 PDT
Created attachment 196986 [details]
Patch
Comment 2 Mark Hahnenberg 2013-04-08 20:43:13 PDT
<rdar://problem/13603937>
Comment 3 Mark Hahnenberg 2013-04-08 20:47:03 PDT
Created attachment 196989 [details]
Benchmark results

~2% progression, ~41% progression on string-fasta.
Comment 4 Filip Pizlo 2013-04-08 21:04:30 PDT
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.
Comment 5 Mark Hahnenberg 2013-04-08 21:14:08 PDT
> 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 6 Geoffrey Garen 2013-04-08 21:14:47 PDT
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.
Comment 7 Mark Hahnenberg 2013-04-08 21:52:49 PDT
(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.
Comment 8 Mark Hahnenberg 2013-04-08 22:01:27 PDT
Created attachment 196995 [details]
Patch
Comment 9 Mark Hahnenberg 2013-04-08 22:02:38 PDT
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 10 Geoffrey Garen 2013-04-08 23:50:51 PDT
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.
Comment 11 Mark Hahnenberg 2013-04-09 12:14:51 PDT
Committed r148036: <http://trac.webkit.org/changeset/148036>
Comment 12 Mark Hahnenberg 2013-04-10 08:33:23 PDT
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>)
Comment 13 Mark Hahnenberg 2013-04-10 12:25:33 PDT
Created attachment 197357 [details]
Patch
Comment 14 Mark Hahnenberg 2013-04-10 12:27:46 PDT
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.
Comment 15 Mark Hahnenberg 2013-04-10 13:16:33 PDT
Created attachment 197393 [details]
Patch
Comment 16 Mark Hahnenberg 2013-04-10 14:22:35 PDT
Created attachment 197405 [details]
Patch
Comment 17 Mark Hahnenberg 2013-04-10 15:24:11 PDT
Committed r148142: <http://trac.webkit.org/changeset/148142>
Comment 18 Alexey Proskuryakov 2013-04-10 22:06:37 PDT
*** Bug 114360 has been marked as a duplicate of this bug. ***