735099 - Re-enable incremental GC on desktop platforms

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Bug 734946 will disable incremental GC. This bug is to re-enable it and to fix the leaks that seem to show up when it's enabled during browser-chrome tests.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Here is a recent try push for re-enabling. It still leaks.
https://tbpl.mozilla.org/?tree=Try&rev=59b6f9fff21d

It's possible that this is related to bug 723832, which shows a leak of a similar size happening several times per day. However, with incremental GC enabled we leak much more frequently.

Comment 2

[Andrew McCreight [:mccr8]]

The patch in bug 727965 makes incremental GC leak on Linux (32 or 64 or both?) Moth, or it did a few months ago.  If it happens to be the same underlying issue, it might be easier to track down on Linux instead of XP.

Comment 3

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

I think I have reduced the leak to LayoutModuleDtor not being called. This try run should confirm it:

https://tbpl.mozilla.org/?tree=Try&rev=9c8bdd5e3ab8

Comment 4

[Luke Wagner [:luke]]

Any verdict?

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Bug 750424 fixes the leak. I want to take a look at some memory usage regressions that showed up the last time incremental was enabled. Otherwise, this is ready to go.

Comment 6

[David Mandelin [:dmandelin]]

+ for Kilimanjaro. IGC is one of the key things we need for smooth game perf.

Comment 7

[Cameron Kaiser [:spectre]]

Why just Intel?

Comment 8

[Andrew McCreight [:mccr8]]

See bug 750959 for Android.  It had a different problem blocking it from being turned on, or something like that.

Comment 9

[Andrew McCreight [:mccr8]]

Has a different problem, rather than had.

Comment 10

[Danial Horton]

unfortunately Bill, i just logged bug 752191 which may delay incremental gc again :(

and it was going oh so well till now....

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a9c5ac02776

Let's see if this sticks. There are a bunch of tuning fixes I want to get in, but I'd like to land this now so that other patches don't regress it.

Comment 13

[Matt Brubeck (:mbrubeck)]

\o/ https://hg.mozilla.org/mozilla-central/rev/1a9c5ac02776

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac968ff4fe41
https://hg.mozilla.org/mozilla-central/rev/ac968ff4fe41

I had to back this out for crashes. I think I know the cause, though.

Comment 15

[Matt Brubeck (:mbrubeck)]

By the way, here's another intermittent crash that hadn't been filed yet:

https://tbpl.mozilla.org/php/getParsedLog.php?id=11717280&tree=Mozilla-Inbound
Rev4 MacOSX Snow Leopard 10.6 mozilla-inbound debug test mochitests-5/5 on 2012-05-13 08:43:26 PDT for push 5fbb80b8e84f
TEST-UNEXPECTED-FAIL | /tests/toolkit/components/passwordmgr/test/test_prompt_async.html | Exited with code 1 during test run

 0  XUL!js::gc::GetGCThingTraceKind [Heap.h : 497 + 0x1c]
    rbx = 0x0756d000   r12 = 0x00000000   r13 = 0x04a94230   r14 = 0x00000000
    r15 = 0xffffffff   rip = 0x035512ce   rsp = 0x5fbfc9d0   rbp = 0x5fbfc9e0
    Found by: given as instruction pointer in context
 1  XUL!js::gc::MarkKind [Marking.cpp : 230 + 0x4]
    rbx = 0x5fbfca28   r12 = 0x00000000   r13 = 0x04a94230   r14 = 0x00000000
    r15 = 0xffffffff   rip = 0x0354bcd7   rsp = 0x5fbfc9f0   rbp = 0x5fbfca10
    Found by: call frame info
 2  XUL!js::gc::MarkValueInternal [Marking.cpp : 329 + 0xb]
    rbx = 0x0756d080   r12 = 0x092e57e8   r13 = 0x04a94230   r14 = 0x00000000
    r15 = 0xffffffff   rip = 0x0354c054   rsp = 0x5fbfca20   rbp = 0x5fbfca60
    Found by: call frame info
 3  XUL!proxy_TraceObject [jsproxy.cpp : 1259 + 0x19]
    rbx = 0x092e57c0   r12 = 0x04a94230   r13 = 0x042ad140   r14 = 0x04a94000
    r15 = 0x008f3600   rip = 0x03389177   rsp = 0x5fbfca70   rbp = 0x5fbfca80
    Found by: call frame info
 4  XUL!js::GCMarker::processMarkStackTop [Marking.cpp : 1092 + 0x9]
    rbx = 0x1ca93fb0   r12 = 0x042b18a0   r13 = 0x042ad140   r14 = 0x04a94000
    r15 = 0x008f3600   rip = 0x035582b4   rsp = 0x5fbfca90   rbp = 0x5fbfcb20
    Found by: call frame info
 5  XUL!js::GCMarker::drainMarkStack [Marking.cpp : 1136 + 0xa]
    rbx = 0x04a94230   r12 = 0x5fbfcbb0   r13 = 0x04a94268   r14 = 0x04a94000
    r15 = 0x008f3600   rip = 0x0354f78b   rsp = 0x5fbfcb30   rbp = 0x5fbfcb50
    Found by: call frame info
 6  XUL!GCCycle [jsgc.cpp : 3471 + 0x19]
    rbx = 0x008f3740   r12 = 0x008f3740   r13 = 0x00000002   r14 = 0x46876000
    r15 = 0x008f3600   rip = 0x032a00f2   rsp = 0x5fbfcb60   rbp = 0x5fbfcc40
    Found by: call frame info
 7  XUL!Collect [jsgc.cpp : 3719 + 0x11]
    rbx = 0x04a94320   r12 = 0x04a94000   r13 = 0x04a94b60   r14 = 0x00000001
    r15 = 0x5fbfcc70   rip = 0x032a0a91   rsp = 0x5fbfcc50   rbp = 0x5fbfccb0
    Found by: call frame info
 8  XUL!nsJSContext::GarbageCollectNow [nsJSEnvironment.cpp : 2994 + 0xe]
    rbx = 0x00000000   r12 = 0x00000000   r13 = 0x08298000   r14 = 0x0085d600
    r15 = 0x00000018   rip = 0x01cab24b   rsp = 0x5fbfccc0   rbp = 0x5fbfccf0
    Found by: call frame info
 9  XUL!nsTimerImpl::Fire [nsTimerImpl.cpp : 508 + 0xa]

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a6be10cfee3f

To make this work I had to disable bug 716014. Hopefully we can fix that for the next cycle.

Comment 17

[Andrew McCreight [:mccr8]]

What's with the extra GCs and CCs in the explicit call to GarbageCollect()?  Is that needed to make some test pass?

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a6be10cfee3f

Comment 19

[Alex Keybl [:akeybl]]

No need to track - please keep the status flags in bug 641025 up to date.

Comment 20

[Dave Camp (:dcamp)]

I got a few upset mails from Talos when I merged this changeset (among others) to Fx-Team:

https://groups.google.com/d/topic/mozilla.dev.tree-management/BnQX5sR1M2Q/discussion

Looking at the graph server, this regression seems to have hit inbound around when this changeset landed and followed it to mozilla-central and eventually fx-team.  I'm not sure why I don't see a tree-management mail before the fx-team merge.

There are a few other changesets in range, but this one seems like the prime suspect.  Is this known?

Comment 21

[Matt Brubeck (:mbrubeck)]

(In reply to Dave Camp (:dcamp) from comment #20)
> Looking at the graph server, this regression seems to have hit inbound
> around when this changeset landed and followed it to mozilla-central and
> eventually fx-team.  I'm not sure why I don't see a tree-management mail
> before the fx-team merge.

There were actually many regression mails when this first hit inbound and then central, although they were spread across several threads and some were mis-blamed.  For example:

https://groups.google.com/d/topic/mozilla.dev.tree-management/oQ6HWhKoyQc/discussion

Comment 22

[Danial Horton]

Comment 12 applies.

Comment 23

[Gregor Wagner [:gwagner]]

Why do we have to trigger multiple GCs now?

  for (int i = 0; i < 3; i++) {
    nsJSContext::GarbageCollectNow(js::gcreason::DOM_UTILS, nsGCNormal, true);
    nsJSContext::CycleCollectNow(aListener, aExtraForgetSkippableCalls);
  }

Comment 24

[Danial Horton]

Ccing Olli Pettay to get his input on the above question.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

I don't know why it was added.
It is not in the patch what was reviewed.

Comment 26

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Gregor Wagner [:gwagner] from comment #23)
> Why do we have to trigger multiple GCs now?
> 
>   for (int i = 0; i < 3; i++) {
>     nsJSContext::GarbageCollectNow(js::gcreason::DOM_UTILS, nsGCNormal,
> true);
>     nsJSContext::CycleCollectNow(aListener, aExtraForgetSkippableCalls);
>   }

I was trying to resolve some orange that showed up on tryserver recently.

Comment 27

[Gregor Wagner [:gwagner]]

I also see regressions when running any benchmark or animation with more than one tab open. It seems we don't trigger compartment GCs any more.
My small netbook doesn't like that at all.

Comment 28

[Danial Horton]

i just updated to http://hg.mozilla.org/mozilla-central/rev/dd6ec482a85d and am no longer getting any iGC at all, the value is true and about:support indicates it is enabled.

Comment 29

[Danial Horton]

20120602134306
http://hg.mozilla.org/mozilla-central/rev/9274e6b53af4 is the most recent build i can get iGC to work

Comment 30

[Andrew McCreight [:mccr8]]

I just updated, and on my usual browsing set I'm getting all GCs as "Reason: CC_WAITING, Nonincremental Reason: request".

Comment 31

[Andrew McCreight [:mccr8]]

This was disabled again in 15.

Comment 32

[Martin Best (:mbest)]

How is this looking for mozilla16?

Comment 33

[David Mandelin [:dmandelin]]

(In reply to Martin Best (:mbest) from comment #32)
> How is this looking for mozilla16?

It's on and looking good.