Bug 114235 – JSObject::getOwnNonIndexPropertyNames calculates numCacheableSlots incorrectly

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Hahnenberg]

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.

[Mark Hahnenberg]

Created attachment 196986 [details]
Patch

[Mark Hahnenberg]

<rdar://problem/13603937>

[Mark Hahnenberg]

Created attachment 196989 [details]
Benchmark results

~2% progression, ~41% progression on string-fasta.

[Filip Pizlo]

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.

[Mark Hahnenberg]

> 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.

[Geoffrey Garen]

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.

[Mark Hahnenberg]

(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.

[Mark Hahnenberg]

Created attachment 196995 [details]
Patch

[Mark Hahnenberg]

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.

[Geoffrey Garen]

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.

[Mark Hahnenberg]

Committed r148036: <http://trac.webkit.org/changeset/148036>

[Mark Hahnenberg]

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>)

[Mark Hahnenberg]

Created attachment 197357 [details]
Patch

[Mark Hahnenberg]

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.

[Mark Hahnenberg]

Created attachment 197393 [details]
Patch

[Mark Hahnenberg]

Created attachment 197405 [details]
Patch

[Mark Hahnenberg]

Committed r148142: <http://trac.webkit.org/changeset/148142>

[Alexey Proskuryakov]

*** Bug 114360 has been marked as a duplicate of this bug. ***