[webkit-reviews] review denied: [Bug 20589] PropertyMap speedup : [Attachment 23252] Diff for table size 1 /c

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Zoltan Herczeg
<zherczeg at inf.u-szeged.hu>'s request for review:
Bug 20589: PropertyMap speedup
https://bugs.webkit.org/show_bug.cgi?id=20589

Attachment 23252: Diff for table size 1 /c
https://bugs.webkit.org/attachment.cgi?id=23252&action=edit

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
I prefer the 1 item cache because it's a bigger win on SunSpider, and it's
simpler.

I think the best way to solve the JSOBJECT_READONLY problem is simply to move
the definition of property attributes into PropertyMap.h.

+#if DUMP_PROPERTYMAP_STATS
+	 static int numLookupHits;
+#endif

This change is invalid, since it will increase the size of a JavaScript object,
which has a fixed size limit. I'm surprised that JavaScriptCore still builds
with this change applied -- we have compile-time ASSERTs that should cause a
build failure.

I think the best solution is just to leave DUMP_PROPERTYMAP_STATS out of the
patch. Alternatively, you could use a static counter in PropertyMap.cpp for
these stats. (You've already done that for numLookupMiss.) That wouldn't be
thread-safe, but that's OK, since it's just a debugging tool. I would also
rename this #define to DUMP_PROPERTYMAP_CACHE_HITS, and move it to
PropertyMap.cpp.

I don't like the change to make a bunch of get and put wrapper functions around
getInternal and putInternal functions. I think it bloats PropertyMap
substantially. A better solution would be just to write the caching logic
directly into all the existing get and put functions. 

I think the name "putLookup" is far too generic for a function in a class that
already uses the word "put" to mean something else. "recordLookup" and
"clearRecordedLookup" would be better names.

"keyLookup1" is a pretty generic name too. How about "lastLookup"?