Major changes to the Chrome allocator....

Major changes to the Chrome allocator.

Changes include:
* Fix tcmalloc to release memory.  Implements the TCMalloc_SystemCommit()
  mechanism so that tcmalloc can implement SystemRelease() and later 
  reuse that memory.
* Enable dynamic switching of allocators based on an environment variable.
  Users can now switch between tcmalloc, jemalloc, the default windows
  heap, and the windows low-fragmentation heap.
* Implements set_new_mode() across all allocators so that we can be sure
  that out-of-memory conditions are handled safely.

BUG=18345
TEST=none; plan to get all unit tests running through these allocators.



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=23140

[jar (doing other things), 2009-08-12 00:13:47]

http://codereview.chromium.org/165275/diff/1100/1110
File third_party/tcmalloc/README (right):

http://codereview.chromium.org/165275/diff/1100/1110#newcode7
Line 7: the default windows alloator
alloator-->allocator

http://codereview.chromium.org/165275/diff/1100/1110#newcode55
Line 55: "winlfh"    - Windows Low-Fragmentation heap
You should also give a pointer to where the "default" is established (when the
env var is missing).

http://codereview.chromium.org/165275/diff/1100/1103
File third_party/tcmalloc/allocator_shim.cc (right):

http://codereview.chromium.org/165275/diff/1100/1103#newcode10
Line 10: // so that only tcmalloc can be used as the allocator.
...so only the default allocator can be used.
               ^^^^^^^

http://codereview.chromium.org/165275/diff/1100/1103#newcode12
Line 12: 
Please add a TODO() such as:

TODO(): Ensure that all calls to tcmalloc have the proper call depth from the
"user code" so that debugging tools (Heapchecker?) can work.

http://codereview.chromium.org/165275/diff/1100/1103#newcode105
Line 105: default:
Please ad a case TCMALLOC: falling through into the default.  do_malloc() is not
recognizable as part of tcmalloc.

http://codereview.chromium.org/165275/diff/1100/1103#newcode133
Line 133: do_free(p);
Please add a comment:
// case TCMALLOC:

http://codereview.chromium.org/165275/diff/1100/1103#newcode148
Line 148: default:
Please add a fall through case feeding into default:
case TCMALLOC:

http://codereview.chromium.org/165275/diff/1100/1103#newcode163
Line 163: // TODO(mbelshe): Implement this more usefully.
"Implement stats for other allocators"

http://codereview.chromium.org/165275/diff/1100/1103#newcode209
Line 209: allocator = WINLFH;
an explicit test for TCMALLOC, and possibly an assert (of some flavor for this
phase of loading) if we get anything else??

http://codereview.chromium.org/165275/diff/1100/1103#newcode218
Line 218: return win_heap_init(true) ? 1 : 0;
Please add a TCMALLOC case and a default to fall through.

http://codereview.chromium.org/165275/diff/1100/1102
File third_party/tcmalloc/generic_allocators.cc (right):

http://codereview.chromium.org/165275/diff/1100/1102#newcode85
Line 85: void* result = realloc(p, n * size);
Perhaps you should do the overflow test as done above in line 69 when
multiplying n by size?

http://codereview.chromium.org/165275/diff/1100/1105
File third_party/tcmalloc/jemalloc/jemalloc.c (right):

http://codereview.chromium.org/165275/diff/1100/1105#newcode99
Line 99: #define MOZ_MEMORY_WINDOWS
You told me to remind you to put a comment near any lines you changed or added
in jemalloc.

http://codereview.chromium.org/165275/diff/1100/1108
File third_party/tcmalloc/jemalloc/jemalloc.h (right):

http://codereview.chromium.org/165275/diff/1100/1108#newcode1
Line 1: /* -*- Mode: C; tab-width: 8; c-basic-offset: 8 -*- */
Per discussion, I'm not code-reviewing much of jemalloc at this time, and any of
its headers.

http://codereview.chromium.org/165275/diff/1100/1115
File third_party/tcmalloc/page_heap.cc (right):

http://codereview.chromium.org/165275/diff/1100/1115#newcode256
Line 256: // Chrome:  Changed to 64MB
Should this be 128MB?   ... or did you want to use 13 rather than 14 in next
line?

http://codereview.chromium.org/165275/diff/1100/1115#newcode298
Line 298: // Stop scavenging.
Perhaps better would be:
Note that we only scavenge once successfully, and then we stop looking until the
release delay has passed yet again.

I couldn't quite grok the meaning of the old comment, and I do agree the flow
here is subtle and worthy of a comment.

http://codereview.chromium.org/165275/diff/1100/1114
File third_party/tcmalloc/port.cc (right):

http://codereview.chromium.org/165275/diff/1100/1114#newcode243
Line 243: }
Could you add an assert that ptr == end after the loop?

Also add one after loop below in SystemCommit().

http://codereview.chromium.org/165275/diff/1100/1114#newcode263
Line 263: void* newAddress = VirtualAlloc(ptr, commitSize, MEM_COMMIT,
PAGE_READWRITE);
nit: wrap at 80 columns.

http://codereview.chromium.org/165275/diff/1100/1113
File third_party/tcmalloc/tcmalloc.cc (right):

http://codereview.chromium.org/165275/diff/1100/1113#newcode241
Line 241: #if 0
Please add a comment, or at least use a readably symbol such as:

#ifdef DEFINE_LIBC_SYMBOLS_AND_BECOME_THE_ONLY_ALLOCATOR

http://codereview.chromium.org/165275/diff/1100/1113#newcode339
Line 339: #endif  // #ifdef 0
change 0 to match the define you choose above.

[jar (doing other things), 2009-08-12 00:15:51]

I should have been clearer and noted LGTM with changes or responses to my
comments.

Thanks,

Jim