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.
Created attachment 23556 [details] cache entire PropertyNameArray
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.
(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. >
Created attachment 23585 [details] updated patch
Created attachment 23588 [details] updated
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.
Fixed in r36694.