938945 - Investigate if we should gc/cc more during testing

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

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

Attachment: more_cc_gc.diff

gc/cc scheduling is not optimized for mochitest -like testsuites.
In general we try to postpone especially cc if possible in order to not
affect to responsiveness. Running tests is very different case.

So *maybe* we could explicitly run gc/cc a bit more often.

https://tbpl.mozilla.org/?tree=Try&rev=26e62fdcec83

Comment 1

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

With the patch we have a lot less DOMWindow objects alive at the end of M2, and I don't 
see Bug 935419 on try.

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 832680 [details] [diff] [review]
more_cc_gc.diff

Review of attachment 832680 [details] [diff] [review]:
-----------------------------------------------------------------

I think this looks reasonable, and we should land it to hopefully help reopen the trees.

::: testing/mochitest/tests/SimpleTest/iframe-between-tests.html
@@ +6,5 @@
>    load the next.
>  -->
>  <script>
>  window.addEventListener("load", function() {
> +  var runner = (parent.TestRunner || parent.wrappedJSObject.TestRunner);

Nit: no need for the parens

Comment 3

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

Landed https://hg.mozilla.org/integration/mozilla-inbound/rev/c3d8ec0e53c7


Also, shutdown cc is something which is run only in debug builds, and during it we don't
do various optimizations, so the graph can be a lot larger than usually.
Users don't ever get shutdown CCs.

Comment 4

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

[13:28]	edmorley	smaug: ok, let's land and retrigger a load, but not reopen
[13:28]	smaug	right

Comment 5

[Ed Morley [:emorley]]

Do we know what affect this has on mochitest runtimes?

Comment 6

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

I couldn't see difference when comparing try runs to m-i runs, assuming the information in tooltips is reliable.
gc/cc don't after all run too often.

Comment 7

[Andrew McCreight [:mccr8]]

Will this change the behavior of browser-chrome, too, or does some other file have to be modified for that?

Comment 8

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

This was mochitest only. 

Ted, could we put the gc/cc triggering to some generic place where it would be called in
mochi, browser-chrome and chrome tests

Comment 9

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

This is going to make it so we run a lot fewer incremental GCs during testing. That's bad.

Comment 10

[Andrew McCreight [:mccr8]]

Maybe we could have some kind of special powers function that starts an incremental GC if we're not already in the middle of one?

A simpler fix that doesn't regress our IGC testing might be to force a few GC/CC cycles in the test harness right before we shutdown.

Comment 11

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

(In reply to Andrew McCreight [:mccr8] from comment #10)
> Maybe we could have some kind of special powers function that starts an
> incremental GC if we're not already in the middle of one?

I guess that could work.

> A simpler fix that doesn't regress our IGC testing might be to force a few
> GC/CC cycles in the test harness right before we shutdown.

I don't think that would really help much.

Comment 12

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

Attachment: only gc/cc right before shutdown

Something like this then? Backs out the previous patch.

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

Comment 13

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

(In reply to Bill McCloskey (:billm) from comment #11)
> > A simpler fix that doesn't regress our IGC testing might be to force a few
> > GC/CC cycles in the test harness right before we shutdown.
> 
> I don't think that would really help much.
Why wouldn't it?
As far as I see the m2 issue is that shutdown CC ends up dealing with a too big graph.
We have shutdown CC only on debug builds, and while we're running it, we can't do any CC optimizations
(so the graph ends up being so huge).

But let's see what tryserver tells us.

Comment 14

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

(In reply to Olli Pettay [:smaug] from comment #13)
> (In reply to Bill McCloskey (:billm) from comment #11)
> > > A simpler fix that doesn't regress our IGC testing might be to force a few
> > > GC/CC cycles in the test harness right before we shutdown.
> > 
> > I don't think that would really help much.
> Why wouldn't it?
> As far as I see the m2 issue is that shutdown CC ends up dealing with a too
> big graph.

I misunderstood what Andrew was saying. I thought he was proposing we do a few incremental GCs during shutdown to get minimal test coverage for it. I guess you're only adding new GCs at the end, which seems fine.

Comment 15

[(not currently active) Ted Mielczarek]

(In reply to Olli Pettay [:smaug] from comment #8)
> This was mochitest only. 
> 
> Ted, could we put the gc/cc triggering to some generic place where it would
> be called in
> mochi, browser-chrome and chrome tests

No, mochitest and browser-chrome don't really share any JS code unfortunately.

Comment 16

[(not currently active) Ted Mielczarek]

Comment on attachment 832972 [details] [diff] [review]
only gc/cc right before shutdown

Review of attachment 832972 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mochitest/browser-test.js
@@ +194,5 @@
> +      let utils = window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                        .getInterface(Ci.nsIDOMWindowUtils);
> +      utils.garbageCollect();
> +      utils.garbageCollect();
> +      utils.garbageCollect();

Is there a reason we have to do this three times? Would be nice if we could just say utils.reallyGarbageCollect() or something.

Comment 17

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

One collect may not collect everything.
utils.garbageCollect() calls gc then cc. CC's unlink may lead to new garbage to be collectable.

Comment 18

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

Uh, Win7 debug Moth doesn't like the second patch.

Comment 19

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

Attachment: v3

Looks like TestRunner has a bug where it calls onComplete twice.
https://tbpl.mozilla.org/?tree=Try&rev=f35bcce4e333

Comment 20

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

https://tbpl.mozilla.org/?tree=Try&rev=2ff02d9b49a7

Comment 21

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

Comment on attachment 833179 [details] [diff] [review]
v3

ted, is removing
-        if (TestRunner.repeat == 0 && TestRunner.onComplete) {
-             TestRunner.onComplete();
-         }
ok to you.
We call onComplete later in the same function.

Try is still running...

Comment 22

[(not currently active) Ted Mielczarek]

Comment on attachment 833179 [details] [diff] [review]
v3

Review of attachment 833179 [details] [diff] [review]:
-----------------------------------------------------------------

I'm a little confused about how this repeat logic works, but this *looks* sensible.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

[Ed Morley [:emorley]]

(Followups haven't landed)

Comment 25

[Andrew McCreight [:mccr8]]

Olli, is your followup ready to land?  I'm a little nervous about not running IGC as much in Mochitests.

Comment 26

[Andrew McCreight [:mccr8]]

This does seem to have fixed bug 935419, though!

Comment 27

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

It is not ready to land. It seems that we should run CC reasonable often anyway, not only
right before shutdown.

Comment 28

[Andrew McCreight [:mccr8]]

Since we've branched, should this bug be closed and followup work moved to say bug 939606?

Comment 29

[Martijn Wargers (dead)]

There are tests on b2g, like bug 775227, that oom. So on b2g doing more gc/cc (I think even after every test) would be better there. Some of the tests might not oom then.

Comment 30

[Andrew McCreight [:mccr8]]

A patch landed quite a while ago, so I'm going to mark this as fixed.  Followup work can go in a new bug.