Bug 30887 - Improve for..in enumeration performance
Summary: Improve for..in enumeration performance
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-28 15:35 PDT by Oliver Hunt
Modified: 2009-10-29 13:40 PDT (History)
1 user (show)

See Also:


Attachments
Patch v1 (33.92 KB, patch)
2009-10-28 15:46 PDT, Oliver Hunt
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2009-10-28 15:35:42 PDT
Task bug
Comment 1 Oliver Hunt 2009-10-28 15:46:25 PDT
Created attachment 42063 [details]
Patch v1
Comment 2 Geoffrey Garen 2009-10-28 17:08:53 PDT
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.
Comment 3 Oliver Hunt 2009-10-28 18:26:03 PDT
Committed r50254
Comment 4 Eric Seidel (no email) 2009-10-29 13:40:00 PDT
Looks like this was landed.  Closing.