The default bug view has changed. See this FAQ.

super fast paths for Components.classes and Components.interfaces

RESOLVED WONTFIX

Status

()

Core
XPConnect
P1
normal
RESOLVED WONTFIX
8 years ago
6 years ago

People

(Reporter: vlad, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ts][Tsnap][2010Q1])

Attachments

(1 attachment, 4 obsolete attachments)

We should have support from xpconnect and JS for doing some kind of super fast path transformation of some constructs, for example:

On code that is marked as chrome (that is, has access to xpconnect):

Components.classes["foo"] -- turn into a single GetComponentByClasse(const char *foo) call
Components.interfaces.nsIFoo -- turn into basically a static value (or whatever we use to represent interfaces)
Components.classes["foo"].getService(Components.interfaces.nsIFoo) ->  single GetComponentByClass() call with the right QI on the back end

etc.

These are super common constructs, and we pay lots of xpconnect overhead (accessors off Components, custom indexers, etc.).

For historical reasons, whatever solution should support

var Cc = Components.classes;
var Ci = Components.interfaces;

with the same fast paths supported for Cc and Ci (or whatever variables are used).
(Reporter)

Updated

8 years ago
Whiteboard: [ts][Tsna]
(Reporter)

Updated

8 years ago
Whiteboard: [ts][Tsna] → [ts][Tsnap]
So basically, we want to quickstub Components?
OS: SunOS → All
(Reporter)

Comment 2

8 years ago
More than quickstubs; I think this would need something more specific than just our current quickstub infrastructure.
(Reporter)

Comment 3

8 years ago
That is -- quickstubs could fast path Components.classes["foo"] (can it? it could classes.getClass("foo") but can it do the property access?) but it currently can't transform Components.classes["foo"].getService(Components.interfaces.nsIFoo) into a single quickstub'd call, I don't think.  (or .createInstance, etc.)
Transforming expressions is not something a code-calling bridge can do. Nor can the JS compiler in general (we don't analyze names that carefully, and the global object is hard to impossible to analyze).

Why have we tolerated such long-winded boilerplate for so long?

Better to do the source-to-source change once and hg commit it. We can get static analysis help at scale, I bet.

/be
The source-to-source idea would be to make some built-ins, with sweet names and convenient APIs, and optimize those. We could even use the #builtins#... jazz that self-hosted JS for TraceMonkey is starting to use, if we could convince ourselves it was safe to expose to chrome.

/be
(Reporter)

Comment 6

8 years ago
Source-to-source could work, though any convenient name may well end up with a namespace collision.  It is horrible boilerplate though.  Should talk about it tomorrow/thursday... one option might be Components.getService("foo", Components.interfaces.nsIFoo), but I don't know how to get rid of Components.interfaces (other than putting nsI* as globals).

For the compiler idea -- can we just set some flag when we create the JS context, and have the compiler make certain assumptions?  (e.g. when it sees a "Components" name, just assume it will always be xpconnect, and act accordingly?)  That would make it hard to catch Cc and Ci, though, hmm.

Comment 7

8 years ago
Can we not make Ci, Cc, etc readonly native properties on the Chrome globalobject?

Existing sources could be modified to remove Ci, Cc, etc via jshydra
Assignee: nobody → rflint
Assignee: rflint → nobody
Is the problem here the xpconnect overhead of accessing these objects? A while back I looked at implementing these objects natively, instead of using xpconnect, and it doesn't look especially hard to do... would that solve the problem (would it trace more effectively than using xpconnect)?
That's basically what I was suggesting in comment 1... We do have to be a little careful because we expose Components.classes to content but actually allow access (we throw a security exception from XPCWrappedNative::CallMethod).
is the work in bug 516085 going to be usable for this bug as well?

blake, are there bugs for the issues that block resolving this issue? is there someone on the js team who can work on it?

taras says general win from this -> p1.
Priority: -- → P1

Updated

8 years ago
Depends on: 516085

Comment 11

8 years ago
Seeing that nobody other than Blake knows what is going on I am going to experiment for now.

It looks like it should be possible to define Components.*, Components.classes and Components.interfaces as a pure js. I'm pretty sure nobody uses Components.QueryInterface.

Updated

8 years ago
Assignee: nobody → tglek

Comment 12

8 years ago
Created attachment 412702 [details] [diff] [review]
pure-js Components

Here is my first stab. This replaces the Components object with a js one + C++ lookup. This is still missing isSuccessCode() a couple of other pointless utility functions.

next step will be to make .interfaces and .classes native and then i can look at making .nsIFoo/etc xpcom component accessors cheap.

I wonder if we can share a single Components* object across chrome. It may be useful for caching services/etc on js side, but I am not there yet.

Comment 13

8 years ago
Created attachment 412731 [details] [diff] [review]
Components.interfaces & Components.classes are fast now

It turned out to be easy to finish up the rest of major stuff. Posting it here while I do some measurements on n810.
Attachment #412702 - Attachment is obsolete: true

Comment 14

8 years ago
This seems to save about 50ms on fennec startup. Doing something clever to avoid overhead on Components.interfaces/classes.* should bring that up to at least 100ms.

Comment 15

8 years ago
So more data.

With this patch, Components.utils/classes. now amount for 45ms of startup. vs somewhere > 100ms before.

Components.interfaces.* cost 231ms, and components.classes.* cost another 54ms.

Total is 382ms for normal fennec startup. This is for all members of Components. and Components.interfaces/utils.

On top of this there would be cost of Components.Constructor, etc.


This is all a pretty heavy penalty to pay for something that should be free.

Comment 16

8 years ago
OK, I give up. Optimizing the existing approach is a dead-end.

We could redefine what Components.(interfaces/classes).* mean. Returning an xpconnected object only to have it passed in as an argument into getService or something is insanity in terms of performance.

I think getting rid of pointless xpconnect should save us around 10% in warm startup speed on desktop(50ms) or 500ms on mobile.

In order to continue this I'll need someone who is experienced with the Components* API so we can draft up a new/equivalent api.
Assignee: tglek → nobody
(Reporter)

Comment 17

8 years ago
mrbkap, can you help here?

Comment 18

7 years ago
Talked with mrbkap about this. The plan of attack is
a) Convert Components* to pure JSNative objects with some stuff reimplented as native callbacks(no xpconnect).
b) CID and IID objects should become pure js objects of some sort.
c) xpconnect can be taught to convert those into what CID/IID-expecting functions expect.

I think once that is done, the next thing to do will be to move on to switching heavily used xpconnect calls to jsctype ones.
(Reporter)

Comment 19

7 years ago
(In reply to comment #18)
> Talked with mrbkap about this. The plan of attack is
> a) Convert Components* to pure JSNative objects with some stuff reimplented as
> native callbacks(no xpconnect).
> b) CID and IID objects should become pure js objects of some sort.
> c) xpconnect can be taught to convert those into what CID/IID-expecting
> functions expect.

Sounds good.  Quickstubs should be taught how to convert those as well.

> I think once that is done, the next thing to do will be to move on to switching
> heavily used xpconnect calls to jsctype ones.

Hm, heavily used xpconnect calls meaning calls that go across xpconnect, or calls into xpconnect itself?  If the former, isn't this just quickstubs?  If the latter, I'm not sure how jsctypes helps (as opposed to just setting up a js native function).

Comment 20

7 years ago
Created attachment 418912 [details] [diff] [review]
mostly native Components.interfaces

this patch 90% works, brings up a browser. Main thing that still files is that js code that does
 if (fo instanceof Ci.someinterface)
always returns false.

I couldnt get rid of nsJSIID sillyness because xpconnect parameter conversion stuff is too voodoo for me. But i did make it so nsJSIIDs are created lazily so declaring stuff like

myconstant = Ci.nsIFoo is basically free until someone passes myconstant into xpconnect.

Comment 21

7 years ago
I combined the two patches and sent them off to the try server. Results are discouraging. Ts is 1-5% faster, but that doesn't seem to be worth the hassle of deCOMtaminating Components.*

The only remotely sensible thing to do would be to do what the name of the bug suggests and add fast Cc.* and Ci.* globals to chrome scripts. I'm not sure if that's worth complicating our code for.
(Reporter)

Comment 22

7 years ago
I think it's worth it -- every percentage point helps, and this is code that's used all across the browser, not just startup.  A measurable 1-5% faster Ts win is pretty awesome, especially since it's on top of a lot of the "easy win" Ts work that has already been done!
Whiteboard: [ts][Tsnap] → [ts][Tsnap][2010Q1]
Blocks: 447581
(In reply to comment #21)
> I combined the two patches and sent them off to the try server. Results are
> discouraging. Ts is 1-5% faster, but that doesn't seem to be worth the hassle
> of deCOMtaminating Components.*
> 
> The only remotely sensible thing to do would be to do what the name of the bug
> suggests and add fast Cc.* and Ci.* globals to chrome scripts. I'm not sure if
> that's worth complicating our code for.

Taras, really?  1-5% faster seems worth it to me.  Can we drive this home?

Comment 24

7 years ago
Created attachment 431221 [details] [diff] [review]
combined patch
Attachment #412731 - Attachment is obsolete: true
Attachment #418912 - Attachment is obsolete: true

Comment 25

7 years ago
Created attachment 431409 [details] [diff] [review]
combined patch

This passes xpcshell tests here. Going to wait on try-server and places branch to confirm wins before embarking on finalizing these changes.
Attachment #431221 - Attachment is obsolete: true
result summary from landing on the Places branch. win is xp. didn't get multiple test runs, so this is comparing a single datapoint for each test. (though, for things like Ts, it's median of 10 runs, for example).

http://spreadsheets.google.com/ccc?key=0AjIcGNW1bFV5dGNLaVhtYlpKeVR5NXMyUGVoS2p2SFE&hl=en

Linux Ts log: http://tinderbox.mozilla.org/showlog.cgi?log=Places/1269987934.1269991379.19265.gz&fulltext=1
Mac Ts log: http://tinderbox.mozilla.org/showlog.cgi?log=Places/1269995479.1269999868.9655.gz&fulltext=1
WinXP Ts log: http://tinderbox.mozilla.org/showlog.cgi?log=Places/1269992772.1269995464.30821.gz&fulltext=1
It looks like this sort of went to sleep - taras - can we make it go?

Comment 28

7 years ago
(In reply to comment #27)
> It looks like this sort of went to sleep - taras - can we make it go?

I don't think so. This is a pretty delicate retrofit with hard-to-measure gains in overall startup. I think a more effective use of time would be to move on switching chome over to xpconnect-less api variety ala bug 563742.

Comment 29

6 years ago
See comment 28
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.