Last Comment Bug 778993 - Separate runtime's gcMallocBytes from compartment's gcMallocBytes
: Separate runtime's gcMallocBytes from compartment's gcMallocBytes
Status: REOPENED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal with 2 votes (vote)
: mozilla19
Assigned To: Nobody; OK to take it and work on it
: general
Mentors:
Depends on: 782072 806087
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-30 16:37 PDT by Bill McCloskey (:billm)
Modified: 2014-01-15 14:11 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (1.97 KB, patch)
2012-07-30 16:37 PDT, Bill McCloskey (:billm)
anygregor: review+
Details | Diff | Splinter Review
fix for resets (1.83 KB, patch)
2012-11-01 15:34 PDT, Bill McCloskey (:billm)
till: review+
Details | Diff | Splinter Review

Description Bill McCloskey (:billm) 2012-07-30 16:37:57 PDT
Created attachment 647352 [details] [diff] [review]
patch

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 1 Gregor Wagner [:gwagner] 2012-07-30 16:57:05 PDT
Comment on attachment 647352 [details] [diff] [review]
patch

I like it!
Comment 2 Gregor Wagner [:gwagner] 2012-07-30 17:28:20 PDT
Can we track that this patch is a real fix for people that see such GCs?
Comment 3 Till Schneidereit [:till] (ECOOP July 18 - July 22, pto July 23 - July 31) 2012-07-31 00:23:26 PDT
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.
Comment 4 Bill McCloskey (:billm) 2012-07-31 18:39:21 PDT
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.
Comment 5 Ed Morley [:emorley] 2012-08-01 02:52:01 PDT
https://hg.mozilla.org/mozilla-central/rev/c894763d1bbc
Comment 6 Bill McCloskey (:billm) 2012-08-15 16:59:00 PDT
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.
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-10-30 08:05:48 PDT
Backout cset:
https://hg.mozilla.org/mozilla-central/rev/cb997ba8433b
Comment 9 Gregor Wagner [:gwagner] 2012-10-30 11:41:29 PDT
Bill do you know why this happens? Do we have a code path where we don't update a malloc counter?
Comment 10 Bill McCloskey (:billm) 2012-10-30 11:47:41 PDT
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.
Comment 11 Richard Newman [:rnewman] 2012-10-30 13:24:29 PDT
(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?
Comment 12 Till Schneidereit [:till] (ECOOP July 18 - July 22, pto July 23 - July 31) 2012-10-31 07:50:22 PDT
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.
Comment 13 Jon Coppeard (:jonco) 2012-10-31 08:46:09 PDT
(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.
Comment 14 Gregory Szorc [:gps] 2012-10-31 09:56:20 PDT
(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.
Comment 15 Bill McCloskey (:billm) 2012-11-01 15:34:22 PDT
Created attachment 677588 [details] [diff] [review]
fix for resets

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 16 Till Schneidereit [:till] (ECOOP July 18 - July 22, pto July 23 - July 31) 2012-11-02 00:40:29 PDT
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.
Comment 17 Gregor Wagner [:gwagner] 2012-11-02 09:30:49 PDT
The new patch is a followup and not a replacement for the old patch right?
Comment 18 Bill McCloskey (:billm) 2012-11-02 10:01:45 PDT
(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.
Comment 19 Bill McCloskey (:billm) 2012-11-02 15:09:08 PDT
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.
Comment 20 Paul Wright 2012-11-02 17:12:47 PDT
This seems to have regressed Kraken on AWFY by roughly 5%.
Comment 21 Bill McCloskey (:billm) 2012-11-02 18:23:23 PDT
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
Comment 22 Gregor Wagner [:gwagner] 2012-11-02 23:33:35 PDT
(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.
Comment 23 Ryan VanderMeulen [:RyanVM] 2012-11-03 06:25:20 PDT
Backout:
https://hg.mozilla.org/mozilla-central/rev/63defe9bc7d5
Comment 24 Till Schneidereit [:till] (ECOOP July 18 - July 22, pto July 23 - July 31) 2012-11-03 06:55:34 PDT
(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.
Comment 25 Bill McCloskey (:billm) 2014-01-15 14:11:50 PST
Unassigning myself from old bugs. Don't worry, not quitting.

Note You need to log in before you can comment on or make changes to this bug.