Note: There are a few cases of duplicates in user autocompletion which are being worked on.

TM: New Scope patch changes GC behavior in browser

RESOLVED FIXED in mozilla2.0b5

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: gwagner, Assigned: gwagner)

Tracking

unspecified
mozilla2.0b5
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(4 attachments, 4 obsolete attachments)

(Assignee)

Description

7 years ago
Created attachment 470518 [details]
GC Times

I looked at the testcase from bug 590379. The memory usage for the browser increases up to 4GB instead of 1GB in a build from 2 days ago.
The attached file shows that we keep allocation without performing GC.
(Assignee)

Comment 1

7 years ago
Created attachment 470520 [details]
stats
Attachment #470518 - Attachment is obsolete: true
The patch for bug 592001 restores some memory pressure updating. Can you give it a try and see whether it helps? Thanks,

/be
Assignee: general → brendan
Blocks: 558451
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla2.0b5
(Assignee)

Comment 3

7 years ago
Created attachment 470531 [details]
stats with patch from bug 592001

I tried with the patch from bug 592001 but I still see the same behavior.
(Assignee)

Comment 4

7 years ago
Reducing JSGC_MAX_MALLOC_BYTES to 32MB again also doesn't work.
(Tested with patch from 592001 applied)
Gregor, wow, how about to less than 16MB, say 8? Binary search for value with same effects as before?

/be

Comment 6

7 years ago
In the browser JSGC_MAX_BYTES is 2^32. We should try to reduce this to some sane value as now, without malloced scopes, we have much less malloc pressure.
Gregor has kindly agreed to take this. It's clear we were relying on malloc throughput to page GC, which happened to work (better, or more often) when we had scopes bloating up the malloc heap for objects with own properties. Now that we do not, measuing malloc bytes is not going to save us from GC-heap exhaustion.

/be
Assignee: brendan → anygregor

Comment 8

7 years ago
A possible quick fix could be make cx->realloc to update the malloc pressure counter on each realloc. Then at least slot arrays would contribute the pressure counter.
(In reply to comment #7)
> Gregor has kindly agreed to take this. It's clear we were relying on malloc
> throughput to page GC,

I meant "pace GC", but "page GC" as in "paging Dr. GC", works too.

Really do want more precise memory pressure accounting, or perhaps instead of us crudding up our code and data structures trying to keep track, plan B: ask the OS and avoid running up against its walls.

/be

Comment 10

7 years ago
Changing realloc is problematic. It caused our string behavior to be wonky before.

Comment 11

7 years ago
There is a patch for #9 floating around.
(Assignee)

Comment 12

7 years ago
Created attachment 470663 [details] [diff] [review]
patch

This is a simple approach where I count all arenas that are alive after a GC and when the allocation of new arenas reaches a certain threshold (relative to the live arenas after the last GC), a new GC is triggered. This heuristic should only apply if we have a high memory throughput.
The current patch only triggers GCs once we have allocated more than 40 Chunks.
We have about 250 Arenas per Chunk and for a browser startup we allocate about 2000 arenas. 

The remaining question is the tuning. 

The activity monitor tells me that "real memory usage" is down to about 750 MB again.
(Assignee)

Comment 13

7 years ago
Created attachment 470673 [details] [diff] [review]
patch

changed to use gcBytes
Attachment #470663 - Attachment is obsolete: true
Attachment #470673 - Flags: review?(gal)

Comment 14

7 years ago
What's the 40? And is 1.7 really necessary? 2 won't work?
(Assignee)

Comment 15

7 years ago
Created attachment 470681 [details] [diff] [review]
patch

The problem is that this approach shouldn't trigger the gc for a small working set. High throughput benchmarks show clearly that they need a few seconds until they get a stable working set at the beginning if the trigger value is too small. 40 means that we don't trigger a GC if we have less than 40 chunks allocated. 
I found that small changes influence the behavior of the whole browser so we have to find the right setup.
Attachment #470673 - Attachment is obsolete: true
Attachment #470681 - Flags: review?(gal)
Attachment #470673 - Flags: review?(gal)
(Assignee)

Comment 16

7 years ago
Created attachment 470695 [details]
stats

Some statistics for this patch:
We have about 500 Chunks allocated instead of 260 from before the patch. 
The "real memory" usage (as the activity monitor on mac shows) is not increased.
The GC pause is a little bit longer because of the bigger heap.
The good thing is that the pause time does not increase linear with the heap size and is only slightly longer. 
The remaining GC calls (once we have a stable working set) are coming from the browser side with the GC invocation callback.

Comment 17

7 years ago
You can't just sparkle the code with 3 instances of the magic value "25". At least put in some constant and re-use that. Its still not clear to me why we need to check against 2 trigger values, maxbytes and triggerbytes. Shouldn't one threshold be enough?
(Assignee)

Comment 18

7 years ago
As mentioned in comment 6 JSGC_MAX_BYTES is 2^32. I don't want to change this value since this may be set via the API and embedders might use it for a hard upper limit. The triggerbytes should prevent a heap growth to the limit without a GC.
The "magic" 25 comes from the "old" GC heuristics where we triggered a GC for every 50 Chunks we allocated in a high throughput environment. 
If I use a growth factor of 2 instead of 1.5 we end up with 800 Chunks instead of 500 and the pause time gets worse. 
So the whole GC heuristic is pretty unstable right now and it needs more than one afternoon to stabilize it again. This patch prevents the heap explosion but it is definitely not the best solution.

Comment 19

7 years ago
At least add a couple clean constants to the code and then lets land this. We should pick up the memory pressure patch after this one. The current code is total clownshoes. Lets do it after your stuff lands though.
(Assignee)

Comment 20

7 years ago
Created attachment 470722 [details] [diff] [review]
patch

Lets hope that not too many testcases and benchmarks relied on the old GC heuristics...
Attachment #470681 - Attachment is obsolete: true
Attachment #470722 - Flags: review?(gal)
Attachment #470681 - Flags: review?(gal)

Updated

7 years ago
Attachment #470722 - Flags: review?(gal) → review+
(Assignee)

Comment 21

7 years ago
http://hg.mozilla.org/tracemonkey/rev/007b92be0804
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/007b92be0804
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.