Bug 30887 – Improve for..in enumeration performance

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

[Oliver Hunt]

Task bug

[Oliver Hunt]

Created attachment 42063 [details]
Patch v1

[Geoffrey Garen]

Comment on attachment 42063 [details]
Patch v1

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.

[Oliver Hunt]

Committed r50254

[Eric Seidel (no email)]

Looks like this was landed.  Closing.