Closed Bug 559962 Opened 11 years ago Closed 11 years ago
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 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.
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+
(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.