970645 - Fix Mochitest-BC leak finder to use a shrinking instead of non-shrinking gc

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

Bug 929374 wants to enable TI and Ion for chrome. This is causing nsGlobalWindow leaks in Linux Opt builds on Try.

The problem is that ShouldPreserveJITCode returns false if TI is disabled but with TI enabled it's possible we keep JIT code around when we GC and this can cause leaks because ICs/code can embed object pointers etc.

This patch fixes browser-chrome to do a shrinking GC so that ShouldPreserveJITCode returns false again. This fixes all the leaks.

Andrew, no idea who should review this. I asked you because it's GC+CC related, it's a really simple patch.

Comment 1

[Andrew McCreight [:mccr8]]

Comment on attachment 8373712 [details] [diff] [review]
Patch

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

ttaubert wrote the original leak finder, but this is simple enough.  r=me assuming you check that this doesn't make BC times even more horribly bad than they already are.

::: testing/mochitest/browser-test.js
@@ +433,5 @@
>  
>          // Schedule GC and CC runs before finishing in order to detect
> +        // DOM windows leaked by our tests or the tested code. Note that we
> +        // use a shrinking GC so that the JS engine will discard JIT code and
> +        // caches more aggressively.

Maybe you could say "discard JIT code and JIT caches" here, just to be more explicit?  To a non-IonMonkey hacker like me, it reads like "discard JIT code... and also caches more aggressively!" which is not what you mean.

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Andrew McCreight [:mccr8] from comment #1)
> r=me assuming you check that this doesn't make BC times even more horribly bad
> than they already are.

The Try push below with this patch finished BC in 53 minutes. I compared it to some Linux64 Opt BC runs on inbound and I see times between 52-65 minutes there so I think we're fine. Also note that we call this only for the last test and (fortunately) not after every test.

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

> Maybe you could say "discard JIT code and JIT caches" here, just to be more
> explicit?  To a non-IonMonkey hacker like me, it reads like "discard JIT
> code... and also caches more aggressively!" which is not what you mean.

Good idea, will do. Thanks for the quick review!

Comment 3

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4689576e5568

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4689576e5568