Last Comment Bug 25180 - (maybe) optimize xpconnect wrapping for JS -> JS communication
: (maybe) optimize xpconnect wrapping for JS -> JS communication
Status: NEW
: perf
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: x86 All
: P3 normal with 1 vote (vote)
: Future
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
: 28120 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2000-01-27 01:19 PST by John Bandhauer
Modified: 2009-06-28 04:48 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed interim patch using wrappers around wrappers (12.09 KB, patch)
2000-07-27 22:49 PDT, John Bandhauer
no flags Details | Diff | Review

Description John Bandhauer 2000-01-27 01:19:10 PST
Currently when JS code calls some interface that returns an xpcom object that 
happens to be implemented in JS then that object is received by the JS caller as 
a plain JS object; i.e. it is unwrapped for direct JS to JS calls.

This is efficient, but it undermines some of the cool support for xpcom objects 
built into xpconnect. And it is counter to the goal of transparent use of 
components regardless of implementation language because it makes 
unavailable some of the features available to JS code that uses a native 
interface. For instance... 

- automatic handling of QI
- "foo instanceof Components.interfaces.nsIFoo"
- foo.toString()
- the "foo.nsIFoo" for QI scheme proposed in bug 24694
- flattening

In order to really make JS components work right for JS callers I propose to 
implement a new kind of wrapper (wrappedJS4JS?) with its own JSOps. This should 
be pretty easy to implement. I can steal and convert a lot of the code I need 
from existing code.
Comment 1 Mike Shaver (:shaver -- probably not reading bugmail closely) 2000-01-27 01:30:44 PST
Is that the case right now?

When I getService on a JS-implemented component, I get an ``[xpconnect wrapped
nsIWhatever]'' back.  But methods on that object that return JS objects as
implementations of interfaces do exhibit the unwrapping (never-wrapping?) stuff
that you're talking about.

Comment 2 John Bandhauer 2000-01-27 01:52:31 PST
You are seeing the "wrappers around wrappers" situation that I checked in a fix 
for last night. createInstance and getService were calling wrapNative 
regardless of the actual language of the component:
http://bugzilla.mozilla.org/show_bug.cgi?id=16130
In today's build you won't see that.
Comment 3 Robert Ginda 2000-02-17 11:21:12 PST
*** Bug 28120 has been marked as a duplicate of this bug. ***
Comment 4 John Bandhauer 2000-07-27 22:49:13 PDT
Created attachment 12058 [details] [diff] [review]
proposed interim patch using wrappers around wrappers
Comment 5 John Bandhauer 2000-07-27 22:52:17 PDT
The patch above uses wrappers around wrappers to simulate the effect of the 
JS -> JS wrappers. This will give us the consistent behavior we want. It can be 
transparently replaced with the better wrappers later.

I added support for the wrapper user to ask for the underlying JSObject. This 
will only succeed if the underlying object has decided to allow it AND the 
security manager does not veto. See the comments.
Comment 6 John Bandhauer 2000-07-27 22:59:34 PDT
nominating for nsbeta3 because JS components are happen'n and we don't want to 
break them later when we add this insulation layer.
Comment 7 Warren Harris 2000-08-01 20:10:02 PDT
John: nsbeta3 goes in the keyword field, not status whiteboard. 
Comment 8 John Bandhauer 2000-08-01 22:33:39 PDT
Interim patch checked in. The long term fix still needs to be done. 

Clearing "nsbeta2" decorations.
Comment 9 John Bandhauer 2001-05-07 00:02:02 PDT
mass reassign of xpconnect bugs to dbradley@netscape.com
Comment 10 David Bradley 2001-06-15 10:00:23 PDT
Removing Rob as QA contact
Comment 11 Magnus Melin 2006-05-19 12:21:49 PDT
This shouldn't be an in issue anymore...
Comment 12 David Bradley 2006-05-19 12:44:33 PDT
Can you provide a little more explanation.

From reading the bug, it was left open for a longer term fix that hasn't happened -> "Interim patch checked in. The long term fix still needs to be done."

If we're not going to bother with the long term fix, that's fine, but it should be stated in closing this bug.
Comment 13 Magnus Melin 2006-05-19 12:57:17 PDT
Aah, missed the part about "long term fix"... Should I reopen? Or feel free to do it for me.

(I just found the bug searching something related, was first led to believe it wouldn't work, found out it works, thought it just had been forgotten/fixed elsewhere)
Comment 14 David Bradley 2006-05-25 05:46:55 PDT
I'm tempted to just leave it closed. It's worked ok this long. One question would be how often this case comes up and would we get a decent perf boost by creating the special wrappers and eliminating the double wrapping. Going to reopen until that question is answered.
Comment 15 Boris Zbarsky [:bz] 2006-05-27 09:12:59 PDT
There's also the side question of how much of our UI we'd break.  :(  I bet people are relying on getting the unwrapped JS objects... :(
Comment 16 neil@parkwaycc.co.uk 2006-05-27 13:49:31 PDT
(In reply to comment #15)
>I bet people are relying on getting the unwrapped JS objects... :(
Do you mean via .wrappedJSObject? If so, that's fairly popular; Chatzilla uses it, for instance, and so apparently do parts of toolkit and browser, and I even used it in suite to work around an XPConnect exception-handling bug.
If I do something like tree.view = { selection: {} }; then tree.view.selection is still a wrapped JS object, not the original JS object.
Comment 17 John Bandhauer 2006-05-27 15:01:30 PDT
I doubt anyone will ever feel the need to implement the new sort of wrapper that this bug proposes. It is a new level of complication. And, AFAIK, no one has shown that the performance of the current system is bad enough to justify the change. 

For the benefit of people who don't know the history... The original comment in this bug describes the situation *before* XPConnect did double wrapping. Double wrapping, along with the wrappedJSObject mechanism as a way to get at the underlying JSObject, has been in place for a long time. *That* is the status quo. If this new sort of wrapper were implemented it would be a completely transparent change to any code using such objects.

FWIW, I'm of the mind that bugs and feature requests that are likely to never be addressed ought to be resolved as LATER or WONTFIX rather than having them linger on bug lists forever :)
Comment 18 Mike Shaver (:shaver -- probably not reading bugmail closely) 2006-05-29 13:16:52 PDT
Fixing this wouldn't have to break .wrappedJSObject at all, though it might let code be poked in ways that it doesn't expect.

Some instrumentation here would, as dbradley says, be most fruitful, since the marshalling/demarshalling code in XPConnect is not especially cheap.
Comment 19 Boris Zbarsky [:bz] 2006-05-29 13:24:12 PDT
> Do you mean via .wrappedJSObject?

No, I mean as described at the end of comment 1.
Comment 20 Mike Shaver (:shaver -- probably not reading bugmail closely) 2006-05-29 13:42:55 PDT
Comment #1 was fixed by the checkin referenced in comment #8, no?  We should always be wrapping return values now, regardless of implementation language.  If not, we have a major (other) bug.
Comment 21 Boris Zbarsky [:bz] 2006-05-29 14:08:52 PDT
Ah, ok.  So what's the issue remaining, then?  I don't see a clear descrption of it in this bug....
Comment 22 Mike Shaver (:shaver -- probably not reading bugmail closely) 2006-05-29 14:15:14 PDT
The issue is that we currently create two wrappers for JS-calls-JS, and therefore go through the value marshalling and xptcall/stub code twice, at non-zero cost in time and malloc churn.

As the bug is now summarized (and was originally filed; see also comment 5), it's about optimizing those cases via a custom JS-to-JS wrapper to improve performance -- if we decide that it's worth it.
Comment 23 Myk Melez [:myk] [@mykmelez] 2007-11-11 03:43:42 PST
We're using many more JS XPCOM components these days.  I wonder if it's worth it now.

Note You need to log in before you can comment on or make changes to this bug.