Bug 20928 – Speed up JS property enumeration by caching entire PropertyNameArray

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

[Sam Weinig]

We can speed up JavaScript property enumeration (for .. in) by caching the entire PropertyNameArray in the StructureID instead of just the properties in the PropertyMap.

[Sam Weinig]

Created attachment 23556 [details]
cache entire PropertyNameArray

[Darin Adler]

Comment on attachment 23556 [details]
cache entire PropertyNameArray

JSPropertyNameIterator::invalidate is so simple now it should probably be inlined. Maybe also use clear() instead of assigning 0?

+    , m_data(0)

Don't need that for RefPtr.

+inline JSPropertyNameIterator::JSPropertyNameIterator(JSObject* object, PropertyNameArrayData* propertyNameArrayData)

Should PropertyNameArrayData* should be PassRefPtr, since this takes ownership.

+        void setCachedPrototypeChain(StructureIDChain* cachedPrototypeChain) { m_cachedPrototypeChain = cachedPrototypeChain; }

+        void setData(PropertyNameArrayData* data) { m_data = data; } 

Should be PassRefPtr since these take ownership.

+        if ((*a).get() != (*b).get())
+            return false;

These would read better with -> but can't you just omit the get() altogether?

+        if (!(*a).get())
+            return true;

This would read better with -> but can't you just omit the get() altogether?

+    ASSERT_NOT_REACHED();
+    return false;

This will just create a warning on Windows and make the build fail. Leave it out.

+    // FIXME: This breaks the optimization for the JSGlobalObject. 

I think you should write a clearer comment. Does this really need a FIXME?

I'm going to say review- because of the number of actionable comments.

[Sam Weinig]

(In reply to comment #2)
> (From update of attachment 23556 [details] [edit])
> JSPropertyNameIterator::invalidate is so simple now it should probably be
> inlined. Maybe also use clear() instead of assigning 0?
> 
> +    , m_data(0)
> 
> Don't need that for RefPtr.

Fixed.

> 
> +inline JSPropertyNameIterator::JSPropertyNameIterator(JSObject* object,
> PropertyNameArrayData* propertyNameArrayData)

Fixed.

> 
> Should PropertyNameArrayData* should be PassRefPtr, since this takes ownership.
> 
> +        void setCachedPrototypeChain(StructureIDChain* cachedPrototypeChain) {
> m_cachedPrototypeChain = cachedPrototypeChain; }
> 
> +        void setData(PropertyNameArrayData* data) { m_data = data; } 
> 
> Should be PassRefPtr since these take ownership.

I don't think this is true, because the ownership is actually shared.

> 
> +        if ((*a).get() != (*b).get())
> +            return false;
> 
> These would read better with -> but can't you just omit the get() altogether?
> 
> +        if (!(*a).get())
> +            return true;
> 
> This would read better with -> but can't you just omit the get() altogether?

Fixed

> 
> +    ASSERT_NOT_REACHED();
> +    return false;
> 
> This will just create a warning on Windows and make the build fail. Leave it
> out.
> 

Fixed.

> +    // FIXME: This breaks the optimization for the JSGlobalObject. 
> 
> I think you should write a clearer comment. Does this really need a FIXME?

Removed.

> 
> I'm going to say review- because of the number of actionable comments.
> 

[Sam Weinig]

Created attachment 23585 [details]
updated patch

[Sam Weinig]

Created attachment 23588 [details]
updated

[Darin Adler]

Comment on attachment 23588 [details]
updated

r=me

We discussed some things you forgot to do in person, and I think it would be good to do them, but the patch is super-great to land even as-is.

[Sam Weinig]

Fixed in r36694.