669346 - Don't force a second GC on memory pressure

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Justin Lebar (not reading bugmail)]

mobile/chrome/content/browser.js forces a GC [1] when it detects memory pressure.  But nsJSEnvironment.cpp already does this [2].

Should I modify [3] so it still forces a GC?  Or should I leave it to unload all tabs and just change the dump?  (You can now force a gc through about:memory.)

[1] http://hg.mozilla.org/mozilla-central/file/4a74ff2faa69/mobile/chrome/content/browser.js#l2531

[2] http://hg.mozilla.org/mozilla-central/file/4a74ff2faa69/dom/base/nsJSEnvironment.cpp#l203

[3] http://hg.mozilla.org/mozilla-central/file/4a74ff2faa69/mobile/chrome/content/browser.js#l100

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #0)
> mobile/chrome/content/browser.js forces a GC [1] when it detects memory
> pressure.  But nsJSEnvironment.cpp already does this [2].
> 
> Should I modify [3] so it still forces a GC?  Or should I leave it to unload
> all tabs and just change the dump?  (You can now force a gc through
> about:memory.)
> 
> [1]
> http://hg.mozilla.org/mozilla-central/file/4a74ff2faa69/mobile/chrome/
> content/browser.js#l2531

This will destroy browser elements before doing the GC. We want to make sure we destroy as much as possible before GC'ing.

> [2]
> http://hg.mozilla.org/mozilla-central/file/4a74ff2faa69/dom/base/
> nsJSEnvironment.cpp#l203

If we only allow this GC, the destroyed <browser>s may not be reclaimed ASAP, right?

> [3]
> http://hg.mozilla.org/mozilla-central/file/4a74ff2faa69/mobile/chrome/
> content/browser.js#l100

Depends on the answer above

Comment 2

[Justin Lebar (not reading bugmail)]

> If we only allow this GC, the destroyed <browser>s may not be reclaimed ASAP, 
> right?

Ah, I see.  That's correct.

It's still kind of silly to gc twice.  I wonder what's the right fix here...

Comment 3

[Justin Lebar (not reading bugmail)]

I wonder: Do we even know that those browsers are immediately collected when we do the GC?  In bug 664642, I found that I needed to spin the event loop before I GC'ed in order for it to have an effect [1].  (But I was in content, and I was collecting an ArrayBuffer...)

I'm not sure what is the right way to communicate to nsJSEnvironment that we don't want it to gc on memory pressure.  I could use a pref...

[1] https://bug664642.bugzilla.mozilla.org/attachment.cgi?id=540532

Comment 4

[Justin Lebar (not reading bugmail)]

> I'm not sure what is the right way to communicate to nsJSEnvironment that we 
> don't want it to gc on memory pressure.  I could use a pref...

bz, do you have any suggestions here?

Comment 5

[Boris Zbarsky [:bzbarsky]]

A pref (with a cache to avoid getting prefs on memory pressure) sounds just fine to me.

Comment 6

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 544535 [details] [diff] [review]
Patch v1

The mobile changes look good to me. The end result will do what we want it to do.

You'll need someone else to review the nsJSEnvironment part though. Maybe jst ?

r+ for mobile parts

Comment 8

[Justin Lebar (not reading bugmail)]

Comment on attachment 544535 [details] [diff] [review]
Patch v1

bz already has his head in this bug -- do you mind, Boris?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 544535 [details] [diff] [review]
Patch v1

This looks fine except for the pref name.  It's not really a javascript.options, pref, it's a DOM pref, no?  I guess it doesn't matter that much...

Maybe change it to javascript.options.automatically_gc_on_memory_pressure?

Comment 10

[Justin Lebar (not reading bugmail)]

> Maybe change it to javascript.options.automatically_gc_on_memory_pressure?

How about

 javascript.options.gc_on_memory_pressure

?

Comment 11

[Boris Zbarsky [:bzbarsky]]

That seems fine.

Comment 12

[Justin Lebar (not reading bugmail)]

inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/78509271aa68

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/78509271aa68