98673 - Turbo mode memory usage

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect, P2)

Description

[Peter Trudelle]

Using QuickLaunch in Netscape 6.1 on Win98:

Launch netscape -mail -turbo
Close mail window
(Netscape disappears, but stays loaded)
Check memory usage (I used TaskInfo 2000)
Netscape is occupying 32 MEGABYTES of RAM, with no windows open, not even a
systray icon.
For comparison, IE uses 12 MB with no windows open.
Historical note: When this sort of thing happened in previous releases, we used
to call it a severe leak.

Comment 1

[Paul Chen]

Umm, not sure what I can do about it myself, but will look into it. Marking p2
and mozilla0.9.7

Comment 2

[Peter Trudelle]

->blaker/nsbeta1. Note this grows seemingly without bound.  I think we need to
put a cap on it, if at all possible.

Comment 3

[Blake Ross]

Task Manager now reports that we use 900K when first starting turbo with no
windows open, and about 5MB when closing the last window while in turbo.  But
win9x is not fixed yet; the fix checked in only works on Windows NT/2000/XP.

Comment 4

[Peter Trudelle]

nsbeta1+ per Nav triage team.  We need to set a cap on memory usage, as close to
IE as we can reasonably get.  We don't need to keep everything around, just
enough launch and display a window quickly.  Are there issues with keeping
things in VM, but letting them get paged out?  Might it not be better to just
flush them?

Comment 5

[Blake Ross]

dp, is your group working on any 9x solutions?

Comment 6

[Suresh Duddi (gone)]

Nope :-( We dont even know what can be done.

Comment 7

[Peter Trudelle]

we need to get a handle on this issue.  cc law, who else do we need to nail this?

Comment 8

[Blake Ross]

I tried law's suggestion, loading a more minimal nav window (at the expense of a
small perf hit), a few weeks ago. It did not seem to noticeably improve memory
usage.

Comment 9

[Jaime Rodriguez, Jr.]

cc'ing cathleen and dp.

Comment 10

[Cathleen]

Unfortunately, the fix that works on NT/2K/XP are system calls that don't exist
on 9x machines.  see bug 123729, bug 123728

Comment 11

[Peter Trudelle]

ADT2 per ADT triage team.

Comment 12

[Peter Trudelle]

To reiterate, the issue is not how much RAM is used initially, but whether it
continues to eat up the peak amount of RAM after all windows are closed.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Is bug 130157 relevant here?  That is, can someone try running through the
testcase there and then see whether the memory is deallocated when all windows
are closed in turbo mode?  (You'll need a machine with at least 512MB or ram or
a lot of patience as you wait on swap.)

Comment 14

[Blake Ross]

cathleen, does your group have any time to look at this (since they're looking 
at mem usage/footprint issues in general)?

Comment 15

[selmer (gone)]

Boris, bug 130157 claims to be Linux only.  I don't think it's relevant here
unless the same affect can be seen on win98.  

The original usage numbers are from quite a while ago.  In the meantime, mail
has killed a lot of leaks and bloat.  Any chance that the number is small enough
by now?  Do we even have any realistic alternatives?

Comment 16

[Blake Ross]

-> morse

Comment 17

[Trevor Andreas]

After quick launch was broken and fixed, bug 133120, I am not seeing Mozilla's
memory usage decrease upon closing the last window on Windows XP. With quick
launch enabled and no windows open, taskmanager was telling me it was using
about 40 megs of RAM.

Comment 18

[Suresh Duddi (gone)]

The memory shown by taskmanager not going down is because of fixing bug 125100

Comment 19

[Stephen P. Morse]

In order to get a handle on this bug, we need to be able to accurately measure 
our heap usage, and see whether individual tweeks can improve it.  To 
accomplish this I've put together a testbed based on dp's heapinfo tool

   http://alexquinn.org/mozilla/shared.js

The problem with the above tool is that it looks for a mozilla window (or a 
netscape window) to send the HeapDump message.  Unfortunately when we are closed 
down in turbo mode, there are no open mozilla windows.  So the tool won't work 
as is.

Therefore, instead of using the heapdump.exe to send the message, I am sending 
the message from a "session-logout" observer within the browser.  There is such 
an observer in the cookie module (extensions/cookie/nsCookieService.cpp), so I 
am using that for my test bed.  Below is an abridgement of the patch that I am 
using in that observer.

+    // initialization for accessing heapdump
+    HWND mozwindow = 0;
+    EnumWindows(EnumWindowsProc, (LPARAM) &mozwindow);
+    if (mozwindow == 0) {
+        printf("Cannot find Mozilla or Netscape 6 window. Exit\n");
+        exit(-1);
+    }
+    UINT msgHandle = RegisterWindowMessage("MOZ_HeapDump");
+
+    // dump the heap at this point to see its size before flushing
+    SendMessage(mozwindow, msgHandle, 0, 0);
+
+    // flush the XULPrototypeCache
+    static NS_DEFINE_CID(kXULPrototypeCacheCID, NS_XULPROTOTYPECACHE_CID);
+    nsCOMPtr<nsIXULPrototypeCache> cache =
+        do_GetService(kXULPrototypeCacheCID, &rv);
+
+    if (cache)
+        cache->Flush();
+
+    // dump heap again to see its size after flushing
+    SendMessage(mozwindow, msgHandle, 0, 0);

Note that the above patch calls for a heapdump, then flushes the 
XULPrototypeCache, and then calls for a second heapdump.  By comparing the 
results of the two heapdumps, we can see how much our heapusage will be improved 
by doing this flush.  Of course the same testbed can be used to test other 
candidates for memory improvement.  Below are the results of the comparison.

==== BEFORE ====

Heap statistics : whatever
------------------------------------------------------------
USED               : 10946.17 K in  99660 blocks
FREE               :   286.74 K in   4155 blocks
Uncommitted FREE   :  3840.00 K in     24 blocks
Overhead           :   854.84 K in 103825 blocks
Alignment overhead :    32.59 K in   4158 blocks
             Total : 15960.34 K
FREE at heap end   :  3340.00 K in      6 blocks - 80.94% of FREE

Total Commit    : 12096.00 K
Total Uncommit  :  3840.00 K
          Total : 15936.00 K

==== AFTER ====

Heap statistics : whatever
------------------------------------------------------------
USED               : 10537.47 K in  95187 blocks
FREE               :   694.45 K in   5193 blocks
Uncommitted FREE   :  3860.00 K in     28 blocks
Overhead           :   835.84 K in 100390 blocks
Alignment overhead :    40.73 K in   5197 blocks
             Total : 15968.48 K
FREE at heap end   :  3340.00 K in      6 blocks - 73.33% of FREE

Total Commit    : 12076.00 K
Total Uncommit  :  3860.00 K
          Total : 15936.00 K

Comment 20

[Stephen P. Morse]

Attachment: testbed for measuring improvements in heap usage

Comment 21

[Stephen P. Morse]

To use the above testbed, you do the following:

1. modify the code in nsCookieService.cpp to flush out the item you want to test

2. start turbo mode (mozilla -turbo)

3. start mozilla normally

4. go to a site that sets cookies (need this because cookie module won't be 
loaded otherwise)

5. close mozilla window

At this point the heap info has been collected.  Each call to dump the heap will 
put it into a different file -- the first being c:\heapinfo1.txt, then 
heapinfo2.txt, etc.  This way we can compare values and see differences.

To view the various heap dumps do:

   cd mozillla\tools\footprint
   perl heapmap.pl < c:\heapdump1.txt
   perl heapmap.pl < c:\heapdump2.txt
      etc

Comment 22

[Stephen P. Morse]

Attaching a patch that restores turbo to its initial state after each shutdown 
so memory usage while in the dormant turbo state is reduced to the bare minimum.

Comment 23

[Stephen P. Morse]

Attachment: Restore turbo to its initial state after each shutdown

Comment 24

[jag (Peter Annema)]

Comment on attachment 84218 [details] [diff] [review]
Restore turbo to its initial state after each shutdown

>Index: xpfe/appshell/src/nsWebShellWindow.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/appshell/src/nsWebShellWindow.cpp,v
>retrieving revision 1.375
>diff -u -r1.375 nsWebShellWindow.cpp
>--- xpfe/appshell/src/nsWebShellWindow.cpp	11 Apr 2002 03:49:30 -0000	1.375
>+++ xpfe/appshell/src/nsWebShellWindow.cpp	20 May 2002 03:21:40 -0000
>@@ -378,13 +378,32 @@
> }
> 
> 
>+#include "nsINativeAppSupport.h"
>+
> /*
>  * Close the window
>  */
> NS_METHOD
> nsWebShellWindow::Close()
> {
>-   return Destroy();
>+   // Need to notify nsNativeAppSupport that we are closing
>+   //   This allows nsNativeAppSupport to distinguish between a subsequent call to
>+   //   SetIsServerMode coming from window closings (normal part of browser
>+   //   shutdown) as opposed to coming from an external "mozilla -killAll" process
>+   // nsNativeAppSupport needs to know that so it can determine whether or not it
>+   //   has to relaunch turbo mode.
>+
>+   nsresult rv;
>+   nsCOMPtr<nsINativeAppSupport> native;
>+   nsCOMPtr<nsIAppShellService> appShell
>+     (do_GetService( "@mozilla.org/appshell/appShellService;1", &rv));
>+   if (NS_FAILED(rv)) return PR_FALSE;
>+   rv = appShell->GetNativeAppSupport( getter_AddRefs( native ));
>+   if (NS_FAILED(rv)) return PR_FALSE;

PR_FALSE? Shouldn't this be rv or NS_OK?

>+   native->StartIgnoringIsServerMode(); // notify that normal shutdown starting
>+   rv = Destroy();
>+   native->StopIgnoringIsServerMode(); // notify that normal shutdown is finished
>+   return rv;
> }
> 
> 
>Index: xpfe/bootstrap/nsAppRunner.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/bootstrap/nsAppRunner.cpp,v
>retrieving revision 1.352
>diff -u -r1.352 nsAppRunner.cpp
>--- xpfe/bootstrap/nsAppRunner.cpp	7 May 2002 23:06:54 -0000	1.352
>+++ xpfe/bootstrap/nsAppRunner.cpp	20 May 2002 03:21:56 -0000
>@@ -1744,6 +1751,26 @@
>   setupProfilingStuff();
> #endif
> 
>+  // add an extra char at start of arg[0] for communication with nsNativeAppSupport 
>+  char*  extendedProgramName;
>+  extendedProgramName = PL_strdup(" ");
>+  int length = PL_strlen(argv[0]);
>+  extendedProgramName = (char *) PR_Realloc(extendedProgramName, length+2);
>+  PL_strcpy(extendedProgramName+1, argv[0]);
>+  char* programName = argv[0];
>+  argv[0] = extendedProgramName+1;

Isn't it simpler (and cheaper) to just alloc for length+2, put a space on the
first char, then copy argv[0] after that?
You should probably make sure the Malloc (or Realloc) has succeeded.

It looks okay, but I'd like law's r=, if you'll take my sr=jag.

Comment 25

[Stephen P. Morse]

Attachment: address issues raised by jag in comment 24

Comment 26

[Bill Law]

Attachment: How I think this should be done

Steve's scheme seems way too "Rube Goldberg"-ish.

Here's a much simpler patch that I did a couple of weeks ago.  Doesn't this
accomplish the same thing?

Comment 27

[Stephen P. Morse]

Yes, Bill's patch is simpler.
With the printf's removed, r=morse

Comment 28

[Stephen P. Morse]

Attachment: new patch to satisfy some verbal comments received from jag.

Comment 29

[Bill Law]

Attachment: explanator text

Here is some explanatory text I wrote about this patch back when I first came
up with it.  I have to re-read it and see if there were any other remaining
open issues.  I think we may want to re-arrange the code slightly in order to
ensure that the "turbo dialog" (which says "Mozilla is still runnig and using
memory") still appears.

Comment 30

[Stephen P. Morse]

Attachment: rearranging code so that turbo dialog still appears

Comment 31

[Bill Law]

+    nsCOMPtr<nsIAppShellService> appShell =
+        do_GetService( "@mozilla.org/appshell/appShellService;1", &rv);
+    if ( NS_SUCCEEDED( rv ) ) {
+        // Instead of staying alive, launch a new instance of the application
and then
+        // terminate for real.  We take steps to ensure that the new instance
will run
+        // as a "server process" and not try to pawn off its request back on this
+        // instance.
+    
+        if ( !mShownTurboDialog ) {

I think the getting of the app shell service and rv check should come after
displaying the turbo dialog.  Note that displaying the turbo dialog doesn't need
the app shell service so there is no reason to make that code dependent on
having gotten it.  That also leaves the existing code as it is and consolidates
the new code into one place.

Comment 32

[Stephen P. Morse]

Attachment: bringing up turbo dialog before getting the appshell

Comment 33

[Bill Law]

Comment on attachment 84487 [details] [diff] [review]
bringing up turbo dialog before getting the appshell

r=law

Comment 34

[jag (Peter Annema)]

Comment on attachment 84487 [details] [diff] [review]
bringing up turbo dialog before getting the appshell

sr=jag

Comment 35

[Stephen P. Morse]

Fix checked in on trunk

Comment 36

[scottputterman]

jrgm, could you try this out on the trunk?

Comment 37

[John Morrison]

I've been trying this in my own trunk build on win2k. First, this is very 
slick and works well. Thanks! I'll be doing some more thorough testing
and will report back later.

One note: (discussed somewhat above but I don't grok what the real intent was).
The "turbo dialog" (when not preffed off) comes up every time the last window 
is closed. In previous builds, independent of the pref, it only showed the 
"first time" that the user close the last visible window.

Arguably, we could leave it this way and just let the user set the pref once 
they have had their file of warnings (like the POST warning dialog on form 
submission). I suppose restoring the previous behaviour might be difficult
since there would need to be some way to flag the next child process (or 
otherwise have the child infer) that this was not the "original" invocation 
of the application.

Another question: are we open to any problems resulting from a race condition
in closing/opening XUL.mfl, parent.lock, history.dat, etc. files or does the 
mutex (and/or parent.lock itself) ensure an orderly shutdown/startup.

Comment 38

[John Morrison]

I'll note that this mode of operation kills background "biff" for mail, which
previously had stayed alive in the system tray when all mozilla windows had
been closed.  But, I think that this is the appropriate behaviour (unless
mailnews wants to implement biff as a lightweight out-of-process agent).

All the menu options on the system tray turbo applet work (open their
respective windows, or disable turbo, or exit the application fully).

I've closed the last window as Navigator, Mailnews, Addressbook, Composer,
Prefs Panel, MsgCompose, Standalone message window, Help, Bookmarks, History,
and the app exits and respawns turbo correctly.

I've performed an exit with File->Quit and all the above windows open, and the
app exits and respawns turbo correctly.

I used a script to launch 100 mozilli in succession where the start page did
'window.onload=window.close', and other than being a sick, evil thing to do,
nothing "broke".

I've done several reboots and this is starting correctly at boot time.
I did a skin switch, which caused turbo to completely exit and when mozilla
was restarted, the newly chosen skin was in place.

I've done an install while quicklaunch is running (which caused it to exit 
gracefully).

I also did some timing measurements (times +/- 1 second). I used a modified 
form of morse's program to malloc a ton of memory, and then touch all the
pages in that space (modified to malloc two times my physical RAM, touch
all the pages and then sleep for 15 seconds). This forces almost all other
VM pages to be posted to disk, so they must be paged back in when they are
next referenced.

1) normal "cold" boot of 05/21 mozilla trunk (i.e., not using 
   the morse mem flusher)                                                7 sec

2) run mem flusher, then do same "cold" boot of 05/21 mozilla trunk     13 sec

3) 05/21 trunk, WITHOUT patch, in turbo mode; run mem flusher, then     16 sec
   time to get first browser window from double-click on turbo icon

4) 05/21 trunk, WITH patch, in turbo mode; run mem flusher, then        21 sec
   time to get first browser window from double-click on turbo icon

5) 05/21 trunk, WITHOUT patch, in turbo mode; open many dialogs,        58 sec
   use mailnews and composer, many browser windows, etc. to make 
   mozilla sassy and fat; then run mem flusher, then time to get 
   first browser window from double-click on turbo icon

Comment 39

[Bill Law]

Question about 3) and 4).  Both these involve double-clicking on the QuickLaunch
systray icon while no windows were currently open?

I can't come up with any reason why it would be slower with this patch, since
the patch only affects what happens when you *close* windows.

Perhaps the difference is due to the fact that you had previously done some
additional stuff in case 3)?  E.g., you opened a browser window, viewed some
pages, then closed the browser window.  *That* could speed up a subsequent
request to open a new browser window due to caching of objects/services and
caching in a more literal sense (xul and/or web content).

An apples-to-apples comparison might be useful (start both versions from scratch
with -turbo option, then measure time to first window when you double-click the
systray icon).  But maybe you tested that way already.

Comment 40

[Stephen P. Morse]

I had the same concerns that Bill had.  It doesn't seem possible for this patch 
to have changed the timings between 3 and 4.

Comment 41

[Peter Trudelle]

Double-click on systray icon is not worth worrying about.  If it causes any
problems at all, just remove it.

Comment 42

[John Morrison]

(Hmm, I don't quite understand your last comment, Peter).

Anyways, I had thought that the difference in those times was due to, as you 
suggest, the fact that in (3) I had previously brought up a browser window, 
which then would have populated the xul cache and loaded some additional DLL's
that are only loaded when the first window is created. But I missed a more 
obvious reason: "I'm with stupid.".

I went back to reproduce the 21/16sec times and could not. I'm not sure what
I did, but it seems now that I just mixed something up. Both builds/modes take
about 16 seconds to bring up a browser window from the backgrounded process
after having run that memory flusher.

However, the time in (5) still stands (in fact it can be worse or better, but 
~60 seconds is the ballpark). And that's the situation that we really want to 
avoid.

I think this should be landed for machv.

Comment 43

[Stephen P. Morse]

Double clicking on systray item (or the equivalent, which is right-clicking on 
it and selecting browser) is the way to bring the application back to life when 
it is turbo sleeping.  I don't think we want to remove that.

Item 5 is certainly much too high.  But that's a separate issue -- see bug 
76831.

Comment 44

[Peter Trudelle]

Double-clicking on the systray icon, or the equivalent, is *one* way to launch
the app from turbo mode, and a new one at that. We added it as low-hanging
fruit- nice to have, but not critical functionality.  We could easily choose to
resolve any signficant problems with that gesture by simply removing it.

Comment 45

[John Morrison]

re: comment #43 and bug 76831: The point of measurement (5) though, is that 
by now relaunching on the close of the last window, we avoid the scenario 
of a user who has been running with quicklaunch enabled and has not rebooted 
in a long time. We are less likely to get a huge bloated image swapped out. 
It can still happen by other means but perhaps not as often or as severe.

re: comment #44: I don't see anything going wrong with double-clicking the 
systray icon. Is there a problem?

Comment 46

[Stephen P. Morse]

Yes, you are correct that launching the browser directly from the system tray 
icon is not essential.  But I think there's some confusion here.  John never 
said that there was a problem with it -- he was just using it as a means of 
getting his measurements.  He could have clicked on the mozilla desktop icon 
instead and would probably get the same numbers.

Comment 47

[Peter Trudelle]

Comment #44 was just referring to comment #39, which referred to comment #38.  I
am unaware of any problems unique to that gesture.

Comment 48

[Peter Trudelle]

I took the reference to be specific. When talking about launch, let's just say
'launch'.

Comment 49

[John Morrison]

Ah, ok. Yes, the reference to the 'double-click on icon' could have just said
"launched the browser" instead. There is no specific problem associated with 
double-clicking on the icon in the systray. Sorry for the confusion.

Comment 50

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's behalf) for approval to checkin to the 1.0 branch, pending
Driver's approval.  After, checking in, please add the fixed1.0 keyword.

Comment 51

[Stephen P. Morse]

The patch in this bug has also fixed the following bugs:

bug 133552: quicklaunch message appears even when you tell it not to
bug 104872: quicklaunch message doesn't appear even when you tell it to
bug 138274: need to flush and release localstore.rdf on session-logout
bug 139226: can't start browser with quicklaunch and more than one profile
bug 106434: unload plugins

and possibly these as well:

bug 142229: crosstalk between profiles
bug 137881: component failure when switching profiles

but on the other hand it has created these two bugs:

bug 146340: quicklaunch icon momentarily disappears when closing last window
bug 147254: download manager dies when last window is closed

Comment 52

[Stephen P. Morse]

possibly also fixed:

   bug 115370: Hang when logging to SSL dlg

Comment 53

[Blake Ross]

When you update QuickLaunch, the QuickLaunch design spec
(http://mozilla.org/xpapps/QuickLaunch/) needs to be updated, too...otherwise it
will quickly get outdated.

Comment 54

[scottputterman]

Adding adt1.0.1+ for adt approval for branch checkin for the 1.0.1 milestone. 
Please get drivers approval before checking in.

Comment 55

[Stephen P. Morse]

One consequence of this patch is that the session-logout signal is no longer 
being sent.  The following bugs have been opened to clear out the unnecessary 
code that handles this signal:

   bug 148187 for cookies
   bug 148189 for embedding
   bug 148191 for preferences
   bug 148192 for netwerking
   bug 148194 for security
   bug 148195 for mailnews

Comment 56

[Blake Ross]

Bug 148196 filed to update the QL spec.

Comment 57

[Randell Jesup [:jesup] (needinfo me)]

please check into the 1.0.1 branch ASAP. once landed remove the
mozilla1.0.1+ keyword and add the fixed1.0.1 keyword

Comment 58

[Bill Law]

Could we test whether this code does indeed fix all the residual
profile-switching problems that are cited above?  Seems like we need to know
that to figure out how to resolve the various open QL issues.

Comment 59

[Peter Trudelle]

Yes, it would be good to ensure that the 'blocked' bugs are verified on the trunk.

Comment 60

[Brian 'netdragon' Bober]

How is this bug fixed if it depends on an unfixed bug?

Comment 61

[Stephen P. Morse]

That dependency was created by [e-mail] with no explanation 
as to why.

Comment 62

[Jaime Rodriguez, Jr.]

jrgm: can you pls verify this as fixed, then replace "fix1.0.1", with
:veified1.0.1." thanks!