Summary: | Speed up JS property enumeration by caching entire PropertyNameArray | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | ||||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Mac | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Sam Weinig
2008-09-18 17:21:46 PDT
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.
|