44352 - graceful handling of *really* low memory conditions

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

It would be a really fine thing if applications using nsIMemory were 
encouraged (required?) to register at least one nsIMemoryPressureObserver that 
would do the following:

* sleep for a bit to let all other nsIMPOs complete their flushMemory() 
  callbacks.

* hope that the just-completed flushes have left enough memory to pop up a 
  dialog (or equivalent) saying "Mozilla is running out of memory.    Please
  consider closing any unused windows applications in order to free up some
  memory."

Comment 1

[Warren Harris]

Need nsIMemoryPressureObservers for:

- memory cache
- style cache
- (what else?)

The reserve memory thing may already be handled by the lower-level code on Mac. 
Don't know about the other platforms though. This is going to be really hard to 
test (or useless, depending on how you look at it) until we can get our memory 
requirements down to a reasonable level to begin with. Pavlov tried to set a 64M 
threshold, but we're way over that at this point.

We probably should file individual bugs on for individual 
nsIMemoryPressureObservers though.

Comment 2

[Brendan Eich [:brendan]]

Separate bugs might be better, or not (depending on who's gonna do the work).  
For now, here's my recent guilty secret:

layout/xbl/src/nsXBLService.{h,cpp} maintain an LRU cache of nsXBLJSClasses, at 
most 64.  That quota could be raised (maybe it should be -- I need to talk with 
hyatt).  Right now, it's low enough that only at most 64(quota)*21(words)*4 or 
5376 bytes can be tied up in the cache.  But I think the right thing in general 
is for nsXBLService to implement nsIMemoryPressureObserver.  I'll do this unless 
you want dibs, warren.

/be

Comment 3

[Brendan Eich [:brendan]]

Warren, do you want this bug too?  It really should not be assigned to nobody.

/be

Comment 4

[Brendan Eich [:brendan]]

Adding more memory/cache buddies.

/be

Comment 5

[Alec Flett]

I do a similar hack for string bundle caching, with a fixed size of 10 (total 
size can vary greatly depending on the size of the string bundles in use)

Comment 6

[Warren Harris]

I take it back.

Other caches to think about:
- xul cache (brutal sharing)
- string bundle cache
- registry?
- xpt
- open jar file cache
- rdf sort cache
- mail header cache
- mork?

Comment 7

[Brendan Eich [:brendan]]

Attachment: proposed memory pressure observer impl for XBL service

Comment 8

[Brendan Eich [:brendan]]

Attachment: better patch (it decrements gClassLRUListLength in FlushMemory)

Comment 9

[Brendan Eich [:brendan]]

I thought I would hack up an impl to see how it felt, what code bloat it added. 
It just subroutined a bit of nsXBLService's destructor, and added a vtbl pointer 
and the usual MI impl overhead.  Hyatt, could you review my last patch and give 
it an r=?  I'll check it in when it's blessed by you or warren (or anyone on the 
cc: list, really -- I'm easy!).

/be

Comment 10

[Warren Harris]

Bug 41398 addresses the flusher for the memory cache.

Comment 11

[Brendan Eich [:brendan]]

I checked in the nsXBLService patch, so you can strike XBL from the list.

/be

Comment 12

[David Hyatt]

The chrome registry holds on to overlay RDF data sources that it could 
conceivably flush.

Comment 13

[rusty.lynch]

cc'ing myself

Comment 14

[Warren Harris]

One discovery on my part, and a good suggestion by Wade:

First, I have the memory flushers hooked up on my system so that every 100,000 
allocations, they kick in. This uncovered that they have to be called on the 
right thread (duh). So then I was thinking that we should marshal a synchronous 
event over to the mozilla thread to to the flushing, etc... ugh.

Wade suggested that we should abandon this nsIMemoryPressureObserver stuff in 
favor of a simpler IsLowMemory predicate. This could be called at the point 
where you're in the cache code, about to insert a new entry. If you discover 
IsLowMemory is true, you can throw some things out. 

This makes the algorithm more distributed, and avoids the thread-switching 
problem, but it might not cover all low-memory cases we care about. (Note that 
our current memory pressure hooks in nsIMemory aren't called in all cases either 
though.)

Comment 15

[Warren Harris]

*** Bug 45944 has been marked as a duplicate of this bug. ***

Comment 16

[Simon Fraser [no longer active]]

I'm implementing low memory handling on Mac, for bug 20743, so I'm going to start 
calling nsMemory::HeapMinimize(). Currently, only the XBL service implements 
this.

Another thing that needs to implement it: the image cache. Filed bug 46337 on 
pnun.

Comment 17

[Warren Harris]

*** Bug 46337 has been marked as a duplicate of this bug. ***

Comment 18

[Warren Harris]

This should not be marked Future. It's definitely a beta3 thing.

Comment 19

[Judson Valeski]

need flusher for xul freelist (see 45353)

Comment 20

[Asa Dotzler [:asa]]

*** Bug 48551 has been marked as a duplicate of this bug. ***

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

So from reading the bug, it appears that the plan may be to get rid of the
MemoryPressure stuff.  Is this true?  This seems like a bad thing to me, because
IsLowMemory() by itself doesn't allow for existing stuff to be flushed.  e.g. if
I cache a connection, it may be reasonable to keep around at the time I cached
it, but 3 seconds later memory could fill up, and I'd be happy to throw it out.

Comment 22

[Chris Waterson]

Attachment: memory flusher thread, replace nsIMemoryPressureObserver with nsIObserver

Comment 23

[Chris Waterson]

See above patch:

1. Got rid of nsIMemoryPressureOBserver in favor of nsIObserver. Converted
current clients. Need review for string bundles, XBL service, and image loader.

2. Added memory flusher thread, predicated on implementation of IsLowMemory()
test. (In other words, it's #ifdef windows for now.)

3. Implemented pressure observer for XUL cache (hard whackage; replace
nsHashtable with PLDHashTable, mostly because nsHashtable is lame and won't let
you remove elements while enumerating). Need r=

Comment 24

[Simon Fraser [no longer active]]

r=sfraser on the nsImageManager changes

Comment 25

[Alec Flett]

you rock chris
r=alecf on the string bundle stuff

Comment 26

[Chris Waterson]

Talked to warren about the nsXULPrototypeCache changes: gonna whack nsHashtable
to not bastardize the enumerator's return value so it can handle
deletion-during-enumeration. Tomorrow...

Comment 27

[Chris Waterson]

Attachment: round 2, with feedback from sfraser and warren

Comment 28

[Chris Waterson]

Added "aImmediate" argument to HeapMinimize(); when set to true, we'll try to do
heap minimization immediately; otherwise, we'll defer it by  enqueuing an event.
sfraser: will this cover your case where Mac knows that it's low on memory and
wants people to start dumping stuff?

Also, fixed so that we *really* dump the image cache. Removed
nsXULPrototypeCache from this round: I'll get it in a second pass.

Comment 29

[Chris Waterson]

Checked in slightly modified round #2. Filing dependent bugs for remaining tasks...

Comment 30

[Warren Harris]

This is in now, right?

=> waterson

Comment 31

[Chris Waterson]

Yes, the dependencies are a big bogus, too. Marking fixed. We can evaluate
dependencies individually.

Comment 32

[Dan Mosedale (:dmosedale, :dmose)]

The memory pressure stuff rocks.  Reopening, though, since there is not yet a
dialog that gets popped up (see the initial comment in this bug).

Comment 33

[Chris Waterson]

to ben goodger, who can assess whether or not we want to have a dialog pop up as
one of the memory pressure observers.

Comment 34

[Ben Goodger (use ben at mozilla dot org for email)]

nsbeta3- to get off my radar. 

Comment 35

[Doron Rosenberg (IBM)]

*** Bug 60191 has been marked as a duplicate of this bug. ***

Comment 36

[Brian 'netdragon' Bober]

*** Bug 70824 has been marked as a duplicate of this bug. ***

Comment 37

[Brian 'netdragon' Bober]

See also: bug 70821. Limit memory usage per window
See also: bug 70824. Mozilla could give an option to save state and close 
mozilla (or else make it temporarily inactive) in a dialog box in order to be 
able to free up memory and recover later.

Comment 38

[Paul Chen]

nav triage team:

Not going to get done for beta, marking nsbeta1- and future. Reassigning to 
alecf.

Comment 39

[Wayne Mery (:wsmwk)]

This had at least one checked in patch (that was not done via blocking bug) and only issue, comment 32, was not addressed. So I suspect this should be notated as FIXED

(add bug 125400, because blocking bug 53313 was duped to it)