[Memory] Add system_memory_stats_recorder_win.cc

[Memory] Add system_memory_stats_recorder_win.cc

This adds stats recording on Windows when discarding a tab.

Note: This is CL #6 towards expanding Chromeos tab killing to other platforms.

BUG=463597
Committed: https://crrev.com/b7adc5b57caeea0033b2757d5babc03df73f9bb9
Cr-Commit-Position: refs/heads/master@{#337490}

[Georges Khalil, 2015-06-30 17:07:54]

Patchset #1 (id:1) has been deleted

[Georges Khalil, 2015-06-30 17:08:56]

georgesak@chromium.org changed reviewers:
+ asvitkine@chromium.org, sky@chromium.org

[Georges Khalil, 2015-06-30 17:08:57]

asvitkine@chromium.org: Please review changes in histograms.xml

sky@chromium.org: Please review changes in chrome/*

[chrisha, 2015-06-30 17:11:18]

chrisha@chromium.org changed reviewers:
+ chrisha@chromium.org

[chrisha, 2015-06-30 17:11:18]

Do we really need OS specific metrics in histograms? We can always filter by OS
only from the actual dashboard.

Maybe prefer to deprecate the *.ChromeOS.* specific histograms, and then
introduce new ones that are across all platforms?

[Georges Khalil, 2015-06-30 17:16:27]

georgesak@chromium.org changed reviewers:
+ rkaplow@chromium.org
- asvitkine@chromium.org

[Georges Khalil, 2015-06-30 17:16:28]

-asvitkine@ (OOO)
+rkaplow@

[Georges Khalil, 2015-06-30 17:27:30]

On 2015/06/30 17:11:18, chrisha wrote:
> Do we really need OS specific metrics in histograms? We can always filter by
OS
> only from the actual dashboard.
> 
> Maybe prefer to deprecate the *.ChromeOS.* specific histograms, and then
> introduce new ones that are across all platforms?

Currently, we have the following metrics:
- MemGraphicsMB & MemShmemMB which are CrOS only
- MemAllocatedMB & MemAvailableMB which are Linux & CrOS

My goal was to get more metrics for Windows, so we can get a good idea of how
well tab killing works. The other option would be to reuse the last 2 metrics
and only report the physical memory usage. But I think having the full set of
metrics could be more useful to us down the road.

[rkaplow, 2015-06-30 18:21:48]

rkaplow@chromium.org changed reviewers:
+ isherman@chromium.org

[rkaplow, 2015-06-30 18:21:48]

Hey Ilya, curious how you think they should suffix this setup, considering there
is 2 dimensions of splits. Does it make more sense to you to split it one way
versus another way? Is it possible to split both, or is that too complex?

https://codereview.chromium.org/1214803006/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1214803006/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:16682: +<histogram
name="Memory.OOMKill.Extensions.Win.AvailPageFile" units="MB">
i wonder if it makes sense to change these to 
Memory.OOMKill.Win.*.Extension
Memory.OOMKill.Win.*.Browser (or nonextension, or something)


You can then use the suffix notation (see docs near top of file) to make this
much more compact, but using Extension and Browser/nonextension as suffixed.


Another possibility is to use the
AvailPhys / AvailVirtual ... as the suffixes. That might even make more sense
since then it works for the Discard histogram. This is an interesting situation
with a MxN setup with suffixes, I've not seen that.

[Ilya Sherman, 2015-07-01 02:25:14]

https://codereview.chromium.org/1214803006/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1214803006/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:16682: +<histogram
name="Memory.OOMKill.Extensions.Win.AvailPageFile" units="MB">
On 2015/06/30 18:21:48, rkaplow wrote:
> i wonder if it makes sense to change these to 
> Memory.OOMKill.Win.*.Extension
> Memory.OOMKill.Win.*.Browser (or nonextension, or something)
> 
> 
> You can then use the suffix notation (see docs near top of file) to make this
> much more compact, but using Extension and Browser/nonextension as suffixed.
> 
> 
> Another possibility is to use the
> AvailPhys / AvailVirtual ... as the suffixes. That might even make more sense
> since then it works for the Discard histogram. This is an interesting
situation
> with a MxN setup with suffixes, I've not seen that.

MxN suffixes already exist in the file -- they're just not obvious. 
histogram_suffixes can list affected histogram names that come from a different
histogram_suffixes element.  There's some limit to the amount of nesting
allowed, but it's at least two levels.

[Ilya Sherman, 2015-07-01 02:28:59]

https://codereview.chromium.org/1214803006/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1214803006/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:16682: +<histogram
name="Memory.OOMKill.Extensions.Win.AvailPageFile" units="MB">
On 2015/07/01 02:25:14, Ilya Sherman wrote:
> On 2015/06/30 18:21:48, rkaplow wrote:
> > i wonder if it makes sense to change these to 
> > Memory.OOMKill.Win.*.Extension
> > Memory.OOMKill.Win.*.Browser (or nonextension, or something)
> > 
> > 
> > You can then use the suffix notation (see docs near top of file) to make
this
> > much more compact, but using Extension and Browser/nonextension as suffixed.
> > 
> > 
> > Another possibility is to use the
> > AvailPhys / AvailVirtual ... as the suffixes. That might even make more
sense
> > since then it works for the Discard histogram. This is an interesting
> situation
> > with a MxN setup with suffixes, I've not seen that.
> 
> MxN suffixes already exist in the file -- they're just not obvious. 
> histogram_suffixes can list affected histogram names that come from a
different
> histogram_suffixes element.  There's some limit to the amount of nesting
> allowed, but it's at least two levels.

I do agree, FWIW, that it probably makes sense to move "Win" to precede
"Extensions" -- I'd make the order "Memory.OOMKill.Win.Extensions.AvailPhys". 
IMO this would do the best job of grouping things together based on what's most
directly comparable.  Alternately, if these histograms *could* be implemented on
non-Windows machines -- i.e. if the memory models are compatible -- then I'd
just omit "Win" entirely.  It would be clear from the dashboard that these are
only implemented on Windows.

[Georges Khalil, 2015-07-06 17:35:16]

Patchset #2 (id:40001) has been deleted

[Georges Khalil, 2015-07-06 17:40:01]

Patchset #2 (id:60001) has been deleted

[Georges Khalil, 2015-07-06 17:47:32]

georgesak@chromium.org changed reviewers:
+ jamescook@chromium.org
- sky@chromium.org

[Georges Khalil, 2015-07-06 17:47:32]

PTAnL.

Changed the naming scheme to use suffixes instead.

Also, -sky@ (OOO) and +jamescook@ for chrome/*

https://codereview.chromium.org/1214803006/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1214803006/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:16682: +<histogram
name="Memory.OOMKill.Extensions.Win.AvailPageFile" units="MB">
On 2015/07/01 02:28:59, Ilya Sherman wrote:
> On 2015/07/01 02:25:14, Ilya Sherman wrote:
> > On 2015/06/30 18:21:48, rkaplow wrote:
> > > i wonder if it makes sense to change these to 
> > > Memory.OOMKill.Win.*.Extension
> > > Memory.OOMKill.Win.*.Browser (or nonextension, or something)
> > > 
> > > 
> > > You can then use the suffix notation (see docs near top of file) to make
> this
> > > much more compact, but using Extension and Browser/nonextension as
suffixed.
> > > 
> > > 
> > > Another possibility is to use the
> > > AvailPhys / AvailVirtual ... as the suffixes. That might even make more
> sense
> > > since then it works for the Discard histogram. This is an interesting
> > situation
> > > with a MxN setup with suffixes, I've not seen that.
> > 
> > MxN suffixes already exist in the file -- they're just not obvious. 
> > histogram_suffixes can list affected histogram names that come from a
> different
> > histogram_suffixes element.  There's some limit to the amount of nesting
> > allowed, but it's at least two levels.
> 
> I do agree, FWIW, that it probably makes sense to move "Win" to precede
> "Extensions" -- I'd make the order "Memory.OOMKill.Win.Extensions.AvailPhys". 
> IMO this would do the best job of grouping things together based on what's
most
> directly comparable.  Alternately, if these histograms *could* be implemented
on
> non-Windows machines -- i.e. if the memory models are compatible -- then I'd
> just omit "Win" entirely.  It would be clear from the dashboard that these are
> only implemented on Windows.

Memory models are not compatible, which is why we have platform specific
histograms.

Changed naming to use suffixes.

[James Cook, 2015-07-06 18:56:35]

https://codereview.chromium.org/1214803006/diff/80001/chrome/browser/memory/s...
File chrome/browser/memory/system_memory_stats_recorder_win.cc (right):

https://codereview.chromium.org/1214803006/diff/80001/chrome/browser/memory/s...
chrome/browser/memory/system_memory_stats_recorder_win.cc:15: #define
UMA_HISTOGRAM_LARGE_MEMORY_MB(name, sample) \
Do you want to move this macro to a header so it can be shared with the similar
one in system_memory_stats_recorder_linux.cc?

https://codereview.chromium.org/1214803006/diff/80001/chrome/browser/memory/s...
chrome/browser/memory/system_memory_stats_recorder_win.cc:26:
UMA_HISTOGRAM_CUSTOM_COUNTS("Memory.Stats.Win.MemoryLoad.TabDiscarded",
The existing tab discarder stats all start with the prefix "Tabs.Discard".
Existing OOM kill stats start with "Memory.OOMKill". Introducing another prefix
seems bad to me.

I would prefer either all the tab discard ones start with "Tabs.Discard", or if
you introduce a new convention, rename/deprecate the existing Tabs.Discard stats
to use the same prefix.

Otherwise it's going to be really hard for people to find where these stats live
across platforms.

https://codereview.chromium.org/1214803006/diff/80001/chrome/browser/memory/s...
chrome/browser/memory/system_memory_stats_recorder_win.cc:46: case
RECORD_MEMORY_STATS_CONTENTS_OOM_KILLED: {
Does this ever actually happen on Windows?

https://codereview.chromium.org/1214803006/diff/80001/chrome/browser/memory/s...
chrome/browser/memory/system_memory_stats_recorder_win.cc:63: case
RECORD_MEMORY_STATS_EXTENSIONS_OOM_KILLED: {
ditto

[Georges Khalil, 2015-07-06 20:01:34]

jamescook@, PTAnL.

https://codereview.chromium.org/1214803006/diff/80001/chrome/browser/memory/s...
File chrome/browser/memory/system_memory_stats_recorder_win.cc (right):

https://codereview.chromium.org/1214803006/diff/80001/chrome/browser/memory/s...
chrome/browser/memory/system_memory_stats_recorder_win.cc:15: #define
UMA_HISTOGRAM_LARGE_MEMORY_MB(name, sample) \
On 2015/07/06 18:56:34, James Cook wrote:
> Do you want to move this macro to a header so it can be shared with the
similar
> one in system_memory_stats_recorder_linux.cc?

Done.

https://codereview.chromium.org/1214803006/diff/80001/chrome/browser/memory/s...
chrome/browser/memory/system_memory_stats_recorder_win.cc:26:
UMA_HISTOGRAM_CUSTOM_COUNTS("Memory.Stats.Win.MemoryLoad.TabDiscarded",
On 2015/07/06 18:56:35, James Cook wrote:
> The existing tab discarder stats all start with the prefix "Tabs.Discard".
> Existing OOM kill stats start with "Memory.OOMKill". Introducing another
prefix
> seems bad to me.
> 
> I would prefer either all the tab discard ones start with "Tabs.Discard", or
if
> you introduce a new convention, rename/deprecate the existing Tabs.Discard
stats
> to use the same prefix.
> 
> Otherwise it's going to be really hard for people to find where these stats
live
> across platforms.

Yes agreed. Right now, they either start with Tabs.Discard, Memory.OOMKill or
Memory.OOMKill.Extensions.

The goal is to unify these to a better naming scheme, but in a subsequent CL.
Had an offline chat with rkaplow@ and that's the route we'll take. We'll
deprecate the old ones and have new ones that are consistent with the naming
scheme. I propose they all start with Memory.Stats. That way, they are also
grouped together in the xml and we'll use suffixes to reduce duplication.

https://codereview.chromium.org/1214803006/diff/80001/chrome/browser/memory/s...
chrome/browser/memory/system_memory_stats_recorder_win.cc:46: case
RECORD_MEMORY_STATS_CONTENTS_OOM_KILLED: {
On 2015/07/06 18:56:34, James Cook wrote:
> Does this ever actually happen on Windows?

Damn, you're right, this is strictly CrOS. Completely missed that.

That simplifies the histograms, no need for suffixes anymore for these metrics.

https://codereview.chromium.org/1214803006/diff/80001/chrome/browser/memory/s...
chrome/browser/memory/system_memory_stats_recorder_win.cc:63: case
RECORD_MEMORY_STATS_EXTENSIONS_OOM_KILLED: {
On 2015/07/06 18:56:35, James Cook wrote:
> ditto

Acknowledged.

[James Cook, 2015-07-06 20:16:02]

LGTM but please CC me on the change to normalize the histogram names. Memory.*
sounds good to me.

[Georges Khalil, 2015-07-06 20:31:40]

On 2015/07/06 20:16:02, James Cook wrote:
> LGTM but please CC me on the change to normalize the histogram names. Memory.*
> sounds good to me.

Will do. I had an issue with the gyp file while merging, I just fixed it (after
your l-g-t-m).

rkaplow@, care to take a look at histograms.xml? Thanks.

[rkaplow, 2015-07-06 21:22:14]

lgtm

LGTM I think but will still need an owner.

DO have a question though - so these are all the base histograms and they will
be suffixed with the OOM.Extension  / Discard, all that stuff, right?

[Georges Khalil, 2015-07-06 21:28:21]

On 2015/07/06 21:22:14, rkaplow wrote:
> lgtm
> 
> LGTM I think but will still need an owner.
> 
> DO have a question though - so these are all the base histograms and they will
> be suffixed with the OOM.Extension  / Discard, all that stuff, right?

The windows one do not need any suffix, as they can only happen when there is a
discard. The linux/chromeos ones will have suffixes to indicate the type of
event.

Makes sense?

[Alexei Svitkine (slow), 2015-07-06 21:37:03]

asvitkine@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2015-07-06 21:37:03]

lgtm

[Georges Khalil, 2015-07-06 21:38:08]

The CQ bit was checked by georgesak@chromium.org

[Georges Khalil, 2015-07-06 21:38:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/1214803006/#ps140001
(title: "Fix merge issue in gyp and line wrapping.")

[commit-bot: I haz the power, 2015-07-06 21:39:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1214803006/140001

[commit-bot: I haz the power, 2015-07-06 22:12:24]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2015-07-06 22:13:22]

Patchset 5 (id:??) landed as
https://crrev.com/b7adc5b57caeea0033b2757d5babc03df73f9bb9
Cr-Commit-Position: refs/heads/master@{#337490}