123728 - periodically HeapMinimize

Status: RESOLVED FIXED

(SeaMonkey :: General, defect, P1)

Description

[Cathleen]

Comment 1

[Garrett Arch Blythe]

Just a small dump of what I know regarding this that wasn't stated in the meeting:

HeapCompact() is the Win32 API function (valid only under NT).
_heapmin() is the MSVC LIBC function.

It has been a long standing belief of mine that _heapmin simply calls
HeapCompact on the personal MSVC LIBC heap; otherwise
HeapCompact(GetProcessHeap(), 0) would do the job....  Last I looked, this was
the case along with some other types of housekeeping the MSVC LIBC allocator was
doing; the remainder of the MSVC LIBC allocation functions proxy to the Heap API
with some overhead as well.
Further, if only NT will gain anything from a HeapCompact call here or there, as
it is the only windows variant which keeps a free list hanging around until
HeapCompact for performance, then the value of this particular experiement is
only good for NT users.

Extra Credit (off topic as well....):    Why use the MSVC LIBC allocator in the
first place when you could remove the call overhead and go to the kernel Heap
API directly?  My observations regarding the LIBC allocator is that it generally
gives you 16 bytes  in which you can grow a little more before getting moved via
realloc.  The Heap API appears to give you only 8 bytes in which to grow before
getting moved.  Our frequency of realloc is low, no?, I bet this to be an
overall win.

Comment 2

[Suresh Duddi (gone)]

Yes. Our frequency of realloc is pretty low. Do you know if we can allocate
using HeapAlloc() and free() the result ?

Comment 3

[Garrett Arch Blythe]

don't think free(HeapAlloc()) is a good idea.
if mixing allocators is a concern, then not a good idea.
if there were a special subset that we knew were not intermingled, then that is
a candidate, but this may not be feasible....  anyhow, just a thought.

Comment 4

[Suresh Duddi (gone)]

It doesnt look like I am getting this right. Here is what I get from the
following patch:

DEBUG-dp: nsXULWindow::Destroy() - 0x05019A90 HeapCompaction in 10
secs...started...ends.
heap = 1245184, HeapCompact returned 835584 [0 ms]

The data shown in TaskManager (RSS I think) is not changing. Guess this is
because the pages that got returned to OS were not resident ? Or am I doing
something wrong ? It is pretty strange that HeapCompact() took no time.

Index: nsXULWindow.cpp
===================================================================
RCS file: /cvsroot/mozilla/xpfe/appshell/src/nsXULWindow.cpp,v
retrieving revision 1.98
diff -c -r1.98 nsXULWindow.cpp
*** nsXULWindow.cpp	5 Feb 2002 23:26:50 -0000	1.98
--- nsXULWindow.cpp	7 Feb 2002 03:26:10 -0000
***************
*** 66,71 ****
--- 66,75 ----
  
  #include "nsStyleConsts.h"
  
+ #if defined(DEBUG_dp) && defined(XP_WIN32)
+ #include <windows.h>
+ #endif
+ 
  #define ABS(x) ((x)<0?-(x):x)
  
  // XXX Get rid of this
***************
*** 408,413 ****
--- 412,434 ----
        mWindow = nsnull;
     }
  
+ #if defined(DEBUG_dp)
+    {
+      // Heap compaction now ?
+      printf("DEBUG-dp: nsXULWindow::Destroy() - 0x%p HeapCompaction in 10
secs...", this);
+      PR_Sleep(PR_SecondsToInterval(10));
+      PRIntervalTime start = PR_IntervalNow();
+      printf("started...");
+      HANDLE heap = GetProcessHeap();
+ 
+      UINT ret = 0;
+      ret = HeapCompact(heap, 0);
+ 
+      PRIntervalTime end = PR_IntervalNow();
+      printf("ends.\nheap = %d, HeapCompact returned %d [%d ms]\n", heap, ret,
+             PR_IntervalToMilliseconds(end-start));
+    }
+ #endif
     return NS_OK;
  }

Comment 5

[Suresh Duddi (gone)]

Ok I used taskinfo2000 and vm doesn't change at all before and after the call. I
am guessing we are working on the wrong heap - is that possible ?

Comment 6

[Garrett Arch Blythe]

I read your patch as compacting the wrong heap:

You're compacting the process heap (GetProcessHeap()).
You need to compact the libc heap (via _heapmin(), or enumerate the heaps and
compact them all).

Comment 7

[Garrett Arch Blythe]

actually, do not enumerate all the heaps and compact them; just use _heapmin().
 some might have been specifically created without synchronization and you have
no way of telling.

Comment 8

[Suresh Duddi (gone)]

Attachment: Patch to try out heapcompaction on window close

This is only a test. Real patch would need to do some additional magic to make
sure popups dont cause heap compaction.

but then, I dont see much of a change happening with this. I am using
_heapmin() here, as Garrett suggested. So no big wins here I think. Still we
will do it for what it is worth.

Sidenote: At some point I see the Vm usage jump from 70MB (readmail, sent mail,
read a couple of news articles from netscape.com) to 90MB and I am unable to
nail what is causing that. That seems like an interesting thing.

Comment 9

[Suresh Duddi (gone)]

Attachment: heap compaction on win32 : First attempt

Still need to do the following:

- Need to find the right ifdef for NT and above only. <showstopper>
- No need to compact when we are quitting (last window close) <minor>
- No need to compact on ad popups. We already dont compact on dialog close.
  Danm mentioned there is a heuristic based on whether size was specified or
  not. Will incorporate that. <minor>

Comment 10

[Suresh Duddi (gone)]

Attachment: Heap compaction on window close

Garrett think win98 should be fine with this.

Improvements that could be made:
- Dont compact on closing web popup ad windows
- Dont compact on last window close

Since the compact takes almost no time, I am not going to sweat on it.

Comment 11

[Garrett Arch Blythe]

Comment on attachment 68466 [details] [diff] [review]
Heap compaction on window close

r=blythe

Comment 12

[Suresh Duddi (gone)]

Attachment: heapcompaction logic + residentset shrinking

Garrett could you review this additional residentset reduction logic too.

Comment 13

[Suresh Duddi (gone)]

Comment on attachment 68618 [details] [diff] [review]
heapcompaction logic + residentset shrinking

Add WINAPI for the typedef and r=blythe (from Garrett)

Comment 14

[Suresh Duddi (gone)]

Attachment: Added WINAPI to the typedef

Comment 15

[Suresh Duddi (gone)]

Comment on attachment 68680 [details] [diff] [review]
Added WINAPI to the typedef

r=blythe

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 68680 [details] [diff] [review]
Added WINAPI to the typedef

sr=[e-mail]

The circle of shame grows!

/be

Comment 17

[Suresh Duddi (gone)]

Fix checked in.