User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.7) Gecko/2009021906 Firefox/3.0.7 Build Identifier: GCAlloc::~GCAlloc zeroes out memory that has been previously freed. This needs to be fixed. Reproducible: Always

(In reply to comment #1) > Created an attachment (id=370232) [details] > [v1] patch I'm not sure... where do these pages get freed now, once this code goes away? They're allocated in GC::GetBits but not freed anywhere. Are we OK with that? I'm not sure what the protocol is for freeing memory, actually. Does everything need to be returned to the heap before we shut down or is the heap OK with lots of stuff floating around at shutdown time? Is this related to Tommy's comment about no real storage management for the bits pool?

With the attached patch a debug build should assert in ~GCHeap at shutdown, its important that these pages get freed.

The bits would be freed inside ~GC during shutdown. In the original code, the bits were being added to the bits' freelist after being deleted which could potentially be reused for another GCAlloc of the same size. This means that we would be writing to deleted memory. The fact that this problem has not showed up is surprising and scary. So removing the call to Free() in ~GCAlloc is fine. Per Tommy, bits should not be freed until the shutdown. However, we should ensure that the GCAllocs are deleted before the blocks corresponding to bits are free because ~GCAlloc calls FreeBits which references the bits. Currently, ~GC frees the bits before deleting GCAlloc instances. I will reorder the destruction order and upload a new patch.

In ~GC the original file had the following line after freeing the bits and destroying GCAlloc instances - // dtors for each GCAlloc will use this pageList = NULL; This is unnecessary because pageList would be NULL anyways after the while loop iteration. Seems like legacy code. I will be running the buildbot for this change to detect any injections since Tommy indicated that he had problems in the past when he tried to change the order of deletion.

> Per Tommy, bits should not be freed until the shutdown. I think its debatable whether a never shrinking bit set is a good thing. Its simple but one could devise a pathological case where we have tons more bits pages than we need. Maybe the out of page bits should be regular allocations? The bits allocator is super fast and perfect fit but these attributes may not justify the additional code. Another justification was that I thought putting all the bits on their own pages would increase the speed of the marking routine by increasing cache performance (this wasn't measured). This is another situation where we could use UNmanaged GC memory, ie GC::Alloc'd stuff that's not auto freed. I didn't want to use FixedMalloc b/c this in theory memory should be charged to the GC instance as far as the collection hueristics are concerned (again debatable, the bits allocs effect on GC hueristics is probably negligible). In summary I'm for the simplest solution that doesn't slow down marking.

(In reply to comment #7) > > Per Tommy, bits should not be freed until the shutdown. > > I think its debatable whether a never shrinking bit set is a good thing. Pretty clearly a bug IMO. We have a few of these: ZCT, mark stack, bits pool. I'm guessing only the mark stack is a problem in practice (because failure to extend it leads to system failure). We should log separate bugs for all three and deal with them as circumstances and test cases dictate. (I'm happy to take care of that.) Your notion of using regular GC objects for the bits pool is intriguing, though I don't understand what you mean by GC-allocated memory that isn't garbage collected.

SPAM's alloc function supported an UNMANAGED flag and that replaced FixedMalloc. Basically use the GCAlloc infrastructure to handle unmanaged allocs. If it works well enough maybe FixedMalloc goes away.

Fixed changeset: 1688:f424325ad998