Report memory retained by memory allocator internals.

Report memory usage retained by TCMalloc freelists.


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171767

[alexeif, 2012-08-07 17:46:46]

This function is needed for native memory instrumentation effort in Web
Inspector.

[darin (slow to review), 2012-08-07 22:06:14]

https://chromiumcodereview.appspot.com/10823205/diff/3001/webkit/glue/webkitp...
File webkit/glue/webkitplatformsupport_impl.cc (right):

https://chromiumcodereview.appspot.com/10823205/diff/3001/webkit/glue/webkitp...
webkit/glue/webkitplatformsupport_impl.cc:765: bool
WebKitPlatformSupportImpl::memoryAllocatorWasteInBytes(size_t* psize) {
nit: avoid Hungarian notation... psize -> size

https://chromiumcodereview.appspot.com/10823205/diff/3001/webkit/glue/webkitp...
webkit/glue/webkitplatformsupport_impl.cc:767: MallocExtension* malloc_ext =
MallocExtension::instance();
nit: indentation

[jamesr, 2012-08-07 22:15:59]

https://chromiumcodereview.appspot.com/10823205/diff/3001/webkit/glue/webkitp...
File webkit/glue/webkitplatformsupport_impl.cc (right):

https://chromiumcodereview.appspot.com/10823205/diff/3001/webkit/glue/webkitp...
webkit/glue/webkitplatformsupport_impl.cc:766: #if defined(USE_TCMALLOC)
having defined(USE_TCMALLOC) set does not mean that you are definitely using
tcmalloc on all platforms. On windows, we support runtime allocator switching
(via an environment variable) so there's no way to know at compile time which
allocator you are going to be using.  Read base/allocator/README and the other
files in that directory for some more background.

Having this somewhat direct dependency on tcmalloc is also somewhat unfortunate.
 Can you look at base/allocator/allocator_extension and plumb this extension
through that mechanism instead?

[alexeif, 2012-08-08 17:18:36]

Thanks for your comments folks! PTAL

https://chromiumcodereview.appspot.com/10823205/diff/3001/webkit/glue/webkitp...
File webkit/glue/webkitplatformsupport_impl.cc (right):

https://chromiumcodereview.appspot.com/10823205/diff/3001/webkit/glue/webkitp...
webkit/glue/webkitplatformsupport_impl.cc:765: bool
WebKitPlatformSupportImpl::memoryAllocatorWasteInBytes(size_t* psize) {
On 2012/08/07 22:06:14, darin wrote:
> nit: avoid Hungarian notation... psize -> size

Done.

https://chromiumcodereview.appspot.com/10823205/diff/3001/webkit/glue/webkitp...
webkit/glue/webkitplatformsupport_impl.cc:766: #if defined(USE_TCMALLOC)
On 2012/08/07 22:15:59, jamesr wrote:
> having defined(USE_TCMALLOC) set does not mean that you are definitely using
> tcmalloc on all platforms. On windows, we support runtime allocator switching
> (via an environment variable) so there's no way to know at compile time which
> allocator you are going to be using.  Read base/allocator/README and the other
> files in that directory for some more background.
> 
> Having this somewhat direct dependency on tcmalloc is also somewhat
unfortunate.
>  Can you look at base/allocator/allocator_extension and plumb this extension
> through that mechanism instead?
done it in a separate CL https://chromiumcodereview.appspot.com/10825250/

https://chromiumcodereview.appspot.com/10823205/diff/3001/webkit/glue/webkitp...
webkit/glue/webkitplatformsupport_impl.cc:767: MallocExtension* malloc_ext =
MallocExtension::instance();
On 2012/08/07 22:06:14, darin wrote:
> nit: indentation

Done.

[jamesr, 2012-08-10 18:59:37]

I don't think it makes sense to expose such a specific detail of a specific
allocator up to WebKit - there's really nothing you can do with it up there. 
What's the use case for someone using devtools to know the size of the tcmalloc
freelists?  We already have instrumentation and (some) UI for this in chrome
(about:tcmalloc), I would think that anyone interesting in tuning tcmalloc would
be using that (or the chrome task manager or something like that) and not
devtools.

[jamesr, 2012-08-10 19:01:05]

One way to take care of the "black hole" problem would be to expose something
more generic for the embedder to report physical memory use that's completely
outside of WebKit's control explicitly, so at least you would know if something
was missing.  I would expect this to include a lot more than just tcmalloc
freelists (especially since we don't even use tcmalloc everywhere).

[alexeif, 2012-08-11 11:33:28]

On 2012/08/10 18:59:37, jamesr wrote:
> I don't think it makes sense to expose such a specific detail of a specific
> allocator up to WebKit - there's really nothing you can do with it up there. 
> What's the use case for someone using devtools to know the size of the
tcmalloc
> freelists?  We already have instrumentation and (some) UI for this in chrome
> (about:tcmalloc), I would think that anyone interesting in tuning tcmalloc
would
> be using that (or the chrome task manager or something like that) and not
> devtools.

We want to instrument all the physical memory consumers in the renderer process.
The goal is not to help the user to tune tcmalloc, but just report the memory
usage breakdown by components. Memory allocator seems to be a pretty significant
memory consumer. I saw it footprint went up to 12% of process physical memory.
Funny enough allocator internals sometime take more memory than DOM structures.
So we definitely want to list it on the chart.

Another point of listing as many components as possible is that we have users
reporting that some pages take gigabytes of memory. And we don't know which
component(s) is responsible for that. If we don't add a component and it happens
to be responsible for memory we won't know that.

[alexeif, 2012-08-11 11:37:27]

On 2012/08/10 18:59:37, jamesr wrote:
> I don't think it makes sense to expose such a specific detail of a specific
> allocator up to WebKit - there's really nothing you can do with it up there. 
> What's the use case for someone using devtools to know the size of the
tcmalloc
> freelists?  We already have instrumentation and (some) UI for this in chrome
> (about:tcmalloc), I would think that anyone interesting in tuning tcmalloc
would
> be using that (or the chrome task manager or something like that) and not
> devtools.
Well I'm not going to expose specific details of a particular memory allocator.
All I need to know is how much memory is used for an allocator internal data,
i.e. physically allocated, but not returned to application. You see the function
is called memoryAllocatorWasteInBytes. Don't take the CL header into account,
it's kinda outdated.

[yurys, 2012-08-13 06:46:34]

On 2012/08/10 19:01:05, jamesr wrote:
> One way to take care of the "black hole" problem would be to expose something
> more generic for the embedder to report physical memory use that's completely
> outside of WebKit's control explicitly, so at least you would know if
something
> was missing.  I would expect this to include a lot more than just tcmalloc
> freelists (especially since we don't even use tcmalloc everywhere).

Having an opaque chunk of embedder's memory won't help you understand where
those 8 out of 9.5Gb's of process's memory were lost. It is not better than
showing it as "unknown" part  while attributing this memory to the memory
allocator could mean that it might be a fragmentation problem and would give you
a hint where to start looking for a problem. Not all such problems can be solved
on the user side but when we narrow down the problem to a specific component it
will be easier to find specific people that could help. And if the problem
recurs we should be able to do either of the following:

1) Figure out if the problem is in the WebKit/embedder (which basically makes no
difference for web developer) and if so do either of the following
  a) Fix the problem in the browser.
  b) Explain the people that it is bare minimum of the system resources required
for performing that task
2) If the problem can only be solved on the user's side we will be able to
provide them with better developer tools that would help optimize that
particular scenario.

[jamesr, 2012-08-14 22:30:47]

On 2012/08/13 06:46:34, Yury Semikhatsky wrote:
> On 2012/08/10 19:01:05, jamesr wrote:
> > One way to take care of the "black hole" problem would be to expose
something
> > more generic for the embedder to report physical memory use that's
completely
> > outside of WebKit's control explicitly, so at least you would know if
> something
> > was missing.  I would expect this to include a lot more than just tcmalloc
> > freelists (especially since we don't even use tcmalloc everywhere).
> 
> Having an opaque chunk of embedder's memory won't help you understand where
> those 8 out of 9.5Gb's of process's memory were lost.

We should never lose this much (or anywhere close to that much) in the
allocator.  If we ever did, then that's a bug in the allocator.  Really the
"waste" factor here from the allocator should be very very low.

I think the real root of the problem here is that you are looking at the memory
allocation patterns on linux.  tcmalloc was developed and tuned for server
operations where the typical allocation pattern is a very low use between
requests and "spikes" as the server handles queries.  In that situation, you
want to keep a sizable buffer of committed-but-available memory in the allocator
to help it absorb spikes.  Browsers don't really work that way - the memory use
is a lot flatter.  On windows, we've tuned the allocator to match this flat
allocation pattern more closely.  I would be really surprised if you ever saw
the "waste" factor get more than a small single-digit %s on windows.  If you do,
file a bug.

I think what you should if this memory use is bothering you is to go fix it at
the source - in our allocator.  It'll never be zero - we have to use some memory
for allocator metadata and we can't map C++ allocations exactly to page sizes in
all cases - but the overhead should be low enough that an end developer
shouldn't care about it.  Then add histograms, etc, to track this so that we
(the chromium developers responsible for the allocator's behavior) can track if
this use is getting out of hand and fix it.

[yurys, 2012-08-15 06:37:10]

On 2012/08/14 22:30:47, jamesr wrote:
> On 2012/08/13 06:46:34, Yury Semikhatsky wrote:
> > On 2012/08/10 19:01:05, jamesr wrote:
> > > One way to take care of the "black hole" problem would be to expose
> something
> > > more generic for the embedder to report physical memory use that's
> completely
> > > outside of WebKit's control explicitly, so at least you would know if
> > something
> > > was missing.  I would expect this to include a lot more than just tcmalloc
> > > freelists (especially since we don't even use tcmalloc everywhere).
> > 
> > Having an opaque chunk of embedder's memory won't help you understand where
> > those 8 out of 9.5Gb's of process's memory were lost.
> 
> We should never lose this much (or anywhere close to that much) in the
> allocator.  If we ever did, then that's a bug in the allocator.  Really the
> "waste" factor here from the allocator should be very very low.
> 
> I think the real root of the problem here is that you are looking at the
memory
> allocation patterns on linux.  tcmalloc was developed and tuned for server
> operations where the typical allocation pattern is a very low use between
> requests and "spikes" as the server handles queries.  In that situation, you
> want to keep a sizable buffer of committed-but-available memory in the
allocator
> to help it absorb spikes.  Browsers don't really work that way - the memory
use
> is a lot flatter.  On windows, we've tuned the allocator to match this flat
> allocation pattern more closely.  I would be really surprised if you ever saw
> the "waste" factor get more than a small single-digit %s on windows.  If you
do,
> file a bug.
> 
> I think what you should if this memory use is bothering you is to go fix it at
> the source - in our allocator.  It'll never be zero - we have to use some
memory
> for allocator metadata and we can't map C++ allocations exactly to page sizes
in
> all cases - but the overhead should be low enough that an end developer
> shouldn't care about it.  Then add histograms, etc, to track this so that we
> (the chromium developers responsible for the allocator's behavior) can track
if
> this use is getting out of hand and fix it.

We cannot say if it is bothering us without having the data about memory
distribution. Looking at 1Gb of 'unknown' memory you can't say if it is
a leaked buffer or if it is a memory allocator overhead. The more details
on the memory usage we have, the higher chances we can tell where the root
of the problem is. And now that we know it takes some noticeable part of the
process memory why hide it from the user?

[jamesr, 2012-08-15 16:14:42]

On 2012/08/15 06:37:10, Yury Semikhatsky wrote:
 
> We cannot say if it is bothering us without having the data about memory
> distribution. 

You can (and should) gather this data in chrome, via about:tcmalloc histograms
etc.

> Looking at 1Gb of 'unknown' memory you can't say if it is
> a leaked buffer or if it is a memory allocator overhead.

The memory allocator overhead should never be 1GB.  It's our job as chromium
developers to make sure that is the case.

>  The more details
> on the memory usage we have, the higher chances we can tell where the root
> of the problem is. And now that we know it takes some noticeable part of the
> process memory why hide it from the user?

Memory allocator overhead should not be a noticeable part of process memory. 
Fix that, and you've resolved the issue.

[yurys, 2012-08-16 10:22:13]

message: On 2012/08/15 16:14:42, jamesr wrote:
> On 2012/08/15 06:37:10, Yury Semikhatsky wrote:
>  
> > We cannot say if it is bothering us without having the data about memory
> > distribution. 
> 
> You can (and should) gather this data in chrome, via about:tcmalloc histograms
> etc.
> 
about:tcmalloc gives you a way more details on the wasted memory than we'd like
to show at the highest level of the memory breakdown. Also it is probably only
useful if you are tuning the allocator/optimizing object allocations as a
browser developer. At the same time it doesn't give you the full view of the
memory distribution. We have JS heap profiler which gives you complete picture
of the v8 heap but it doesn't mean that we don't want to see how much memory v8
heap takes as a whole. I think what we should do here is to include the wasted
memory section into the process memory view and make it possible to drill down
into its details e.g. by linking the section to chrome://tcmalloc. This way web
developer can see the full picture and find suspicious  components without going
to several different places. After that he could either use dedicated tools like
chrome://tcmalloc or report the problem to the browser developers.


> > Looking at 1Gb of 'unknown' memory you can't say if it is
> > a leaked buffer or if it is a memory allocator overhead.
> 
> The memory allocator overhead should never be 1GB.  It's our job as chromium
> developers to make sure that is the case.
> 
> >  The more details
> > on the memory usage we have, the higher chances we can tell where the root
> > of the problem is. And now that we know it takes some noticeable part of the
> > process memory why hide it from the user?
> 
> Memory allocator overhead should not be a noticeable part of process memory. 
> Fix that, and you've resolved the issue.
As you said it will never be zero and the amount of "wasted" memory will be
different depending on the allocation patterns. I agree that the overhead of the
memory allocator shouldn't be noticeable but now it is. We see that it can take
more than 10% of the total private memory and in most cases it is more than 5%
of the process memory which as Alexei said is more than the memory allocated for
the DOM structures. I don't think we can go and easily reduce the overhead by an
order of magnitude. Also tuning the memory allocator is a separate task.
in_reply_to:
ahdzfmNocm9taXVtY29kZXJldmlldy1ocnIcCxIFSXNzdWUYpcyUBQwLEgdNZXNzYWdlGMxlDA
send_mail: 1
subject: Report memory usage retained by TCMalloc freelists.

[jamesr, 2012-08-16 17:19:41]

On 2012/08/16 10:22:13, Yury Semikhatsky wrote:
> message: On 2012/08/15 16:14:42, jamesr wrote:
> > On 2012/08/15 06:37:10, Yury Semikhatsky wrote:
> >  
> > > We cannot say if it is bothering us without having the data about memory
> > > distribution. 
> > 
> > You can (and should) gather this data in chrome, via about:tcmalloc
histograms
> > etc.
> > 
> about:tcmalloc gives you a way more details on the wasted memory than we'd
like
> to show at the highest level of the memory breakdown. Also it is probably only
> useful if you are tuning the allocator/optimizing object allocations as a
> browser developer.

Yes, exactly.  The browser developer is the only person who should ever care.

> > > Looking at 1Gb of 'unknown' memory you can't say if it is
> > > a leaked buffer or if it is a memory allocator overhead.
> > 
> > The memory allocator overhead should never be 1GB.  It's our job as chromium
> > developers to make sure that is the case.
> > 
> > >  The more details
> > > on the memory usage we have, the higher chances we can tell where the root
> > > of the problem is. And now that we know it takes some noticeable part of
the
> > > process memory why hide it from the user?
> > 
> > Memory allocator overhead should not be a noticeable part of process memory.

> > Fix that, and you've resolved the issue.
> As you said it will never be zero and the amount of "wasted" memory will be
> different depending on the allocation patterns. I agree that the overhead of
the
> memory allocator shouldn't be noticeable but now it is. We see that it can
take
> more than 10% of the total private memory and in most cases it is more than 5%
> of the process memory which as Alexei said is more than the memory allocated
for
> the DOM structures. I don't think we can go and easily reduce the overhead by
an
> order of magnitude. Also tuning the memory allocator is a separate task.

Have you tried?

[pfeldman, 2012-08-17 08:25:30]

@jamesr:

This thread got a bit big. So yes, we are fine with perceiving this information
as opaque memory that is allocated by the browser. But the structure of the
upstream memory instrumentation allows diving into the details. I see no harm in
claiming it is related to tcmalloc if user decided to dive into this "Other
things you can't affect" bullet of the memory graph. It sounds like
implementation / maintenance complexity is not high + we'll make sure it does
not confuse the user from the UI standpoint.

[alexeif, 2012-08-17 21:42:24]

On 2012/08/14 22:30:47, jamesr wrote:
> On 2012/08/13 06:46:34, Yury Semikhatsky wrote:
> > On 2012/08/10 19:01:05, jamesr wrote:
> > > One way to take care of the "black hole" problem would be to expose
> something
> > > more generic for the embedder to report physical memory use that's
> completely
> > > outside of WebKit's control explicitly, so at least you would know if
> > something
> > > was missing.  I would expect this to include a lot more than just tcmalloc
> > > freelists (especially since we don't even use tcmalloc everywhere).
> > 
> > Having an opaque chunk of embedder's memory won't help you understand where
> > those 8 out of 9.5Gb's of process's memory were lost.
> 
> We should never lose this much (or anywhere close to that much) in the
> allocator.  If we ever did, then that's a bug in the allocator.  Really the
> "waste" factor here from the allocator should be very very low.
> 
> I think the real root of the problem here is that you are looking at the
memory
> allocation patterns on linux.  tcmalloc was developed and tuned for server
> operations where the typical allocation pattern is a very low use between
> requests and "spikes" as the server handles queries.  In that situation, you
> want to keep a sizable buffer of committed-but-available memory in the
allocator
> to help it absorb spikes.  Browsers don't really work that way - the memory
use
> is a lot flatter.  On windows, we've tuned the allocator to match this flat
> allocation pattern more closely.  I would be really surprised if you ever saw
> the "waste" factor get more than a small single-digit %s on windows.  If you
do,
> file a bug.
We know it's usually a single digit, but we have already got a dozen components
each taking 1-2%, while their total easily exceeds 20%. I don't want to show the
user a big grey sector for these components. We'd really like to be able to
report where these 20% come from. 
> 
> I think what you should if this memory use is bothering you is to go fix it at
> the source - in our allocator.  It'll never be zero - we have to use some
memory
> for allocator metadata and we can't map C++ allocations exactly to page sizes
in
> all cases - but the overhead should be low enough that an end developer
> shouldn't care about it.  Then add histograms, etc, to track this so that we
> (the chromium developers responsible for the allocator's behavior) can track
if
> this use is getting out of hand and fix it.

[alexeif, 2012-09-11 13:29:06]

Sorry for not replying earlier, I'm just back from vacation.

On 2012/08/16 17:19:41, jamesr wrote:
> On 2012/08/16 10:22:13, Yury Semikhatsky wrote:
> > about:tcmalloc gives you a way more details on the wasted memory than we'd
> like
> > to show at the highest level of the memory breakdown. Also it is probably
only
> > useful if you are tuning the allocator/optimizing object allocations as a
> > browser developer.
> 
> Yes, exactly.  The browser developer is the only person who should ever care.

That's true, the user cannot do much about memory consumed by the allocator,
as well as memory used by some other components.
But the fact is that the memory is allocated from the system and its amount
is quite significant for us (5-7%), so we need to report it. Just same way we
report
other components e.g. font cache (5%), DOM tree (6%), inspector data (3%) etc.  

> > As you said it will never be zero and the amount of "wasted" memory will be
> > different depending on the allocation patterns. I agree that the overhead of
> the
> > memory allocator shouldn't be noticeable but now it is. We see that it can
> take
> > more than 10% of the total private memory and in most cases it is more than
5%
> > of the process memory which as Alexei said is more than the memory allocated
> for
> > the DOM structures. I don't think we can go and easily reduce the overhead
by
> an
> > order of magnitude. Also tuning the memory allocator is a separate task.
> 
> Have you tried?

No. We're currently focused on *reporting* how much memory is consumed by each
component.
As soon as we get the whole picture, it will be possible to prioritize which
components
should get an attention.

As of allocator waste I can see it usually floats around 5-7% which is an
expected value.
It is not enormous, so I don't think we should rush to optimize it out
immediately.

[alexeif, 2012-09-24 12:14:31]

James, I've renamed the function (Waste -> Internals) as we agreed in the WebKit
part of the fix. Could you please take a look. Thank you.

[alexeif, 2012-10-11 13:46:36]

Hi James,
I've got an R+ for WebKit API https://bugs.webkit.org/show_bug.cgi?id=93372
So what about the implementation?

[darin (slow to review), 2012-10-18 18:02:54]

https://chromiumcodereview.appspot.com/10823205/diff/19001/webkit/glue/webkit...
File webkit/glue/webkitplatformsupport_impl.cc (right):

https://chromiumcodereview.appspot.com/10823205/diff/19001/webkit/glue/webkit...
webkit/glue/webkitplatformsupport_impl.cc:802: if
(base::allocator::GetProperty("generic.heap_size", &heap_size) &&
nit: Shouldn't there be some named constants for these strings?  It seems
fragile to hard-code these string literals here.

Alternatively, would it make sense for base::allocator:: to declare a method
that corresponds to memoryAllocatorInternalsInBytes?

One more note: I find the name memoryAllocatorInternalsInBytes to be quite
mysterious?  What does it mean?  Is it trying to count the number of bytes that
get used up by the allocator's own data structures?

[alph-g, 2012-10-22 12:19:41]

https://chromiumcodereview.appspot.com/10823205/diff/19001/webkit/glue/webkit...
File webkit/glue/webkitplatformsupport_impl.cc (right):

https://chromiumcodereview.appspot.com/10823205/diff/19001/webkit/glue/webkit...
webkit/glue/webkitplatformsupport_impl.cc:802: if
(base::allocator::GetProperty("generic.heap_size", &heap_size) &&
On 2012/10/18 18:02:54, darin wrote:
> nit: Shouldn't there be some named constants for these strings?  It seems
> fragile to hard-code these string literals here.
> 
> Alternatively, would it make sense for base::allocator:: to declare a method
> that corresponds to memoryAllocatorInternalsInBytes?

Hmm, I don't see any significant benefits of a generic
base::allocator::GetPropery with a string selector over the dedicated function.
I think it is possible to go with your suggestion. If we ever need another
function we can consider other options or return back to GetProperty way. I'll
implement the version you suggested.

> One more note: I find the name memoryAllocatorInternalsInBytes to be quite
> mysterious?  What does it mean?  Is it trying to count the number of bytes
that
> get used up by the allocator's own data structures?

Initially it was Waste instead of Internals, but James didn't like it. I took
the word Waste from the tcmalloc DumpStats report which btw also appears on the
chrome://tcmalloc/ page.

[alexeif, 2012-10-23 15:15:03]

Darin,
I have replaced generic GetProperty with GetAllocatorWasteSize.

[darin (slow to review), 2012-10-23 17:28:30]

https://chromiumcodereview.appspot.com/10823205/diff/30002/base/allocator/all...
File base/allocator/allocator_extension.cc (right):

https://chromiumcodereview.appspot.com/10823205/diff/30002/base/allocator/all...
base/allocator/allocator_extension.cc:14:
base::allocator::thunks::GetGetAllocatorWasteSizeFunction();
nit: no need for base::allocator:: prefix when you have code inside that
namespace.

https://chromiumcodereview.appspot.com/10823205/diff/30002/base/allocator/all...
base/allocator/allocator_extension.cc:40:
base::allocator::thunks::SetGetAllocatorWasteSizeFunction(
ditto:  no need for base::allocator::

https://chromiumcodereview.appspot.com/10823205/diff/30002/content/app/conten...
File content/app/content_main_runner.cc (right):

https://chromiumcodereview.appspot.com/10823205/diff/30002/content/app/conten...
content/app/content_main_runner.cc:489: if
(ext->GetNumericProperty("generic.heap_size", &heap_size) &&
nit: Same comment as before.  We shouldn't be hard-coding string literals here. 
Surely these strings appear elsewhere (on the implementation side), so they
should be exposed as constants that we can use in both places.

[alexeif, 2012-10-23 18:00:14]

https://chromiumcodereview.appspot.com/10823205/diff/30002/base/allocator/all...
File base/allocator/allocator_extension.cc (right):

https://chromiumcodereview.appspot.com/10823205/diff/30002/base/allocator/all...
base/allocator/allocator_extension.cc:14:
base::allocator::thunks::GetGetAllocatorWasteSizeFunction();
On 2012/10/23 17:28:30, darin wrote:
> nit: no need for base::allocator:: prefix when you have code inside that
> namespace.

Done.

https://chromiumcodereview.appspot.com/10823205/diff/30002/base/allocator/all...
base/allocator/allocator_extension.cc:40:
base::allocator::thunks::SetGetAllocatorWasteSizeFunction(
On 2012/10/23 17:28:30, darin wrote:
> ditto:  no need for base::allocator::

Done.

https://chromiumcodereview.appspot.com/10823205/diff/30002/content/app/conten...
File content/app/content_main_runner.cc (right):

https://chromiumcodereview.appspot.com/10823205/diff/30002/content/app/conten...
content/app/content_main_runner.cc:489: if
(ext->GetNumericProperty("generic.heap_size", &heap_size) &&
On 2012/10/23 17:28:30, darin wrote:
> nit: Same comment as before.  We shouldn't be hard-coding string literals
here. 
> Surely these strings appear elsewhere (on the implementation side), so they
> should be exposed as constants that we can use in both places.

I'm not sure. These strings are hardcoded in the tcmalloc implementation and not
present in the interface. So the interface of GetNumericProperty is plain
mapping String->Value.
I could go and declare them somewhere in tcmalloc headers... But although we
have a chromium port, anyway it's kinda third-party library. Moreover there will
be no sense to have string constants on one hand and the String->Value interface
on the other. Why not just change it to just Enum->Value then. So I really
wouldn't like to go this way. At least for this CL.

I added a DCHECK for the case when a tcmalloc property is not available.

[alexeif, 2012-12-01 00:39:15]

Darin, could you please take a look. I seem addressed your comments. Thank you!

[darin (slow to review), 2012-12-03 18:43:18]

LGTM

[commit-bot: I haz the power, 2012-12-03 19:01:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeif@chromium.org/10823205/39001

[commit-bot: I haz the power, 2012-12-03 19:34:56]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-04 20:22:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeif@chromium.org/10823205/50005

[commit-bot: I haz the power, 2012-12-04 20:46:33]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-04 22:11:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeif@chromium.org/10823205/44016

[commit-bot: I haz the power, 2012-12-04 22:55:46]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-07 07:56:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeif@chromium.org/10823205/53005

[commit-bot: I haz the power, 2012-12-07 12:19:52]

Change committed as 171767