[Webkit-unassigned] [Bug 30887] Improve for..in enumeration performance

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=30887


Geoffrey Garen <ggaren at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #42063|review?                     |review+
               Flag|                            |




--- Comment #2 from Geoffrey Garen <ggaren at apple.com>  2009-10-28 17:08:53 PDT ---
(From update of attachment 42063)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.