Closed Bug 559962 Opened 11 years ago Closed 11 years ago

Refactor XPCWrappedNative::CallMethod

Categories

(Core :: General, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: mozilla+ben, Assigned: mozilla+ben)

References

()

Details

Attachments

(1 file)

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.
Summary: Refactor XPCWrappedNative::CallMethod. → Refactor XPCWrappedNative::CallMethod
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
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
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?
(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
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.
Attachment #440076 - Flags: review?(mrbkap)
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.
Attachment #440076 - Flags: review?(mrbkap) → review+
Depends on: 560412
(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.
(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
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?
Looks like to me.
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.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
(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.
You need to log in before you can comment on or make changes to this bug.