Closed Bug 25180 Opened 20 years ago Closed 3 years ago

(maybe) optimize xpconnect wrapping for JS -> JS communication


(Core :: XPConnect, defect, P3)






(Reporter: jband_mozilla, Unassigned)



(Keywords: perf)


(1 file)

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.
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.

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:
In today's build you won't see that.
*** Bug 28120 has been marked as a duplicate of this bug. ***
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.
nominating for nsbeta3 because JS components are happen'n and we don't want to 
break them later when we add this insulation layer.
Whiteboard: nsbeta3
John: nsbeta3 goes in the keyword field, not status whiteboard. 
Keywords: nsbeta3
Whiteboard: nsbeta3 → [nsbeta3+]
Interim patch checked in. The long term fix still needs to be done. 

Clearing "nsbeta2" decorations.
Keywords: nsbeta3
Whiteboard: [nsbeta3+]
mass reassign of xpconnect bugs to
Assignee: jband → dbradley
Target Milestone: --- → Future
Removing Rob as QA contact
QA Contact: rginda → pschwartau
This shouldn't be an in issue anymore...
Closed: 14 years ago
Resolution: --- → WORKSFORME
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.
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)
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.
Resolution: WORKSFORME → ---
Keywords: perf
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... :(
(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.
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 :)
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.
Summary: xpconnect needs wrappers for JS -> JS communication → (maybe) optimize xpconnect wrapping for JS -> JS communication
> Do you mean via .wrappedJSObject?

No, I mean as described at the end of comment 1.
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.
Ah, ok.  So what's the issue remaining, then?  I don't see a clear descrption of it in this bug....
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.
QA Contact: pschwartau → xpconnect
Assignee: dbradley → nobody
We're using many more JS XPCOM components these days.  I wonder if it's worth it now.
We can file new bugs if there's too much overhead from wrappers in chrome JS.
Closed: 14 years ago3 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.