Handle failures allocating mapped local memory.

Handle failures allocating mapped local memory.

BUG=635993
Committed: https://crrev.com/cd4923db62f029122ef81b54d03aa12343846c02
Cr-Commit-Position: refs/heads/master@{#420671}

[bcwhite, 2016-09-22 14:39:38]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 14:39:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 15:06:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-22 15:06:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bcwhite, 2016-09-22 15:27:33]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 15:28:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 16:21:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-22 16:21:44]

Dry run: This issue passed the CQ dry run.

[bcwhite, 2016-09-22 16:50:57]

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

[Alexei Svitkine (slow), 2016-09-22 16:57:13]

https://codereview.chromium.org/2362713002/diff/20001/base/metrics/persistent...
File base/metrics/persistent_memory_allocator.cc (right):

https://codereview.chromium.org/2362713002/diff/20001/base/metrics/persistent...
base/metrics/persistent_memory_allocator.cc:775: // to the process now istead of
when first accessed).
Can you add a histogram to track how often this happens?

https://codereview.chromium.org/2362713002/diff/20001/base/metrics/persistent...
base/metrics/persistent_memory_allocator.cc:778: ::memset(address, 0, size);
Nit: I don't think it's general convention to prefix non-Chrome code with ::
unless there's conflicts.

https://codereview.chromium.org/2362713002/diff/20001/base/metrics/persistent...
base/metrics/persistent_memory_allocator.cc:795: // Must have been allocated by
fallback allocator.
I don't think this is a safe assumption - e.g. if the above fails for some other
reason. I would add a boolean to explicitly track this.

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

https://codereview.chromium.org/2362713002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:63490: +<histogram
name="UMA.LocalPersistentMemoryAllocator.Failures.Win">
Please associate both of these with enums - I believe we should already have
some defined for GetLastError and errno, but you might have to look.

[bcwhite, 2016-09-22 17:25:11]

Patchset #3 (id:40001) has been deleted

[bcwhite, 2016-09-22 17:25:59]

https://codereview.chromium.org/2362713002/diff/20001/base/metrics/persistent...
File base/metrics/persistent_memory_allocator.cc (right):

https://codereview.chromium.org/2362713002/diff/20001/base/metrics/persistent...
base/metrics/persistent_memory_allocator.cc:775: // to the process now istead of
when first accessed).
On 2016/09/22 16:57:12, Alexei Svitkine (very slow) wrote:
> Can you add a histogram to track how often this happens?

I tried that (see patch #1) but it means creating a histogram every time one of
these gets created and since these are used for testing, it messes with the
expected results of a dozen histogram tests.

https://codereview.chromium.org/2362713002/diff/20001/base/metrics/persistent...
base/metrics/persistent_memory_allocator.cc:778: ::memset(address, 0, size);
On 2016/09/22 16:57:12, Alexei Svitkine (very slow) wrote:
> Nit: I don't think it's general convention to prefix non-Chrome code with ::
> unless there's conflicts.

Done.

https://codereview.chromium.org/2362713002/diff/20001/base/metrics/persistent...
base/metrics/persistent_memory_allocator.cc:795: // Must have been allocated by
fallback allocator.
On 2016/09/22 16:57:12, Alexei Svitkine (very slow) wrote:
> I don't think this is a safe assumption - e.g. if the above fails for some
other
> reason. I would add a boolean to explicitly track this.

I'd like to but can't because there is no place to store it. This method is
static because it's used to determine a construction parameter to the base class
meaning no way to safely write to member variables.

According to the man page, the only reason this call should fail is EINVAL which
is the case we're looking at.

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

https://codereview.chromium.org/2362713002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:63490: +<histogram
name="UMA.LocalPersistentMemoryAllocator.Failures.Win">
On 2016/09/22 16:57:13, Alexei Svitkine (very slow) wrote:
> Please associate both of these with enums - I believe we should already have
> some defined for GetLastError and errno, but you might have to look.

Cool.  I didn't know about those.

[bcwhite, 2016-09-22 19:50:45]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 19:51:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2016-09-22 19:57:38]

As discussed, new patch that passes Memory structure with both address and type
that can be used to properly release the memory upon destruction.

[Alexei Svitkine (slow), 2016-09-22 20:23:54]

Looks good. Just a comment about histograms.xml.

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

https://codereview.chromium.org/2362713002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:63490: +<histogram
name="UMA.LocalPersistentMemoryAllocator.Failures.Win">
On 2016/09/22 17:25:59, bcwhite wrote:
> On 2016/09/22 16:57:13, Alexei Svitkine (very slow) wrote:
> > Please associate both of these with enums - I believe we should already have
> > some defined for GetLastError and errno, but you might have to look.
> 
> Cool.  I didn't know about those.

Hmm, actually units="GetLastError" doesn't do anything. I checked and I guess
I'm wrong and we didn't have an enum for these.

However, I think defining it is the right thing to do. Please add it. You don't
have to fully fill it in for now, but it could be expanded over time for the
ones we see in the data on the dashboards.

For the posix errno, it seems like we do have an enum, however - OSAgnosticErrno
- so please use that. :)

Could you also switch all existing cases of units="GetLastError" and
units="errno" to these enum="" entries? Thanks!

[commit-bot: I haz the power, 2016-09-22 21:03:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-22 21:03:21]

Dry run: This issue passed the CQ dry run.

[bcwhite, 2016-09-22 23:35:30]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 23:36:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2016-09-22 23:40:29]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[bcwhite, 2016-09-22 23:40:53]

Patchset #5 (id:100001) has been deleted

[commit-bot: I haz the power, 2016-09-22 23:40:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2016-09-22 23:41:46]

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

https://codereview.chromium.org/2362713002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:63490: +<histogram
name="UMA.LocalPersistentMemoryAllocator.Failures.Win">
> Hmm, actually units="GetLastError" doesn't do anything. I checked and I guess
> I'm wrong and we didn't have an enum for these.

It was used in a number of places, too.


> However, I think defining it is the right thing to do. Please add it. You
don't
> have to fully fill it in for now, but it could be expanded over time for the
> ones we see in the data on the dashboards.

Done.  I added all entries for codes 0-999.  It's easy enough to add them all
but there's at least a few thousand more defined by Microsoft.
https://msdn.microsoft.com/en-us/library/windows/desktop/ms681382(v=vs.85).aspx


> For the posix errno, it seems like we do have an enum, however -
OSAgnosticErrno
> - so please use that. :)

Done.


> Could you also switch all existing cases of units="GetLastError" and
> units="errno" to these enum="" entries? Thanks!

Done.

[commit-bot: I haz the power, 2016-09-23 00:34:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-23 00:34:01]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2016-09-23 13:50:53]

https://codereview.chromium.org/2362713002/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2362713002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:23269: +<histogram
name="Media.Initialize.Windows" units="WinGetLastError">
Change units= to enum=

Same for other ones below.

https://codereview.chromium.org/2362713002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:63483: +    units="OSAgnosticErrno">
This should be enum="OsAgnosticErrno", not units=

units="foo" is used just used as a string label for things like Y axis on a
graph, whereas enum actually changed the dashboards to treat the values as an
enumeration and being able to graph individual buckets, etc.

Please also change other places, including some places that currently do
units="errno" to enum="OSAgnosticErrno"

[bcwhite, 2016-09-23 14:06:03]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 14:06:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2016-09-23 14:08:20]

https://codereview.chromium.org/2362713002/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2362713002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:23269: +<histogram
name="Media.Initialize.Windows" units="WinGetLastError">
On 2016/09/23 13:50:53, Alexei Svitkine (very slow) wrote:
> Change units= to enum=
> 
> Same for other ones below.

OH!  You know, I completely missed that "units" != "enum" both in the file and
in your comments.  :-D

https://codereview.chromium.org/2362713002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:63483: +    units="OSAgnosticErrno">
On 2016/09/23 13:50:53, Alexei Svitkine (very slow) wrote:
> This should be enum="OsAgnosticErrno", not units=
> 
> units="foo" is used just used as a string label for things like Y axis on a
> graph, whereas enum actually changed the dashboards to treat the values as an
> enumeration and being able to graph individual buckets, etc.
> 
> Please also change other places, including some places that currently do
> units="errno" to enum="OSAgnosticErrno"

Done.

[Alexei Svitkine (slow), 2016-09-23 14:16:30]

lgtm

[bcwhite, 2016-09-23 14:20:20]

The CQ bit was unchecked by bcwhite@chromium.org

[bcwhite, 2016-09-23 14:20:24]

The CQ bit was checked by bcwhite@chromium.org

[commit-bot: I haz the power, 2016-09-23 14:20:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 14:27:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-23 14:27:50]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[bcwhite, 2016-09-23 14:38:01]

The CQ bit was checked by bcwhite@chromium.org

[commit-bot: I haz the power, 2016-09-23 14:38:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 14:56:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-23 14:56:51]

Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[bcwhite, 2016-09-23 17:14:31]

The CQ bit was checked by bcwhite@chromium.org

[commit-bot: I haz the power, 2016-09-23 17:15:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 18:30:26]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-09-23 18:32:24]

Description was changed from

==========
Handle failures allocating mapped local memory.

BUG=635993
==========

to

==========
Handle failures allocating mapped local memory.

BUG=635993

Committed: https://crrev.com/cd4923db62f029122ef81b54d03aa12343846c02
Cr-Commit-Position: refs/heads/master@{#420671}
==========

[commit-bot: I haz the power, 2016-09-23 18:32:25]

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