Closed Bug 835886 Opened 11 years ago Closed 11 years ago

Share chrome compartments in Firefox

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: gps, Unassigned)

References

Details

(Whiteboard: [MemShrink]) 

+++ This bug was initially created as a clone of Bug #798491 +++

A few people have told me things along the lines of "I'm not aware of a good reason why chrome JS in Firefox does not share the same compartment [like on B2G]."

In this bug, I'm proposing that we have all chrome JS in Firefox share the same compartment (like things were before CPG landed).

Try at https://tbpl.mozilla.org/?tree=Try&rev=6b7c5e3ae543.

From my local OS X build of m-c, explicit memory usage went from 82MB to 65MB (started a near-empty profile, let run for a minute, hit minimize usage button a few times). 17MB savings on a base profile - yes please!

I have no clue who has to make the call on this.

billm is also working on alleviating compartment overhead. My vote (not that it matters) is to move ahead on this bug (if possible) for immediate savings then re-evaluate the state of the world after billm's work is complete.
(In reply to Gregory Szorc [:gps] from comment #0)
> A few people have told me things along the lines of "I'm not aware of a good
> reason why chrome JS in Firefox does not share the same compartment [like on
> B2G]."

We currently have no way to distinguish between "our" JS and addon JS, so flipping the pref right now breaks all the addons.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #1)
> (In reply to Gregory Szorc [:gps] from comment #0)
> > A few people have told me things along the lines of "I'm not aware of a good
> > reason why chrome JS in Firefox does not share the same compartment [like on
> > B2G]."
> 
> We currently have no way to distinguish between "our" JS and addon JS, so
> flipping the pref right now breaks all the addons.

Can you elaborate? It worked before, so what would be required to make it work again?
There is some discussion in bug 807104 as well.
(In reply to Gregory Szorc [:gps] from comment #0)
> billm is also working on alleviating compartment overhead. My vote (not that
> it matters) is to move ahead on this bug (if possible) for immediate savings
> then re-evaluate the state of the world after billm's work is complete.

That work is on track to be done on the soon side, actually. The first application is going to be to make system JSMs share a compartment, which should be very easy once the infrastructure is complete. It may well be done before the branch. I'm a bit nervous shipping a giant change like this for just one release before changing our strategy yet again.

(In reply to Gregory Szorc [:gps] from comment #2)
> > We currently have no way to distinguish between "our" JS and addon JS, so
> > flipping the pref right now breaks all the addons.
> 
> Can you elaborate? It worked before, so what would be required to make it
> work again?

Worked before in what sense? It worked on B2G because B2G doesn't have addons.
(In reply to Bobby Holley (:bholley) from comment #4)
That work is on track to be done on the soon side, actually. The first
> application is going to be to make system JSMs share a compartment

Sorry. That should read "make system JSMs share an arena".
(In reply to Bobby Holley (:bholley) from comment #4)
> (In reply to Gregory Szorc [:gps] from comment #0)
> > billm is also working on alleviating compartment overhead. My vote (not that
> > it matters) is to move ahead on this bug (if possible) for immediate savings
> > then re-evaluate the state of the world after billm's work is complete.
> 
> That work is on track to be done on the soon side, actually. The first
> application is going to be to make system JSMs share a compartment, which
> should be very easy once the infrastructure is complete. It may well be done
> before the branch. I'm a bit nervous shipping a giant change like this for
> just one release before changing our strategy yet again.

We're planning on making them share a compartment but not a global?  I thought we were just going to stick them inside a "zone".
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3)
> There is some discussion in bug 807104 as well.

I wasn't aware of bug 807104. We should consider expanding scope of bug 807104 to cover Firefox and duping.

I have added this issue to today's Platform Team agenda.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #6)

> We're planning on making them share a compartment but not a global?  I
> thought we were just going to stick them inside a "zone".

See comment 5. ;-)
The "merge all system compartments in B2G" was always intended to be a short-term hack until zones were implemented.  It was crucial on B2G because, unlike desktop Firefox, it is multi-process and is operating under very tight memory constraints.

And there's a major disadvantage to merging all the system compartments that hasn't been mentioned:  we lose the fine-grained memory reporting.

So I suggest we WONTFIX this, and I say that as someone who is extremely aware of the current compartment overhead.
(In reply to Nicholas Nethercote [:njn] from comment #9)
> The "merge all system compartments in B2G" was always intended to be a
> short-term hack until zones were implemented.  It was crucial on B2G
> because, unlike desktop Firefox, it is multi-process and is operating under
> very tight memory constraints.

Great to know!

> And there's a major disadvantage to merging all the system compartments that
> hasn't been mentioned:  we lose the fine-grained memory reporting.

Is loss of per-JSM memory usage more important than a 20% baseline memory reduction? What about on Fennec? Put another way, what serves our users more?

> So I suggest we WONTFIX this, and I say that as someone who is extremely
> aware of the current compartment overhead.

Go for it. I'm not going to reopen out of spite. I just wanted to spur a conversation that lead to an agreed plan. If people are confident with the schedule, by all means WONTFIX or dupe on the zones bug.
Per-compartment memory reporting has surely won us more than 20% by providing us with ways to diagnose and fix various kinds of bad memory usage.

But I'm not as quick as njn to dismiss this bug; zones have been coming RSN for a while, and while I'm (very) happy to see movement in the relevant bugs, I'm not holding my breath.  If we had a way to get memory reporters from JSMs which all share one compartment, that would be a win for B2G and would let us turn this on for Firefox.

But I don't know how feasible that idea is.
That's a 20% reduction with no tabs open, right?  I'd guess a lot of that 17mb is base overhead that won't increase much with more tabs, so the % will decrease in a more realistic browsing scenario.  Of course, 17mb still isn't something to sneeze at. ;)
We might also be able to recover some fine-grained memory reporting information, as I'm assuming things are smooshed into a single compartment in a mechanical way we could probably derive information from.
Enough of the hacks, work-arounds and partial fixes.  Zones is the right way to address this.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
Does the same apply to Fennec (bug 807104)?
(In reply to Nicholas Nethercote [:njn] from comment #14)
> Enough of the hacks, work-arounds and partial fixes.  Zones is the right way
> to address this.

Why is this mutually exclusive with zones? We can win a little until the 'better' fix.
> Why is this mutually exclusive with zones? We can win a little until the
> 'better' fix.

billm is working on zones *right now*.  Nobody is working on this bug;  nobody even has a solution for the problem khuey raised in comment 1, AFAIK.
(In reply to Nicholas Nethercote [:njn] from comment #17)
> > Why is this mutually exclusive with zones? We can win a little until the
> > 'better' fix.
> 
> billm is working on zones *right now*.  Nobody is working on this bug; 
> nobody even has a solution for the problem khuey raised in comment 1, AFAIK.

https://bugzilla.mozilla.org/show_bug.cgi?id=807104#c10 is a simple one
I don't want someone to waste time on a short-term hacky fix.  I also don't want a short-term hacky fix to become a long-term hacky fix because it happened to reduce the need for the proper fix.
I think we'd all feel a little better if we understood the expected timeframe for the zones work.  ATM I don't know if it's a project of magnitude more comparable to GGC or about:compartments.
(In reply to Taras Glek (:taras) from comment #16)
> Why is this mutually exclusive with zones? We can win a little until the
> 'better' fix.

For one thing, it means shipping a major behavioral change in a one-off fashion, which is generally a risky thing to do.

(In reply to Justin Lebar [:jlebar] from comment #20)
> I think we'd all feel a little better if we understood the expected
> timeframe for the zones work.

When I talked to billm a few days ago, he guestimated that I'd be able to start doing the XPConnect bits in the next couple of weeks.
You need to log in before you can comment on or make changes to this bug.