[webkit-reviews] review granted: [Bug 30887] Improve for..in enumeration performance : [Attachment 42063] Patch v1

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Oliver Hunt <oliver at apple.com>'s
request for review:
Bug 30887: Improve for..in enumeration performance
https://bugs.webkit.org/show_bug.cgi?id=30887

Attachment 42063: Patch v1
https://bugs.webkit.org/attachment.cgi?id=42063&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
Looks good. A few comments:

(1) No performance numbers. Is this a speedup?

(2) op_get_by_pname:

+	 if (subscript == expectedSubscript && baseValue.isObject() &&
(asObject(baseValue)->structure() == it->cachedStructure()) &&
it->getOffset(index, offset)) {

Use isCell instead of isObject -- isCell is faster. Testing against the cached
structure ensures that, if the object was an object last time, it's an object
this time, too.

(3) op_get_by_pname stub:

Same comment. Also, don't waste time testing for the fast case in the stub;
you're in the stub because the inline fast case has failed.

(4) JSValue and OBJECT_OFFSETOF:

I dont't think the refactoring you did in this area was necessary. See other
code that uses the idiom "OBJECT_OFFSETOF(JSValue, u.asBits.XXX)" -- you don't
need to name the type of the union to get offsets into it.

(5) Register idioms on JSVALUE32_64:

+    emitLoad2(property, regT1, regT2, base, regT3, regT0);

The idiom for code like this is: emitLoad2(x, regT1, regT0, y, regT3, regT2).
Following this idiom ensures that result chaining doesn't have to do any
register-to-register moves. (It also ensures that you don't accidentally tickle
any untested edge cases in the result chaining logic.)

+    emitStore(dst, regT0, regT1);

Here, the idiom is: emitStore(dst, regT1, regT0);

You want to end emit_op_get_by_pname with a call to "map", to map the virtual
register into physical registers for result chaining. That way, the code that
uses the result of the bracket access doesn't have to stall, reloading it. The
form for map is:

    map(m_bytecodeIndex + OPCODE_LENGTH(op_get_by_pname), dst, tagRegister,
payloadRegister);

Be sure that the result is loaded into the same tag/payload registers on the
hot and cold paths. Currently, it is not, so the cold path would cause a crash
if you just added the call to map without changing the register allocation.

I think your patch will work as-is, but making these improvements will make it
even faster and even more readable.