62791 - Mac's realloc() is broken

Status: VERIFIED FIXED

(Core :: XPCOM, defect, P2)

Description

[Simon Fraser [no longer active]]

So I suck. Bigtime.

When I whacked the memory allocators last summer, I broke realloc:
<http://lxr.mozilla.org/mozilla/source/lib/mac/MacMemoryAllocator/src/
nsAllocatorManager.cp#667>

Note that it's throwing away the result of AllocatorResizeBlock(), and always 
doing a malloc() and copy. This both leaks and is costly (it both sucks and 
blows). Here's a patch, please review:

Index: mozilla/lib/mac/MacMemoryAllocator/src/nsAllocatorManager.cp
===================================================================
RCS file: /cvsroot/mozilla/lib/mac/MacMemoryAllocator/src/nsAllocatorManager.cp,v
retrieving revision 1.11
diff -b -u -2 -r1.11 nsAllocatorManager.cp
--- nsAllocatorManager.cp	2000/09/02 02:12:34	1.11
+++ nsAllocatorManager.cp	2000/12/14 00:58:04
@@ -678,13 +678,13 @@
         {
             case nsMemAllocator::eAllocatorTypeFixed:
-                ((nsFixedSizeAllocator*)allocator)->AllocatorResizeBlock(block, 
newSize);
+                newBlock = ((nsFixedSizeAllocator*)allocator)->
AllocatorResizeBlock(block, newSize);
                 break;
                 
             case nsMemAllocator::eAllocatorTypeSmall:
-                ((nsSmallHeapAllocator*)allocator)->AllocatorResizeBlock(block, 
newSize);
+                newBlock = ((nsSmallHeapAllocator*)allocator)->
AllocatorResizeBlock(block, newSize);
                 break;
                 
             case nsMemAllocator::eAllocatorTypeLarge:
-                ((nsLargeHeapAllocator*)allocator)->AllocatorResizeBlock(block, 
newSize);
+                newBlock = ((nsLargeHeapAllocator*)allocator)->
AllocatorResizeBlock(block, newSize);
                 break;
         }

Comment 1

[Simon Fraser [no longer active]]

You may ask yourself what kind of performance impact this will have. Well, the 
only code that uses realloc extensively is ImgLib, particularly for GIF display. 
So fixing this will make GIF rendering faster, and less leaky. JS also uses 
realloc, as does prefs

<http://lxr.mozilla.org/seamonkey/search?string=realloc>

Comment 2

[Mike Pinkerton (not reading bugmail)]

r=pinkerton

Comment 3

[Simon Fraser [no longer active]]

So I lied. The current realloc() doesn't leak, it just does extra malloc/copy 
when it could resize in place.

Comment 4

[Patrick C. Beard]

Looks like a good "optimization" to me. sr=beard

Comment 5

[Simon Fraser [no longer active]]

Fix checked in.

Comment 6

[lchiang]

Simon, to confirm, this is checked into the trunk and not the branch, right?
Also, how do we verify this bug fix?  Only through the code?

Comment 7

[Simon Fraser [no longer active]]

This is checked in on the trunk. It's a code-level fix, so hard to verify, but 
GIF rendering should be a little faster.

Comment 8

[Angela Butler]

Added branch accept to status whiteboard

Comment 9

[Angela Butler]

Please check this in on the branch ASAP. Thanks in advance,
.Angela...

Comment 10

[lchiang]

jrgm: can you help me verify this in the code for branch and trunk?  (Do you 
build on the mac?)

Comment 11

[John Morrison]

I don't build on Mac, no. At any rate, it would be difficult to verify this 
fix. However, the "fix" is clearly the right thing, it's checked-in, and 
multiple Mac folks have reviewed. I think this can be called verified.

Comment 12

[lchiang]

Verified this is checked into the trunk via bonsai.

I used warp's bonsai and can't find a checkin to this file nsAllocatorManager.cp
in the last month for all branches including the 20000922 branch.

Simon - did you check this into the branch also?

Comment 13

[John Morrison]

Yes, the checkin is here (er, in the URL field).

Comment 14

[lchiang]

I'm sorry, perhaps I need an education here.  I did find the checkin you
referenced, but I don't know if that is for both the trunk and the branch.  So,
someone had directed me to bonsai on warp to look for the checkin.
http://warp.netscape.com/webtools/bonsai/cvsqueryform.cgi

Comment 15

[John Morrison]

That checkin is to the Netscape_20000922_BRANCH for the mozilla tree, where 
most of the changes are made. (bonsai.mozilla.org shows the mozilla tree 
changes). warp's bonsai is, I believe, tracking the commercial tree (e.g., AIM 
and other Netscape-specific code). 

So, Simon's fix is on both the mozilla branch (01/03 rev 1.11.4.1)
and trunk (12/14 rev 1.12).  (Marking verified).