Separate runtime's gcMallocBytes from compartment's gcMallocBytes

REOPENED
Unassigned

Status

()

Core
JavaScript Engine
REOPENED
5 years ago
3 years ago

People

(Reporter: billm, Unassigned)

Tracking

(Depends on: 1 bug)

unspecified
mozilla19
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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.
Attachment #647352 - Flags: review?(anygregor)
Comment on attachment 647352 [details] [diff] [review]
patch

I like it!
Attachment #647352 - Flags: review?(anygregor) → review+
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.
(Reporter)

Comment 4

5 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/c894763d1bbc
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
(Reporter)

Comment 6

5 years ago
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.
Depends on: 791674
No longer depends on: 791674

Updated

5 years ago
Depends on: 782072
Blocks: 806087

Comment 7

5 years ago
Backed out from Beta and Aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/8ff69d6c9151
https://hg.mozilla.org/releases/mozilla-beta/rev/2a61e12f904b
No longer blocks: 806087
Depends on: 806087
Target Milestone: mozilla17 → mozilla19
Backout cset:
https://hg.mozilla.org/mozilla-central/rev/cb997ba8433b
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Bill do you know why this happens? Do we have a code path where we don't update a malloc counter?
(Reporter)

Comment 10

5 years ago
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.
(Reporter)

Comment 15

5 years ago
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.
Attachment #677588 - Flags: review?(tschneidereit)
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.
Attachment #677588 - Flags: review?(tschneidereit) → review+
The new patch is a followup and not a replacement for the old patch right?
(Reporter)

Comment 18

5 years ago
(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.
(Reporter)

Comment 19

5 years ago
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

5 years ago
This seems to have regressed Kraken on AWFY by roughly 5%.
(Reporter)

Comment 21

5 years ago
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.
Whiteboard: [js:t]
(Reporter)

Comment 25

3 years ago
Unassigning myself from old bugs. Don't worry, not quitting.
Assignee: wmccloskey → nobody
You need to log in before you can comment on or make changes to this bug.