Add a histogram for renderer memory usage.

Add a histogram for renderer memory usage.

The difference to Renderer.Memory are:
- we're looking at the memory held by our allocator, and not what the system reports
- we're sampling not only once an hour, but much more often
- the memory is only measured if the command line flag --memory-metrics is given.

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

[marja, 2013-01-10 19:31:32]

jar, creis: Could you review this?

This is a modified version of jochen's CL from couple of weeks ago (
https://chromiumcodereview.appspot.com/11361219/ ).

- The histogram getting is behind a command line flag; without the flag, we
don't create the additional TaskObserver.
- Since the histogram is used only in testing, we don't need to go through
HistogramCustomizer.
- We're bypassing the caching of the memory value.
- We're getting the value in KB instead of MB.

[jar (doing other things), 2013-01-14 19:41:18]

Modulo this question below, this looks fine, so creis should see if this is what
he wants.

https://codereview.chromium.org/11829032/diff/9012/base/metrics/histogram.h
File base/metrics/histogram.h (right):

https://codereview.chromium.org/11829032/diff/9012/base/metrics/histogram.h#n...
base/metrics/histogram.h:220: #define HISTOGRAM_MEMORY_KB(name, sample)
HISTOGRAM_CUSTOM_COUNTS( \
Do you deliberately want a histogram that is not going to be uploaded to UMA, as
compared with what is on line 328?

[marja, 2013-01-14 19:42:38]

Thanks for having a look!

https://codereview.chromium.org/11829032/diff/9012/base/metrics/histogram.h
File base/metrics/histogram.h (right):

https://codereview.chromium.org/11829032/diff/9012/base/metrics/histogram.h#n...
base/metrics/histogram.h:220: #define HISTOGRAM_MEMORY_KB(name, sample)
HISTOGRAM_CUSTOM_COUNTS( \
On 2013/01/14 19:41:18, jar wrote:
> Do you deliberately want a histogram that is not going to be uploaded to UMA,
as
> compared with what is on line 328?

Yes, we deliberately want that. The histogram is only created if a command line
switch is present, and that switch is only present in tests. So it doesn't make
sense to upload it to UMA.

[Charlie Reis, 2013-01-14 19:59:31]

Glad to see you're going with an approach behind a flag.  LGTM with nits, but
you'll want an owner of webkit/glue to give a careful look at the memory
collection logic.

https://codereview.chromium.org/11829032/diff/9012/content/public/common/cont...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/11829032/diff/9012/content/public/common/cont...
content/public/common/content_switches.cc:486: // Retrieve memory metrics.
Can you say a little more about what this means?

https://codereview.chromium.org/11829032/diff/9012/webkit/glue/memory_usage.h
File webkit/glue/memory_usage.h (right):

https://codereview.chromium.org/11829032/diff/9012/webkit/glue/memory_usage.h...
webkit/glue/memory_usage.h:14: WEBKIT_GLUE_EXPORT size_t memoryUsageKB();
Presumably you're accounting for the fact that it's a different measurement on
each platform.  Maybe it's worth documenting here what this value is
specifically returning for each platform?

[marja, 2013-01-14 20:17:01]

creis@: Thanks for review!

tony@: As an OWNER of webkit/, could you review?

https://codereview.chromium.org/11829032/diff/9012/content/public/common/cont...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/11829032/diff/9012/content/public/common/cont...
content/public/common/content_switches.cc:486: // Retrieve memory metrics.
On 2013/01/14 19:59:31, creis wrote:
> Can you say a little more about what this means?

Added:
// Sample memory usage with high frequency and store the results to the
// Renderer.Memory histogram. Used in memory tests.

https://codereview.chromium.org/11829032/diff/9012/webkit/glue/memory_usage.h
File webkit/glue/memory_usage.h (right):

https://codereview.chromium.org/11829032/diff/9012/webkit/glue/memory_usage.h...
webkit/glue/memory_usage.h:14: WEBKIT_GLUE_EXPORT size_t memoryUsageKB();
On 2013/01/14 19:59:31, creis wrote:
> Presumably you're accounting for the fact that it's a different measurement on
> each platform.  Maybe it's worth documenting here what this value is
> specifically returning for each platform?

Yes, we're not comparing these values across platforms. Added here:
// Returns an estimate of the memory usage of the renderer process. Different
// platforms implement this function differently, and count in different
// allocations. Results are not comparable across platforms. The estimate is
// computed inside the sandbox and thus its not always accurate.

[tony, 2013-01-14 20:28:40]

webkit LGTM.  I would probably put the new function in webkit_glue.{h,cc} rather
than making a new file, but this is OK too.

[marja, 2013-01-14 20:40:00]

Thanks for review!

I moved the code to webkit_glue.{cc,h}.

[commit-bot: I haz the power, 2013-01-14 20:41:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/11829032/23001

[commit-bot: I haz the power, 2013-01-14 20:41:23]

Presubmit check for 11829032-23001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    base/metrics

Presubmit checks took 3.5s to calculate.

[jar (doing other things), 2013-01-14 21:54:23]

Change to base LGTM

[commit-bot: I haz the power, 2013-01-14 22:11:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/11829032/23001

[commit-bot: I haz the power, 2013-01-15 00:05:36]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
cacheinvalidation_unittests, check_deps, chrome_frame_net_tests,
chrome_frame_unittests, chromedriver2_unittests, content_browsertests,
content_unittests, crypto_unittests, gpu_unittests, installer_util_unittests,
interactive_ui_tests, ipc_tests, jingle_unittests, media_unittests,
mini_installer_test, nacl_integration, net_unittests, ppapi_unittests,
printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests,
sync_unit_tests, unit_tests

[commit-bot: I haz the power, 2013-01-15 09:33:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/11829032/22012

[commit-bot: I haz the power, 2013-01-15 09:59:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/11829032/21021

[commit-bot: I haz the power, 2013-01-15 13:22:59]

Change committed as 176894