997906 - Hoist all System XPCWrappedNatives into a single compartment

Status: NEW

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

In bug 989373, Nick and I have looking at the crazy-large memory regression (on the order of 20 MiB) from turning off the nasty b2g-only compartment-sharing machinery.

Much of the overhead comes from thousands of function objects, which was initially perplexing. I dug into it, and found some very interesting answers.

The gist of the problem is that, for XPCWrappedNative reflections of XPCOM objects without a PreCreate hook, we create a separate XPCWrappedNative instance in each compartment. The logic here is that the compartment has no logical "home", and so the best we can do is to just recreate it in the scope of each caller.

This certainly has a fair amount of overhead, but it gets worse. For any object that doesn't implement nsIClassInfo, we end up with the XPCWN_NoHelper JSClass, which lazily resolves every native getters/setter/method as a function on the object itself. This means that invoking Ci.nsIFoo.equals(x) and Ci.nsIBar.equals(x) resolves and allocates two entirely separate function objects, one for nsIFoo and one for nsIBar. Yikes.

We can solve this problem to some degree by giving more things ClassInfo. But I think we should also consider doing something bigger. XPCWNs are basically only used by chrome these days, and we could have a singleton scope to house all of the WNs. This would replace the heavy-weight XPCWN copies with much cheaper cross-compartment wrappers, and let us share all of the functions across the entire Runtime.

Something like this has the potential to save tens of megabytes on Desktop, and possibly a megabyte or two on b2g, since we still have quite a few different system compartments there.

The tricky part is that there are some native functions that depend on the compartment of the caller, like Cu.import and loadSubscript. [implicit_jscontext] might be a good indicator of these situations. I need to think about it more.

Comment 1

[Nicholas Nethercote [inactive]]

Looking at the data in bug 989373 comment 8, I think the potential desktop win might be "up to 10 MiB" rather than "10s of MiBs", though I'd be happy to be proven wrong :) But still definitely worth doing!

I'm happy to help with any parts that don't require deep xpconnect knowledge, e.g. maybe with giving more things ClassInfo. (Having said that, I also just started a four day Easter break, so I won't be able to do anything until next Tuesday.)

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Great find, Bobby!

Comment 3

[Benjamin Smedberg]

FWIW, a while back I did want to require that every XPCOM object exposed into JS have classinfo. That's a big project and so the other things listed here might be easier.

Comment 4

[Nicholas Nethercote [inactive]]

bholley gave classinfo to nsJSID in bug 1001094.

Comment 5

[Bobby Holley (:bholley)]

I have prototype patches here that launch a functioning browser. I'm going to fix them up into a real patch series.

Comment 6

[Nicholas Nethercote [inactive]]

bholley, you said this bug has stalled -- is it just because you've been busy with other things, or are there difficulties with the code?

Comment 7

[Bobby Holley (:bholley)]

(In reply to Nicholas Nethercote [:njn] from comment #6)
> bholley, you said this bug has stalled -- is it just because you've been
> busy with other things, or are there difficulties with the code?

Both. I got a working browser running with these patches, but memory actually regressed by tens of megabytes. This didn't make sense, and we definitely need to investigate it, because it may indicate this issue was a red herring, and that we still need to dig deeper to figure out why loader global sharing saves more memory than we think it should. But I've had to pause that work to keep on top of reviews and finish my Q2 goals.

I still plan to get back to this in a month or so.

Comment 8

[Bobby Holley (:bholley)]

Attachment: patches.zip

Here's a big stack of very old and bitrotted patches that I wrote while working on this.

Comment 9

[Kris Maglione [:kmag]]

Now that we have the shared JSM global, we should probably just use that as the default global for any WN without a PreCreate hook.

We might need to add more PreCreate hooks for things that are used mainly from windows rather than JSMs to avoid memory regressions from the added CCW overhead, though...

Comment 10

[Eric Rahm [:erahm]]

Kris, do you have an idea of what kind of overhead this has now that JSM's are sharing a global?

Comment 11

[Kris Maglione [:kmag]]

(In reply to Eric Rahm [:erahm] from comment #10)

Kris, do you have an idea of what kind of overhead this has now that JSM's
are sharing a global?

Now that all system principal windows are also in the same compartment, probably not a lot, if any.