I've noticed that it's not uncommon for people to get full (i.e., not compartmental) TOO_MUCH_MALLOC GCs during normal browsing. These GCs are non-incremental, so it's annoying. I suspect that part of the reason this happens is that, if you have a lot of tabs open, each tab might decrement rt->gcMallocBytes by a small amount, and this alone is enough to drive it to zero. This patch solves the problem by decrementing rt->gcMallocBytes only if the allocation happened outside of a compartment. Otherwise, the allocation is only counted in comp->gcMallocBytes. This is more conservative than what bug 723350 does, because if there are a lot of compartmentless malloc allocations, we will still GC eventually.

Comment on attachment 647352 [details] [diff] [review] patch I like it!

Can we track that this patch is a real fix for people that see such GCs?

Comment on attachment 647352 [details] [diff] [review] patch Review of attachment 647352 [details] [diff] [review]: ----------------------------------------------------------------- This makes a lot of sense, yes. ::: js/src/jscompartment.cpp @@ +69,5 @@ > scriptCountsMap(NULL), > sourceMapMap(NULL), > debugScriptMap(NULL) > { > + setGCMaxMallocBytes(rt->gcMaxMallocBytes); This change might cause a small regression on areweslimyet.com, as it might slightly delay GCs, right? I do think that it's the right move, though, and that any regression should be dealt with by decreasing gcMaxMallocBytes itself.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c894763d1bbc I'll check areweslimyet to see if anything changes. I have an extension to monitor and record people's GCs. Once this lands and people update, I'll see if we have as many non-compartmental TOO_MUCH_MALLOC GCs. If not, then the patch worked.

I just checked the GC monitoring data and this seems to have worked well. For a 7/25 build I see a ton of TOO_MUCH_MALLOC GCs. For an 8/14 build, I haven't seen any so far. I also don't see any problems on areweslimyet, so I don't think this regressed anything.

Backed out from Beta and Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/8ff69d6c9151 https://hg.mozilla.org/releases/mozilla-beta/rev/2a61e12f904b

Backout cset: https://hg.mozilla.org/mozilla-central/rev/cb997ba8433b

Bill do you know why this happens? Do we have a code path where we don't update a malloc counter?

No, the problem is that we're doing a lot of compartment GCs and we never do a full GC. I still don't know why we need a full GC in this case. However, I do know that sync does a lot of cross-compartment stuff, so it's probably related to that. I want to investigate more, but I also wanted to make sure this got fixed for FF17.

(In reply to Bill McCloskey (:billm) from comment #10) > No, the problem is that we're doing a lot of compartment GCs and we never do > a full GC. I still don't know why we need a full GC in this case. However, I > do know that sync does a lot of cross-compartment stuff, so it's probably > related to that. I want to investigate more, but I also wanted to make sure > this got fixed for FF17. As I understand it, the vast majority of Sync's references are cross-compartment: in the processing of incoming items, for example, strings will be fetched in resource.js, turned into records by record.js, and be handled within engines.js and each individual engine (e.g., history.js), with logging handled by log4moz. Calls will weave back and forth between these modules. That is: as far as I can tell, the GC assumption that each compartment is a large-grained, self-contained module or page, with infrequent and low-volume communication between compartments, does not hold true for modern modular JavaScript applications. Assuming that my understanding of all this is correct (that is, compartment ~= file for chrome code), is there a way we can retain modularity whilst reducing the number of compartments? Services code alone is currently spread across 51 files, and that trend will only continue. If my understanding isn't correct, could you enlighten me?

This sounds a lot like a resurgence of bug 756549. @billm & jonco: we talked about that at the work week. That problem *should* have been fixed by the patch in bug 758278, so I'm not sure why it's coming up again now.

(In reply to Richard Newman [:rnewman] from comment #11) > is there a way we can retain modularity whilst reducing the number of compartments? I believe bug 759585 is going to address this, by allocating/collecting in terms of zones, which may contain multiple compartments.

(In reply to Jon Coppeard (:jonco) from comment #13) > (In reply to Richard Newman [:rnewman] from comment #11) > > is there a way we can retain modularity whilst reducing the number of compartments? > > I believe bug 759585 is going to address this, by allocating/collecting in > terms of zones, which may contain multiple compartments. See also bug 807205 where I am requesting the ability to import JS modules into the same compartment.

I looked into this some more. The problem is that when we do a GC, we clear the gcMallocBytes for every compartment, rather than just the ones being collected. Till discovered this problem a while ago and fixed it, but his fix got backed out along with some other changes related to gcMallocBytes.

Comment on attachment 677588 [details] [diff] [review] fix for resets Review of attachment 677588 [details] [diff] [review]: ----------------------------------------------------------------- Oh, right. I should have extracted that into its own patch back then.

The new patch is a followup and not a replacement for the old patch right?

(In reply to Gregor Wagner [:gwagner] from comment #17) > The new patch is a followup and not a replacement for the old patch right? Right, it's a followup.

Relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/63defe9bc7d5 I also decided to change the malloc quota for an individual compartment to be half that of the runtime. This seems to help in practice with sync.

This seems to have regressed Kraken on AWFY by roughly 5%.

OK, backed out again. I'll have to investigate Kraken. It looks like a number of things regressed on it. https://hg.mozilla.org/integration/mozilla-inbound/rev/f0eed1436c8b

(In reply to Bill McCloskey (:billm) from comment #19) > > I also decided to change the malloc quota for an individual compartment to > be half that of the runtime. This seems to help in practice with sync. I remember playing around with this number but malloc based benchmarks didn't like it at all.

Backout: https://hg.mozilla.org/mozilla-central/rev/63defe9bc7d5

(In reply to Gregor Wagner [:gwagner] from comment #22) > (In reply to Bill McCloskey (:billm) from comment #19) > > > > I also decided to change the malloc quota for an individual compartment to > > be half that of the runtime. This seems to help in practice with sync. > > I remember playing around with this number but malloc based benchmarks > didn't like it at all. And so do I: working on bug 679026, I saw that V8 was very sensitive to changes to malloc quotas. The number of GCs run during the various benchmarks varied quite a bit depending on the exact values.

Unassigning myself from old bugs. Don't worry, not quitting.