559962 - Refactor XPCWrappedNative::CallMethod

Status: RESOLVED FIXED

(Core :: General, enhancement)

Description

[Ben Newman (:bnewman) (:benjamn)]

XPCWrappedNative::CallMethod is a 755-line function with a whole ton of local/temporary variables and goto statements, cluttered with micro-optimizations and calls to tightly coupled helper functions, some of which have as many as nine parameters.

In short, it's a haven for bugs and a hell for maintainability.

Comment 1

[Ben Newman (:bnewman) (:benjamn)]

Since this bug is very much an exploratory effort, subject to changes in direction at any moment, I won't clutter the attachment list with patches.

Instead, please refer to my patch queue repository, which is hosted here:
http://github.com/benjamn/callmethod

Comment 2

[Ben Newman (:bnewman) (:benjamn)]

The overarching theme of my current approach is to eliminate as many local/temporary variables as possible, make the rest const members of CallMethodHelper, and then decompose CallMethodHelper::Call into smaller methods, preferably const.

Snapshot of my progress so far:
http://github.com/benjamn/callmethod/tree/4544131d4bc7a04e2b9f57c884dbb672717a2107

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

This seems like a decent approach to the problem. My first question is why haven't you just made the XPCCallContext member of CallMethodHelper a reference?

Comment 4

[Ben Newman (:bnewman) (:benjamn)]

(In reply to comment #3)
> My first question is why
> haven't you just made the XPCCallContext member of CallMethodHelper a
> reference?

The original reason was uniformity (the other members are pointers), but making it a reference does save a lot of explicit dereferences.

Implemented:
http://github.com/benjamn/callmethod/blob/master/ccx-reference.diff

Comment 5

[Ben Newman (:bnewman) (:benjamn)]

I just sent this version of my patch queue to the tryserver:
http://github.com/benjamn/callmethod/tree/ce2e03090eb2aec8818509204f82cdea48355915

That's 20 patches in all.  The most interesting are

http://github.com/benjamn/callmethod/blob/master/method-object.diff
http://github.com/benjamn/callmethod/blob/master/memberize-params.diff
http://github.com/benjamn/callmethod/blob/master/eliminate-goto-done.diff
http://github.com/benjamn/callmethod/blob/master/GetDispatchParam.diff

I'll attach a combined diff momentarily.

Comment 6

[Ben Newman (:bnewman) (:benjamn)]

Attachment: All 20 patches combined into one.

I'm posting this patch with r?=mrbkap just to have a record of what was reviewed in bugzilla.  I expect he'll be reviewing the smaller patches individually.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 440076 [details] [diff] [review]
All 20 patches combined into one.

This has been the fastest I've ever reviewed a 54kb patch. Thanks for breaking it up into easily-reviewable sub-patches. The only comments I had were:

* Try using js::LazilyConstructed for the nsAutoString that we lazily construct.
* Nuke the else-after-return in CallMethodHelper::Invoke.
* Kill trailing whitespace!
* Make sure that things get inlined. My one main concern with this patch is that it could potentially add a bunch of function call overhead to an already-slow path. It might be enough to sprinkle |inline| all over CallMethodHelper's private helper functions.

Comment 8

[Ben Newman (:bnewman) (:benjamn)]

(In reply to comment #7)
> (From update of attachment 440076 [details] [diff] [review])
> This has been the fastest I've ever reviewed a 54kb patch. Thanks for breaking
> it up into easily-reviewable sub-patches.

Glad you liked it :D

> * Try using js::LazilyConstructed for the nsAutoString that we lazily
> construct.

Filed bug 560412 (with patch) to make this possible, then switched the mAutoString type:
http://github.com/benjamn/callmethod/blob/2af2a180/expose-LazilyConstructed-values.diff

> * Nuke the else-after-return in CallMethodHelper::Invoke.

Updated the existing patch:
http://github.com/benjamn/callmethod/blob/2af2a180/extract-invoke.diff

> * Kill trailing whitespace!

http://github.com/benjamn/callmethod/blob/2af2a180/delete-trailing-whitespace.diff

> * Make sure that things get inlined. My one main concern with this patch is
> that it could potentially add a bunch of function call overhead to an
> already-slow path. It might be enough to sprinkle |inline| all over
> CallMethodHelper's private helper functions.

Using JS_ALWAYS_INLINE:
http://github.com/benjamn/callmethod/blob/2af2a180/add-JS_ALWAYS_INLINEs.diff

Still need to run through dromaeo.  I'll report back.

Comment 9

[Ben Newman (:bnewman) (:benjamn)]

(In reply to comment #8)
> > * Try using js::LazilyConstructed for the nsAutoString that we lazily
> > construct.
> 
> Filed bug 560412 (with patch) to make this possible, then switched the
> mAutoString type:
> http://github.com/benjamn/callmethod/blob/2af2a180/expose-LazilyConstructed-values.diff

Also:
http://github.com/benjamn/callmethod/blob/2af2a180/lazily-construct-mAutoString.diff

Comment 10

[Ben Newman (:bnewman) (:benjamn)]

Dromaeo results without these patches applied:
184.49runs/s
http://dromaeo.com/?id=100629

Dromaeo results with these patches applied:
184.08runs/s
http://dromaeo.com/?id=100634

That seems within tolerances, right?

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Looks like to me.

Comment 12

[Ben Newman (:bnewman) (:benjamn)]

It is done:
http://hg.mozilla.org/mozilla-central/rev/4e8ed7f34b0b

I would note that there is still plenty of room for further cleaning up this code, but I've done as much as I can justify for now.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Ben Newman (:bnewman) (:benjamn) from comment #12)
> I would note that there is still plenty of room for further cleaning up this
> code, but I've done as much as I can justify for now.

I'm picking up where this left off in bug 683802.