PageHeap::New fails after large TCMalloc_SystemAlloc in PageHeap::GrowHeap fails

[GoogleCodeExporter]

I am convinced I have identified a bug in the current TCMalloc 
implementation. The bug shows when first call to TCMalloc_SystemAlloc 
from PageHeap::GrowHeap fails but the second requesting n pages 
succeeds. In this case the resulting span is not stored in large_, as 
expected, but rather in free_[n].

Original issue reported on code.google.com by ond...@bistudio.com on 28 Jan 2011 at 7:26

[GoogleCodeExporter]

I suggested a fix as follows: 

In page_heap.cc at the end of Span* PageHeap::New(Length n) 
current: 

  return AllocLarge(n); 
replace with: 

  result = AllocLarge(n); 
  if (result != NULL) return result; 
  // GrowHeap may have grown only n instead of large (kMinSystemAlloc) 
  Span* ll = &free_[n].normal; 
  // If we're lucky, ll is non-empty, meaning it has a suitable span. 
  if (!DLL_IsEmpty(ll)) { 
    ASSERT(ll->next->location == Span::ON_NORMAL_FREELIST); 
    return Carve(ll->next, n); 
  } 
  // Alternatively, maybe there's a usable returned span. 
  ll = &free_[n].returned; 
  if (!DLL_IsEmpty(ll)) { 
    ASSERT(ll->next->location == Span::ON_RETURNED_FREELIST); 
    return Carve(ll->next, n); 
  } 
  return NULL;

I have identified this bug when experimenting with porting TCMalloc 
into a windows application, I am unable to judge if and how it is 
relevant for Linux, but I think the fix is very safe and fixes real 
logic error on the side of PageHeap::New, which should not assume 
AllocLarge will create a span in large_, therefore it seems to be it 
is applicable on all plaftorms. 

Original comment by ond...@bistudio.com on 28 Jan 2011 at 7:26

[GoogleCodeExporter]

Thanks, I'll have the experts at that part of the code take a look, and see 
what they think.  In the meantime, can you sign the CLA?
   http://code.google.com/legal/individual-cla-v1.0.html

Also, is it possible for you to develop a unittest case that exercises this 
bug?  That would help make sure we don't accidentally regress here.

Original comment by csilv...@gmail.com on 28 Jan 2011 at 10:36

• Added labels: Priority-Medium, Type-Defect

[GoogleCodeExporter]

Original comment by csilv...@gmail.com on 28 Jan 2011 at 10:45

• Changed state: Started

[GoogleCodeExporter]

> Also, is it possible for you to develop a unittest case that exercises this 
bug?  That would help make sure we don't accidentally regress here.

I would be happy to, however I am afraid I am not familiar enough with your 
framework so that I do not know what "infrastructure" is expected for a unit 
test. (I am currently using TCMalloc as a bunch of somewhat modified sources 
compiled right in my application.) Futhermore I am more or less unable do 
develop anything on Linux, and I am not sure if Windows test would be helpful 
for you.

The principle of the test would be (with some possible finetuning):

- allocate very much of virtual memory by some artificial means (VirtualAlloc 
on Windows) so that any allocation for "large" fails, but still keep some space 
free, so that a smaller allocation can succeed. This can be done by allocating 
exactly "large" repeatedly until it fails, and then allocating e.g. large/2.

Original comment by ond...@bistudio.com on 29 Jan 2011 at 8:52

[GoogleCodeExporter]

OK, I've got a modified version of your patch which will make it into the next 
release.

I agree with you it will be hard to come up with a unittest.  Using up 
almost-all virtual memory can cause all sorts of problems, which can make 
testing this fragile.  I think the revamped code is pretty straightforward, 
though, so hopefully it will be ok not having a special test just for that.

Original comment by csilv...@gmail.com on 1 Feb 2011 at 7:05

[GoogleCodeExporter]

This should be fixed in perftools 1.7, just released.

Original comment by csilv...@gmail.com on 5 Feb 2011 at 12:25

• Changed state: Fixed