[WIP] Add SharedMemoryTracker to dump base::SharedMemory usage

[WIP] Add SharedMemoryTracker to dump base::SharedMemory usage

This CL adds SharedMemoryTracker to dump base::SharedMemory usage.

Remaining tasks:
* Confirm there is no regression in terms of performance
* Add tests

BUG=604726
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

[hajimehoshi, 2016-11-29 11:53:06]

Description was changed from

==========
[WIP] Add SharedMemoryDumpProfiler to dump base::SharedMemory usage

This CL adds SharedMemoryDumpProfiler to dump base::SharedMemory usage.

Remaining tasks:
* Call Increment/DecrementMemoryUsage on other platforms than POSIX
* Confirm there is no regression in terms of performance
* Add tests

BUG=604726
==========

to

==========
[WIP] Add SharedMemoryDumpProfiler to dump base::SharedMemory usage

This CL adds SharedMemoryDumpProfiler to dump base::SharedMemory usage.

Remaining tasks:
* Call Increment/DecrementMemoryUsage on other platforms than POSIX
* Confirm there is no regression in terms of performance
* Add tests

BUG=604726
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[hajimehoshi, 2016-11-29 11:55:09]

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

[commit-bot: I haz the power, 2016-11-29 11:55:26]

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

[hajimehoshi, 2016-11-29 11:56:14]

hajimehoshi@chromium.org changed reviewers:
+ bashi@chromium.org, haraken@chromium.org, primiano@chromium.org

[hajimehoshi, 2016-11-29 11:59:08]

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

[commit-bot: I haz the power, 2016-11-29 11:59:22]

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-11-29 12:24:18]

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

[commit-bot: I haz the power, 2016-11-29 12:24:19]

Dry run: Try jobs failed on following builders:
  win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_...)

[haraken, 2016-11-30 04:39:26]

haraken@chromium.org changed reviewers:
+ reveman@chromium.org

[haraken, 2016-11-30 04:39:27]

How did you measure the performance impact of this CL?

reveman@: Would you take a brief look at if this approach is okay?

https://codereview.chromium.org/2535213002/diff/20001/base/memory/shared_memo...
File base/memory/shared_memory_posix.cc (right):

https://codereview.chromium.org/2535213002/diff/20001/base/memory/shared_memo...
base/memory/shared_memory_posix.cc:362:
SharedMemoryDumpProvider::GetInstance()->IncrementMemoryUsage(mapped_size_);

We want to get not only the mmaped size but also the effective size. Is there
any way to get an approximate number for the effective size?

[hajimehoshi, 2016-11-30 07:06:47]

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

[commit-bot: I haz the power, 2016-11-30 07:07:29]

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

[hajimehoshi, 2016-11-30 07:15:04]

On 2016/11/30 04:39:27, haraken wrote:
> How did you measure the performance impact of this CL?

I'm thinking of telemetry. To be honest, I'm not sure what tests are suitable to
do this, and what number we should care...

> 
> reveman@: Would you take a brief look at if this approach is okay?
> 
>
https://codereview.chromium.org/2535213002/diff/20001/base/memory/shared_memo...
> File base/memory/shared_memory_posix.cc (right):
> 
>
https://codereview.chromium.org/2535213002/diff/20001/base/memory/shared_memo...
> base/memory/shared_memory_posix.cc:362:
> SharedMemoryDumpProvider::GetInstance()->IncrementMemoryUsage(mapped_size_);
> 
> We want to get not only the mmaped size but also the effective size. Is there
> any way to get an approximate number for the effective size?

[hajimehoshi, 2016-11-30 07:15:18]

Description was changed from

==========
[WIP] Add SharedMemoryDumpProfiler to dump base::SharedMemory usage

This CL adds SharedMemoryDumpProfiler to dump base::SharedMemory usage.

Remaining tasks:
* Call Increment/DecrementMemoryUsage on other platforms than POSIX
* Confirm there is no regression in terms of performance
* Add tests

BUG=604726
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
[WIP] Add SharedMemoryDumpProfiler to dump base::SharedMemory usage

This CL adds SharedMemoryDumpProfiler to dump base::SharedMemory usage.

Remaining tasks:
* Confirm there is no regression in terms of performance
* Add tests

BUG=604726
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[hajimehoshi, 2016-11-30 07:26:23]

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

[commit-bot: I haz the power, 2016-11-30 07:26:56]

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-11-30 07:50:25]

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

[commit-bot: I haz the power, 2016-11-30 07:50:26]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[reveman, 2016-11-30 20:39:55]

On 2016/11/30 at 04:39:27, haraken wrote:
> How did you measure the performance impact of this CL?
> 
> reveman@: Would you take a brief look at if this approach is okay?

ui/gl/gl_image_shared_memory.cc and
components/discardable_memory/common/discardable_shared_memory_heap.cc lgtm

> 
>
https://codereview.chromium.org/2535213002/diff/20001/base/memory/shared_memo...
> File base/memory/shared_memory_posix.cc (right):
> 
>
https://codereview.chromium.org/2535213002/diff/20001/base/memory/shared_memo...
> base/memory/shared_memory_posix.cc:362:
SharedMemoryDumpProvider::GetInstance()->IncrementMemoryUsage(mapped_size_);
> 
> We want to get not only the mmaped size but also the effective size. Is there
any way to get an approximate number for the effective size?

[hajimehoshi, 2016-12-01 06:19:30]

On 2016/11/30 07:15:04, hajimehoshi wrote:
> On 2016/11/30 04:39:27, haraken wrote:
> > How did you measure the performance impact of this CL?
> 
> I'm thinking of telemetry. To be honest, I'm not sure what tests are suitable
to
> do this, and what number we should care...

primiano@, reveman@: Do you have any suggestion how to check a performance
regression?

> 
> > 
> > reveman@: Would you take a brief look at if this approach is okay?
> > 
> >
>
https://codereview.chromium.org/2535213002/diff/20001/base/memory/shared_memo...
> > File base/memory/shared_memory_posix.cc (right):
> > 
> >
>
https://codereview.chromium.org/2535213002/diff/20001/base/memory/shared_memo...
> > base/memory/shared_memory_posix.cc:362:
> > SharedMemoryDumpProvider::GetInstance()->IncrementMemoryUsage(mapped_size_);
> > 
> > We want to get not only the mmaped size but also the effective size. Is
there
> > any way to get an approximate number for the effective size?

[hajimehoshi, 2016-12-02 06:41:30]

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

[commit-bot: I haz the power, 2016-12-02 06:41:44]

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-12-02 07:54:30]

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

[commit-bot: I haz the power, 2016-12-02 07:54:31]

Dry run: This issue passed the CQ dry run.

[Primiano Tucci (use gerrit), 2016-12-02 10:56:20]

Left some comments on crbug.com/604726

TL;DR
1) I think you want to count only anonymous shared memory here and not file
mapped
2) You need some kind of unique identifier for each mapping, which matches both
sides of the IPC, to avoid double-counting the same memory on both sides.

On 2016/12/01 06:19:30, hajimehoshi wrote:
> On 2016/11/30 07:15:04, hajimehoshi wrote:
> > On 2016/11/30 04:39:27, haraken wrote:
> > > How did you measure the performance impact of this CL?
> > 
> > I'm thinking of telemetry. To be honest, I'm not sure what tests are
suitable
> to
> > do this, and what number we should care...
> 
> primiano@, reveman@: Do you have any suggestion how to check a performance
> regression?
> 
> > 
> > > 
> > > reveman@: Would you take a brief look at if this approach is okay?
> > > 
> > >
> >
>
https://codereview.chromium.org/2535213002/diff/20001/base/memory/shared_memo...
> > > File base/memory/shared_memory_posix.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2535213002/diff/20001/base/memory/shared_memo...
> > > base/memory/shared_memory_posix.cc:362:
> > >
SharedMemoryDumpProvider::GetInstance()->IncrementMemoryUsage(mapped_size_);
> > > 
> > > We want to get not only the mmaped size but also the effective size. Is
> there
> > > any way to get an approximate number for the effective size?

[hajimehoshi, 2016-12-20 09:18:56]

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

[commit-bot: I haz the power, 2016-12-20 09:19:15]

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-12-20 09:26:29]

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

[commit-bot: I haz the power, 2016-12-20 09:26:30]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[hajimehoshi, 2016-12-20 09:37:51]

primiano@: This is not finished yet (I need to implement
GetIDFromSharedMemoryHandle on other platforms than POSIX), but I'd like you to
check if the direction is OK. PTAL

[hajimehoshi, 2016-12-21 12:15:54]

ping

[Primiano Tucci (use gerrit), 2016-12-21 13:32:37]

On 2016/12/21 12:15:54, hajimehoshi wrote:
> ping

Do you have a trace to see how the final result looks like?

[Primiano Tucci (use gerrit), 2016-12-21 15:06:41]

Had a first high-level pass, here's some initial comments.
I think overall is shooting in the right direction but we might need some
iterations on this to put it in shape. Thanks for working on this.



1) I think SharedMemoryDumpProvider is both a bit of a confusing name and
misplaced.
I'd name it SharedMemoryTracker and put under base/trace_event

2) The fact that all those shared memory allocations are serialized under a lock
makes me a bit nervous performance-wise. Did you manage to measure the impact of
this? Perhaps a simple and very effective approach would be:
Instead of having one big lock around the memory_usage_ field you could have
a fixed-size array where usages are indexed by either id or mapped address
(essetially a hashtable) where the lock is on each bucket of the array.
In essence something like:
struct TrackerBucket {
  base::Lock lock;
  std::unordered_map<Id, size_t> usage;
}

TrackerBucket buckets_[N];
Essentially before inserting and removing you'd hash the address/id.
This would greatly reduce the collision domain on the lock and 
make it practically lockless, given a reasonable hash and N.

3) I see that you stringprintf upfront. Would be nicer if we could just
store whatever numeric IDs you need for the handle and stringify them only
at dump time. So we avoid a string operation evey time a shared memory handle
is created and destroyed.

4) I see that a SharedMemory handle can be mapped or not. Are you dumping
all of them regardless, or only if mapped? If the latter you can use the
mapping address as hashing key (otherwise hash the IDs).

 
5) I think might be easier if this CL gets split into the //base part that
introduces the various utilities in memory-infra and such, and the changes
to the clients.

6) Can I see a trace which shows the final effect in memory infra about this
(even if is not 100% complete and is missing some details is fine)?

[hajimehoshi, 2017-01-10 09:54:01]

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

[commit-bot: I haz the power, 2017-01-10 09:54:16]

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, 2017-01-10 10:04:27]

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

[commit-bot: I haz the power, 2017-01-10 10:04:28]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[hajimehoshi, 2017-01-10 11:34:55]

On 2016/12/21 13:32:37, Primiano Tucci (back but slow) wrote:
> On 2016/12/21 12:15:54, hajimehoshi wrote:
> > ping
> 
> Do you have a trace to see how the final result looks like?

https://drive.google.com/file/d/0BwW8PrCcts4WZUdMSThBdlFxMEk/view?usp=sharing
The result is shown on memory-infra as 'sharedmemory’.

[hajimehoshi, 2017-01-11 07:27:37]

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

[commit-bot: I haz the power, 2017-01-11 07:28:03]

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, 2017-01-11 07:38:55]

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

[commit-bot: I haz the power, 2017-01-11 07:38:56]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[hajimehoshi, 2017-01-11 09:13:23]

Patchset #8 (id:140001) has been deleted

[hajimehoshi, 2017-01-11 09:13:28]

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

[commit-bot: I haz the power, 2017-01-11 09:14:03]

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, 2017-01-11 09:21:14]

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

[commit-bot: I haz the power, 2017-01-11 09:21:15]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[hajimehoshi, 2017-01-11 09:28:15]

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

[hajimehoshi, 2017-01-11 09:28:38]

Patchset #8 (id:160001) has been deleted

[commit-bot: I haz the power, 2017-01-11 09:28:40]

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, 2017-01-11 10:01:05]

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

[commit-bot: I haz the power, 2017-01-11 10:01:06]

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

[hajimehoshi, 2017-01-12 05:20:09]

I'm now fixing the CL as you suggested to improve performance. Thank you!

On 2016/12/21 15:06:41, Primiano Tucci (back but slow) wrote:
> Had a first high-level pass, here's some initial comments.
> I think overall is shooting in the right direction but we might need some
> iterations on this to put it in shape. Thanks for working on this.
> 
> 
> 
> 1) I think SharedMemoryDumpProvider is both a bit of a confusing name and
> misplaced.
> I'd name it SharedMemoryTracker and put under base/trace_event
> 
> 2) The fact that all those shared memory allocations are serialized under a
lock
> makes me a bit nervous performance-wise. Did you manage to measure the impact
of
> this? Perhaps a simple and very effective approach would be:
> Instead of having one big lock around the memory_usage_ field you could have
> a fixed-size array where usages are indexed by either id or mapped address
> (essetially a hashtable) where the lock is on each bucket of the array.
> In essence something like:
> struct TrackerBucket {
>   base::Lock lock;
>   std::unordered_map<Id, size_t> usage;
> }
> 
> TrackerBucket buckets_[N];
> Essentially before inserting and removing you'd hash the address/id.
> This would greatly reduce the collision domain on the lock and 
> make it practically lockless, given a reasonable hash and N.


I've not measured the performance but I’ll try. Which metrics or telemetry tests
would you suggest to use?

> 
> 3) I see that you stringprintf upfront. Would be nicer if we could just
> store whatever numeric IDs you need for the handle and stringify them only
> at dump time. So we avoid a string operation evey time a shared memory handle
> is created and destroyed.
> 
> 4) I see that a SharedMemory handle can be mapped or not. Are you dumping
> all of them regardless, or only if mapped? If the latter you can use the
> mapping address as hashing key (otherwise hash the IDs).
> 
>  
> 5) I think might be easier if this CL gets split into the //base part that
> introduces the various utilities in memory-infra and such, and the changes
> to the clients.
> 
> 6) Can I see a trace which shows the final effect in memory infra about this
> (even if is not 100% complete and is missing some details is fine)?

[hajimehoshi, 2017-01-12 08:28:47]

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

[commit-bot: I haz the power, 2017-01-12 08:29:03]

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

[hajimehoshi, 2017-01-12 08:31:44]

Thank you for reviewing!

> 1) I think SharedMemoryDumpProvider is both a bit of a confusing name and
> misplaced.
> I'd name it SharedMemoryTracker and put under base/trace_event

Done.

> 2) The fact that all those shared memory allocations are serialized under a
lock
> makes me a bit nervous performance-wise. Did you manage to measure the impact
of
> this? Perhaps a simple and very effective approach would be:
> Instead of having one big lock around the memory_usage_ field you could have
> a fixed-size array where usages are indexed by either id or mapped address
> (essetially a hashtable) where the lock is on each bucket of the array.
> In essence something like:
> struct TrackerBucket {
>   base::Lock lock;
>   std::unordered_map<Id, size_t> usage;
> }
> 
> TrackerBucket buckets_[N];
> Essentially before inserting and removing you'd hash the address/id.
> This would greatly reduce the collision domain on the lock and 
> make it practically lockless, given a reasonable hash and N.

When inserting a value to |usage|, isn’t a lock required?

> 3) I see that you stringprintf upfront. Would be nicer if we could just
> store whatever numeric IDs you need for the handle and stringify them only
> at dump time. So we avoid a string operation evey time a shared memory handle
> is created and destroyed.


Done (StringPrintf is now used only at AddOwnershipEdgeToSharedGlobalDump and
OnMemoryDump)

> 4) I see that a SharedMemory handle can be mapped or not. Are you dumping
> all of them regardless, or only if mapped? If the latter you can use the
> mapping address as hashing key (otherwise hash the IDs).


IncrementMemoryUsage is called only after mmap is called, so the address might
be available as identifier. However, I’m a bit confused since you said “2) You
need some kind of unique identifier for each mapping, which matches both sides
of the IPC, to avoid double-counting the same memory on both sides.” before. Am
I missing something?

> 5) I think might be easier if this CL gets split into the //base part that
> introduces the various utilities in memory-infra and such, and the changes
> to the clients.

Sure, I agree.

> 6) Can I see a trace which shows the final effect in memory infra about this
> (even if is not 100% complete and is missing some details is fine)?

[commit-bot: I haz the power, 2017-01-12 08:37:03]

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

[commit-bot: I haz the power, 2017-01-12 08:37:04]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[hajimehoshi, 2017-01-12 08:50:34]

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

[commit-bot: I haz the power, 2017-01-12 08:50:46]

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, 2017-01-12 09:00:57]

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

[commit-bot: I haz the power, 2017-01-12 09:00:58]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[hajimehoshi, 2017-01-12 11:38:03]

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

[commit-bot: I haz the power, 2017-01-12 11:38:19]

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, 2017-01-12 11:47:24]

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

[commit-bot: I haz the power, 2017-01-12 11:47:25]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[hajimehoshi, 2017-01-12 11:53:19]

Patchset #11 (id:240001) has been deleted

[hajimehoshi, 2017-01-12 11:53:30]

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

[commit-bot: I haz the power, 2017-01-12 11:53:44]

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

[hajimehoshi, 2017-01-12 11:58:31]

Patchset #10 (id:220001) has been deleted

[commit-bot: I haz the power, 2017-01-12 12:02:52]

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

[commit-bot: I haz the power, 2017-01-12 12:02:53]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[hajimehoshi, 2017-01-13 08:29:24]

Patchset #10 (id:250001) has been deleted

[hajimehoshi, 2017-01-13 08:36:58]

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

[commit-bot: I haz the power, 2017-01-13 08:37:14]

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, 2017-01-13 08:46:24]

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

[commit-bot: I haz the power, 2017-01-13 08:46:26]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[hajimehoshi, 2017-01-13 09:15:43]

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

[commit-bot: I haz the power, 2017-01-13 09:16:00]

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

[hajimehoshi, 2017-01-13 09:16:55]

Patchset #10 (id:270001) has been deleted

[commit-bot: I haz the power, 2017-01-13 10:46:56]

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

[commit-bot: I haz the power, 2017-01-13 10:46:57]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[hajimehoshi, 2017-01-19 10:30:49]

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

[commit-bot: I haz the power, 2017-01-19 10:31:11]

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, 2017-01-19 11:03:16]

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

[commit-bot: I haz the power, 2017-01-19 11:03:17]

Dry run: Try jobs failed on following builders:
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[hajimehoshi, 2017-01-19 11:12:05]

> I've not measured the performance but I’ll try. Which metrics or telemetry
tests
> would you suggest to use?

ping

On 2017/01/12 08:31:44, hajimehoshi ooo til Jan 9 wrote:
> Thank you for reviewing!
> 
> > 1) I think SharedMemoryDumpProvider is both a bit of a confusing name and
> > misplaced.
> > I'd name it SharedMemoryTracker and put under base/trace_event
> 
> Done.
> 
> > 2) The fact that all those shared memory allocations are serialized under a
> lock
> > makes me a bit nervous performance-wise. Did you manage to measure the
impact
> of
> > this? Perhaps a simple and very effective approach would be:
> > Instead of having one big lock around the memory_usage_ field you could have
> > a fixed-size array where usages are indexed by either id or mapped address
> > (essetially a hashtable) where the lock is on each bucket of the array.
> > In essence something like:
> > struct TrackerBucket {
> >   base::Lock lock;
> >   std::unordered_map<Id, size_t> usage;
> > }
> > 
> > TrackerBucket buckets_[N];
> > Essentially before inserting and removing you'd hash the address/id.
> > This would greatly reduce the collision domain on the lock and 
> > make it practically lockless, given a reasonable hash and N.
> 
> When inserting a value to |usage|, isn’t a lock required?

I understood that lock is not expensive as long as only one object actually
accesses the lock. Done.

> 
> > 3) I see that you stringprintf upfront. Would be nicer if we could just
> > store whatever numeric IDs you need for the handle and stringify them only
> > at dump time. So we avoid a string operation evey time a shared memory
handle
> > is created and destroyed.
> 
> 
> Done (StringPrintf is now used only at AddOwnershipEdgeToSharedGlobalDump and
> OnMemoryDump)
> 
> > 4) I see that a SharedMemory handle can be mapped or not. Are you dumping
> > all of them regardless, or only if mapped? If the latter you can use the
> > mapping address as hashing key (otherwise hash the IDs).
> 
> 
> IncrementMemoryUsage is called only after mmap is called, so the address might
> be available as identifier. However, I’m a bit confused since you said “2) You
> need some kind of unique identifier for each mapping, which matches both sides
> of the IPC, to avoid double-counting the same memory on both sides.” before.
Am
> I missing something?

ping

> 
> > 5) I think might be easier if this CL gets split into the //base part that
> > introduces the various utilities in memory-infra and such, and the changes
> > to the clients.
> 
> Sure, I agree.
> 
> > 6) Can I see a trace which shows the final effect in memory infra about this
> > (even if is not 100% complete and is missing some details is fine)?

[Primiano Tucci (use gerrit), 2017-01-19 12:47:04]

Ops sorry lost track of this

On 2017/01/19 11:12:05, hajimehoshi ooo til Jan 9 wrote:
> > I've not measured the performance but I’ll try. Which metrics or telemetry
> tests
> > would you suggest to use?

Good question. I'd try cc_perftests (I expect cc triggering a lot of mmaps) and
page_cycler_v2.typical_25 (maybe look at time to first contentful paint for some
pages)

> 
> On 2017/01/12 08:31:44, hajimehoshi ooo til Jan 9 wrote:
> > Thank you for reviewing!
> > 
> > > 1) I think SharedMemoryDumpProvider is both a bit of a confusing name and
> > > misplaced.
> > > I'd name it SharedMemoryTracker and put under base/trace_event
> > 
> > Done.
> > 
> > > 2) The fact that all those shared memory allocations are serialized under
a
> > lock
> > > makes me a bit nervous performance-wise. Did you manage to measure the
> impact
> > of
> > > this? Perhaps a simple and very effective approach would be:
> > > Instead of having one big lock around the memory_usage_ field you could
have
> > > a fixed-size array where usages are indexed by either id or mapped address
> > > (essetially a hashtable) where the lock is on each bucket of the array.
> > > In essence something like:
> > > struct TrackerBucket {
> > >   base::Lock lock;
> > >   std::unordered_map<Id, size_t> usage;
> > > }
> > > 
> > > TrackerBucket buckets_[N];
> > > Essentially before inserting and removing you'd hash the address/id.
> > > This would greatly reduce the collision domain on the lock and 
> > > make it practically lockless, given a reasonable hash and N.
> > 
> > When inserting a value to |usage|, isn’t a lock required?
> 
> I understood that lock is not expensive as long as only one object actually
> accesses the lock. Done.

What you say is true. So the issue here is that you have one lock for
everything, which would cause all calls to sharedmemory to linearize on a single
lock.
instead my suggesiton was to reduce the chance that two sharedmemory calls would
collide on the same lock by having one lock per bucket. does it make sense?

> 
> > 
> > > 3) I see that you stringprintf upfront. Would be nicer if we could just
> > > store whatever numeric IDs you need for the handle and stringify them only
> > > at dump time. So we avoid a string operation evey time a shared memory
> handle
> > > is created and destroyed.
> > 
> > 
> > Done (StringPrintf is now used only at AddOwnershipEdgeToSharedGlobalDump
and
> > OnMemoryDump)
> > 
> > > 4) I see that a SharedMemory handle can be mapped or not. Are you dumping
> > > all of them regardless, or only if mapped? If the latter you can use the
> > > mapping address as hashing key (otherwise hash the IDs).
> > 
> > 
> > IncrementMemoryUsage is called only after mmap is called, so the address
might
> > be available as identifier. However, I’m a bit confused since you said “2)
You
> > need some kind of unique identifier for each mapping, which matches both
sides
> > of the IPC, to avoid double-counting the same memory on both sides.” before.
> Am
> > I missing something?

So, if you want to report the same mapping on the browser side and renderer side
how do you do?
You cannot just use the virtual address, because when you mmap the same
sharedmemory handle on two processes the mapping will get a different virtual
address on each process, it won't be the same.
So my question is: say that we have a SharedMemory handle and is mapped on two
processes. How do you construct the string for memory-infra ? when you create
the global dump on both processes it has to be the same string do do the
matching. What do you use to make it so that the two mmaps dumped on both
processes end up with the same string (the arg of the guid ctor)

> > 
> > > 5) I think might be easier if this CL gets split into the //base part that
> > > introduces the various utilities in memory-infra and such, and the changes
> > > to the clients.
> > 
> > Sure, I agree.
> > 
> > > 6) Can I see a trace which shows the final effect in memory infra about
this
> > > (even if is not 100% complete and is missing some details is fine)?

[hajimehoshi, 2017-01-20 07:44:06]

On 2017/01/19 12:47:04, Primiano Tucci wrote:
> Ops sorry lost track of this
> 
> On 2017/01/19 11:12:05, hajimehoshi ooo til Jan 9 wrote:
> > > I've not measured the performance but I’ll try. Which metrics or telemetry
> > tests
> > > would you suggest to use?
> 
> Good question. I'd try cc_perftests (I expect cc triggering a lot of mmaps)
and
> page_cycler_v2.typical_25 (maybe look at time to first contentful paint for
some
> pages)
> 
> > 
> > On 2017/01/12 08:31:44, hajimehoshi ooo til Jan 9 wrote:
> > > Thank you for reviewing!
> > > 
> > > > 1) I think SharedMemoryDumpProvider is both a bit of a confusing name
and
> > > > misplaced.
> > > > I'd name it SharedMemoryTracker and put under base/trace_event
> > > 
> > > Done.
> > > 
> > > > 2) The fact that all those shared memory allocations are serialized
under
> a
> > > lock
> > > > makes me a bit nervous performance-wise. Did you manage to measure the
> > impact
> > > of
> > > > this? Perhaps a simple and very effective approach would be:
> > > > Instead of having one big lock around the memory_usage_ field you could
> have
> > > > a fixed-size array where usages are indexed by either id or mapped
address
> > > > (essetially a hashtable) where the lock is on each bucket of the array.
> > > > In essence something like:
> > > > struct TrackerBucket {
> > > >   base::Lock lock;
> > > >   std::unordered_map<Id, size_t> usage;
> > > > }
> > > > 
> > > > TrackerBucket buckets_[N];
> > > > Essentially before inserting and removing you'd hash the address/id.
> > > > This would greatly reduce the collision domain on the lock and 
> > > > make it practically lockless, given a reasonable hash and N.
> > > 
> > > When inserting a value to |usage|, isn’t a lock required?
> > 
> > I understood that lock is not expensive as long as only one object actually
> > accesses the lock. Done.
> 
> What you say is true. So the issue here is that you have one lock for
> everything, which would cause all calls to sharedmemory to linearize on a
single
> lock.
> instead my suggesiton was to reduce the chance that two sharedmemory calls
would
> collide on the same lock by having one lock per bucket. does it make sense?
> 
> > 
> > > 
> > > > 3) I see that you stringprintf upfront. Would be nicer if we could just
> > > > store whatever numeric IDs you need for the handle and stringify them
only
> > > > at dump time. So we avoid a string operation evey time a shared memory
> > handle
> > > > is created and destroyed.
> > > 
> > > 
> > > Done (StringPrintf is now used only at AddOwnershipEdgeToSharedGlobalDump
> and
> > > OnMemoryDump)
> > > 
> > > > 4) I see that a SharedMemory handle can be mapped or not. Are you
dumping
> > > > all of them regardless, or only if mapped? If the latter you can use the
> > > > mapping address as hashing key (otherwise hash the IDs).
> > > 
> > > 
> > > IncrementMemoryUsage is called only after mmap is called, so the address
> might
> > > be available as identifier. However, I’m a bit confused since you said “2)
> You
> > > need some kind of unique identifier for each mapping, which matches both
> sides
> > > of the IPC, to avoid double-counting the same memory on both sides.”
before.
> > Am
> > > I missing something?
> 
> So, if you want to report the same mapping on the browser side and renderer
side
> how do you do?
> You cannot just use the virtual address, because when you mmap the same
> sharedmemory handle on two processes the mapping will get a different virtual
> address on each process, it won't be the same.
> So my question is: say that we have a SharedMemory handle and is mapped on two
> processes. How do you construct the string for memory-infra ? when you create
> the global dump on both processes it has to be the same string do do the
> matching. What do you use to make it so that the two mmaps dumped on both
> processes end up with the same string (the arg of the guid ctor)


My current plan is, as implemented in this CL, 'device_id + inode_id' on Posix
and 'volume serial number + file index (high + low)' on Windows. File
descriptor's ID like inode must be same between two processes. Is this correct?
Unfortunately both device id and inode id are 64bit so the identifier would
consist of two 64bit integers.

One thing I need to consider is Mac: shared memory can be mach_port_t instead of
a file descriptor, but this is also an integer and might work if I appropriate
them properly.

> > > 
> > > > 5) I think might be easier if this CL gets split into the //base part
that
> > > > introduces the various utilities in memory-infra and such, and the
changes
> > > > to the clients.
> > > 
> > > Sure, I agree.
> > > 
> > > > 6) Can I see a trace which shows the final effect in memory infra about
> this
> > > > (even if is not 100% complete and is missing some details is fine)?

[hajimehoshi, 2017-01-20 09:23:39]

On 2017/01/20 07:44:06, hajimehoshi ooo til Jan 9 wrote:
> On 2017/01/19 12:47:04, Primiano Tucci wrote:
> > Ops sorry lost track of this
> > 
> > On 2017/01/19 11:12:05, hajimehoshi ooo til Jan 9 wrote:
> > > > I've not measured the performance but I’ll try. Which metrics or
telemetry
> > > tests
> > > > would you suggest to use?
> > 
> > Good question. I'd try cc_perftests (I expect cc triggering a lot of mmaps)
> and
> > page_cycler_v2.typical_25 (maybe look at time to first contentful paint for
> some
> > pages)
> > 
> > > 
> > > On 2017/01/12 08:31:44, hajimehoshi ooo til Jan 9 wrote:
> > > > Thank you for reviewing!
> > > > 
> > > > > 1) I think SharedMemoryDumpProvider is both a bit of a confusing name
> and
> > > > > misplaced.
> > > > > I'd name it SharedMemoryTracker and put under base/trace_event
> > > > 
> > > > Done.
> > > > 
> > > > > 2) The fact that all those shared memory allocations are serialized
> under
> > a
> > > > lock
> > > > > makes me a bit nervous performance-wise. Did you manage to measure the
> > > impact
> > > > of
> > > > > this? Perhaps a simple and very effective approach would be:
> > > > > Instead of having one big lock around the memory_usage_ field you
could
> > have
> > > > > a fixed-size array where usages are indexed by either id or mapped
> address
> > > > > (essetially a hashtable) where the lock is on each bucket of the
array.
> > > > > In essence something like:
> > > > > struct TrackerBucket {
> > > > >   base::Lock lock;
> > > > >   std::unordered_map<Id, size_t> usage;
> > > > > }
> > > > > 
> > > > > TrackerBucket buckets_[N];
> > > > > Essentially before inserting and removing you'd hash the address/id.
> > > > > This would greatly reduce the collision domain on the lock and 
> > > > > make it practically lockless, given a reasonable hash and N.
> > > > 
> > > > When inserting a value to |usage|, isn’t a lock required?
> > > 
> > > I understood that lock is not expensive as long as only one object
actually
> > > accesses the lock. Done.
> > 
> > What you say is true. So the issue here is that you have one lock for
> > everything, which would cause all calls to sharedmemory to linearize on a
> single
> > lock.
> > instead my suggesiton was to reduce the chance that two sharedmemory calls
> would
> > collide on the same lock by having one lock per bucket. does it make sense?
> > 
> > > 
> > > > 
> > > > > 3) I see that you stringprintf upfront. Would be nicer if we could
just
> > > > > store whatever numeric IDs you need for the handle and stringify them
> only
> > > > > at dump time. So we avoid a string operation evey time a shared memory
> > > handle
> > > > > is created and destroyed.
> > > > 
> > > > 
> > > > Done (StringPrintf is now used only at
AddOwnershipEdgeToSharedGlobalDump
> > and
> > > > OnMemoryDump)
> > > > 
> > > > > 4) I see that a SharedMemory handle can be mapped or not. Are you
> dumping
> > > > > all of them regardless, or only if mapped? If the latter you can use
the
> > > > > mapping address as hashing key (otherwise hash the IDs).
> > > > 
> > > > 
> > > > IncrementMemoryUsage is called only after mmap is called, so the address
> > might
> > > > be available as identifier. However, I’m a bit confused since you said
“2)
> > You
> > > > need some kind of unique identifier for each mapping, which matches both
> > sides
> > > > of the IPC, to avoid double-counting the same memory on both sides.”
> before.
> > > Am
> > > > I missing something?
> > 
> > So, if you want to report the same mapping on the browser side and renderer
> side
> > how do you do?
> > You cannot just use the virtual address, because when you mmap the same
> > sharedmemory handle on two processes the mapping will get a different
virtual
> > address on each process, it won't be the same.
> > So my question is: say that we have a SharedMemory handle and is mapped on
two
> > processes. How do you construct the string for memory-infra ? when you
create
> > the global dump on both processes it has to be the same string do do the
> > matching. What do you use to make it so that the two mmaps dumped on both
> > processes end up with the same string (the arg of the guid ctor)
> 
> 
> My current plan is, as implemented in this CL, 'device_id + inode_id' on Posix
> and 'volume serial number + file index (high + low)' on Windows. File
> descriptor's ID like inode must be same between two processes. Is this
correct?
> Unfortunately both device id and inode id are 64bit so the identifier would
> consist of two 64bit integers.
> 
> One thing I need to consider is Mac: shared memory can be mach_port_t instead
of
> a file descriptor, but this is also an integer and might work if I appropriate
> them properly.

s/appropriate them/treat them/

> 
> > > > 
> > > > > 5) I think might be easier if this CL gets split into the //base part
> that
> > > > > introduces the various utilities in memory-infra and such, and the
> changes
> > > > > to the clients.
> > > > 
> > > > Sure, I agree.
> > > > 
> > > > > 6) Can I see a trace which shows the final effect in memory infra
about
> > this
> > > > > (even if is not 100% complete and is missing some details is fine)?

[Primiano Tucci (use gerrit), 2017-01-20 16:34:50]

primiano@chromium.org changed reviewers:
+ ssid@chromium.org

[Primiano Tucci (use gerrit), 2017-01-20 16:34:52]

Thanks your reply makes sense. Also thanks for all the hard work doing all the
digging in the various clients.
I think this will be very useful and unveil a large portion of memory that we
were ignoring until now.

I'm adding +ssid here. ssid@ ignore all the plumbing and hooking into
base::SharedMemory, I'm just wondering if you feel that the edge logic works for
you or you feel we should do something differently.

hajimehoshi: in any case I'd start splitting this CL. A good V0 IMHO is
introducing only the tracker and the hooks into the various sharedmemory files,
to see how base owners react to this.

I added some comments here and there but overall I like the design now.
I have only one comment, that should be easy to address.
Essentially what I learned from reviewing this CL is that each shared memory
object can be in two states:
1) Created and mapped
2) created only, but not mapped

From a concrete viewpoint, on Linux, 1 means file descriptor + mmap, 2 means
file descriptor only.

Now, I think (we might need to dig more in the other OS to check whether this is
true) that 2 doesn't actually use any resource (other than a fd). What I would
do here is tracking both, i.e. reporting two columns to memory-infra:
- the mmaped size (which I'd make the default "size" column): only if created
and mapped, otherwise 0
- segment_size, that is, the size of the Handle object.
In order to do this I'd require the callers to pass also the mmap virtual
address size to the tracker.
Also, for the case 1. I would use the address as reporting key, so the dump
would look like


name                          size           segment_size
sharedmem/0x1234              1M             1M
sharedmem/0x5678              2M             2M
sharedmem/unmapped_01.234     0              1M
sharedmem/unmapped_05.678     0              1M


That would allow us to catch bugs like: we are unmapping memory but not
releasing the file descriptor.

Furthermore, On Linux/Android/OS you can use
ProcessMemoryDump::CountResidentBytes to add a third_column (resident size). But
i'd do this in a separate CL anyways.

https://codereview.chromium.org/2535213002/diff/310001/base/memory/shared_mem...
File base/memory/shared_memory_handle.h (right):

https://codereview.chromium.org/2535213002/diff/310001/base/memory/shared_mem...
base/memory/shared_memory_handle.h:207: bool GetIDFromSharedMemoryHandle(const
SharedMemoryHandle& handle,
should this just be a member function of SharedMemoryHandle? something like
SharedMemoryHandleID SharedMemoryHandle::GetUniqueID() ?
and eventually have the SharedMemoryHandleID struct to have a ToString() method.
I think would be cleaner

https://codereview.chromium.org/2535213002/diff/310001/base/memory/shared_mem...
File base/memory/shared_memory_handle_posix.cc (right):

https://codereview.chromium.org/2535213002/diff/310001/base/memory/shared_mem...
base/memory/shared_memory_handle_posix.cc:19: if
(fstat(static_cast<int>(handle.fd), &file_stat) != 0)
I checked the sandbox and fstat seems allowed (good)

https://codereview.chromium.org/2535213002/diff/310001/base/memory/shared_mem...
File base/memory/shared_memory_mac.cc (right):

https://codereview.chromium.org/2535213002/diff/310001/base/memory/shared_mem...
base/memory/shared_memory_mac.cc:261:
SharedMemoryTracker::GetInstance()->DecrementMemoryUsage(*this);
I think this might not be the right point.
The decription of SharedMemory::Close()  says:
// Closes the open shared memory segment. The memory will remain mapped if
// it was previously mapped.
// It is safe to call Close repeatedly.

In other words, if you Map() and then Close(), you are still using memory. Close
just frees up the file descriptor.
I believe that what you want to intercept here is the SharedMemory::Unmap()

https://codereview.chromium.org/2535213002/diff/310001/base/memory/shared_mem...
File base/memory/shared_memory_tracker.h (right):

https://codereview.chromium.org/2535213002/diff/310001/base/memory/shared_mem...
base/memory/shared_memory_tracker.h:49: std::unordered_map<int64_t,
std::unordered_map<int64_t, size_t>> usage;
I don't think you should expose device id and file id here. I think there is a
cleaner option:
add hash operators to SharedMemoryHandleID  and make this
unordered_map<SharedMemoryHandleId, size_t>

https://codereview.chromium.org/2535213002/diff/310001/base/memory/shared_mem...
File base/memory/shared_memory_win.cc (right):

https://codereview.chromium.org/2535213002/diff/310001/base/memory/shared_mem...
base/memory/shared_memory_win.cc:383:
SharedMemoryTracker::GetInstance()->DecrementMemoryUsage(*this);
see my prev comment about close vs unmap. I think you should just ignore close()

[hajimehoshi, 2017-01-23 11:58:38]

On 2017/01/20 16:34:52, Primiano Tucci wrote:
> Thanks your reply makes sense. Also thanks for all the hard work doing all the
> digging in the various clients.
> I think this will be very useful and unveil a large portion of memory that we
> were ignoring until now.
> 
> I'm adding +ssid here. ssid@ ignore all the plumbing and hooking into
> base::SharedMemory, I'm just wondering if you feel that the edge logic works
for
> you or you feel we should do something differently.
> 
> hajimehoshi: in any case I'd start splitting this CL. A good V0 IMHO is
> introducing only the tracker and the hooks into the various sharedmemory
files,
> to see how base owners react to this.
> 
> I added some comments here and there but overall I like the design now.
> I have only one comment, that should be easy to address.
> Essentially what I learned from reviewing this CL is that each shared memory
> object can be in two states:
> 1) Created and mapped
> 2) created only, but not mapped
> 
> From a concrete viewpoint, on Linux, 1 means file descriptor + mmap, 2 means
> file descriptor only.
> 
> Now, I think (we might need to dig more in the other OS to check whether this
is
> true) that 2 doesn't actually use any resource (other than a fd). What I would
> do here is tracking both, i.e. reporting two columns to memory-infra:
> - the mmaped size (which I'd make the default "size" column): only if created
> and mapped, otherwise 0
> - segment_size, that is, the size of the Handle object.
> In order to do this I'd require the callers to pass also the mmap virtual
> address size to the tracker.
> Also, for the case 1. I would use the address as reporting key, so the dump
> would look like
> 
> 
> name                          size           segment_size
> sharedmem/0x1234              1M             1M
> sharedmem/0x5678              2M             2M
> sharedmem/unmapped_01.234     0              1M
> sharedmem/unmapped_05.678     0              1M
> 
> 
> That would allow us to catch bugs like: we are unmapping memory but not
> releasing the file descriptor.
> 
> Furthermore, On Linux/Android/OS you can use
> ProcessMemoryDump::CountResidentBytes to add a third_column (resident size).
But
> i'd do this in a separate CL anyways.
> 

Thank you! I'll try this fix (adding segment_size info), and at the same time,
I'll try to separate this CL for actual reviewing.

BTW, in the above example, segment_size seems taking 1M or 2M but these figures
are just examples and the actual handlers doesn't take so much memory, right?

[hajimehoshi, 2017-01-23 11:59:09]

https://codereview.chromium.org/2535213002/diff/310001/base/memory/shared_mem...
File base/memory/shared_memory_handle.h (right):

https://codereview.chromium.org/2535213002/diff/310001/base/memory/shared_mem...
base/memory/shared_memory_handle.h:207: bool GetIDFromSharedMemoryHandle(const
SharedMemoryHandle& handle,
On 2017/01/20 16:34:52, Primiano Tucci wrote:
> should this just be a member function of SharedMemoryHandle? something like
> SharedMemoryHandleID SharedMemoryHandle::GetUniqueID() ?
> and eventually have the SharedMemoryHandleID struct to have a ToString()
method.
> I think would be cleaner

I like your suggestion, but on POSIX, SharedMemoryHandle is FileDescriptor, and
I hesitated to add your suggested methods to FileDescriptor. Do you think this
is fine?

https://codereview.chromium.org/2535213002/diff/310001/base/memory/shared_mem...
File base/memory/shared_memory_handle_posix.cc (right):

https://codereview.chromium.org/2535213002/diff/310001/base/memory/shared_mem...
base/memory/shared_memory_handle_posix.cc:19: if
(fstat(static_cast<int>(handle.fd), &file_stat) != 0)
On 2017/01/20 16:34:52, Primiano Tucci wrote:
> I checked the sandbox and fstat seems allowed (good)

Thank you!

https://codereview.chromium.org/2535213002/diff/310001/base/memory/shared_mem...
File base/memory/shared_memory_mac.cc (right):

https://codereview.chromium.org/2535213002/diff/310001/base/memory/shared_mem...
base/memory/shared_memory_mac.cc:261:
SharedMemoryTracker::GetInstance()->DecrementMemoryUsage(*this);
On 2017/01/20 16:34:52, Primiano Tucci wrote:
> I think this might not be the right point.
> The decription of SharedMemory::Close()  says:
> // Closes the open shared memory segment. The memory will remain mapped if
> // it was previously mapped.
> // It is safe to call Close repeatedly.
> 
> In other words, if you Map() and then Close(), you are still using memory.
Close
> just frees up the file descriptor.
> I believe that what you want to intercept here is the SharedMemory::Unmap()

I was wondering if the file descriptor's is still usable for ID even after
closing.

Anyway you are right that Close doesn't affect memory usage. Done.

https://codereview.chromium.org/2535213002/diff/310001/base/memory/shared_mem...
File base/memory/shared_memory_tracker.h (right):

https://codereview.chromium.org/2535213002/diff/310001/base/memory/shared_mem...
base/memory/shared_memory_tracker.h:49: std::unordered_map<int64_t,
std::unordered_map<int64_t, size_t>> usage;
On 2017/01/20 16:34:52, Primiano Tucci wrote:
> I don't think you should expose device id and file id here. I think there is a
> cleaner option:
> add hash operators to SharedMemoryHandleID  and make this
> unordered_map<SharedMemoryHandleId, size_t>

Done.

https://codereview.chromium.org/2535213002/diff/310001/base/memory/shared_mem...
File base/memory/shared_memory_win.cc (right):

https://codereview.chromium.org/2535213002/diff/310001/base/memory/shared_mem...
base/memory/shared_memory_win.cc:383:
SharedMemoryTracker::GetInstance()->DecrementMemoryUsage(*this);
On 2017/01/20 16:34:52, Primiano Tucci wrote:
> see my prev comment about close vs unmap. I think you should just ignore
close()

Done.

[hajimehoshi, 2017-01-23 12:00:40]

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

[commit-bot: I haz the power, 2017-01-23 12:00:59]

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, 2017-01-23 12:08:54]

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

[commit-bot: I haz the power, 2017-01-23 12:08:56]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Primiano Tucci (use gerrit), 2017-01-24 01:26:41]

SG. I think makes sense to start splitting this CL to make reviewable from the
various owners.

https://codereview.chromium.org/2535213002/diff/310001/base/memory/shared_mem...
File base/memory/shared_memory_handle.h (right):

https://codereview.chromium.org/2535213002/diff/310001/base/memory/shared_mem...
base/memory/shared_memory_handle.h:207: bool GetIDFromSharedMemoryHandle(const
SharedMemoryHandle& handle,
On 2017/01/23 11:59:09, hajimehoshi ooo til Jan 9 wrote:
> On 2017/01/20 16:34:52, Primiano Tucci wrote:
> > should this just be a member function of SharedMemoryHandle? something like
> > SharedMemoryHandleID SharedMemoryHandle::GetUniqueID() ?
> > and eventually have the SharedMemoryHandleID struct to have a ToString()
> method.
> > I think would be cleaner
> 
> I like your suggestion, but on POSIX, SharedMemoryHandle is FileDescriptor,
and
> I hesitated to add your suggested methods to FileDescriptor. Do you think this
> is fine?

Ahh okay I didn't notice. I think it was always some wrapper class. If this is a
FD class, my proposal becomes controversial. So your approach makes more sense.
Or probably you can reverse it and make it a static factory method of
MemoryHandleId, something like:

class MemoryHandleId {
static SharedMemoryHandleID FromSharedMemoryhandle(const SHM&);
...
}

[ssid, 2017-01-24 04:39:50]

Primiano, please double check if I am making sense here. I think we need a doc
to discuss these issues since I am not even clear if what I think is correct.

Okay this looks like a great change. I only looked at the ownership edge parts.
The main thing here is we do not need to create global dump in any of the other
dump providers. We only need to sub-allocate or add ownership edge to the local
dump that was already created by the shared-memory dump provider. So, if we are
going to do this, we can remove all the logic related to global dumps from all
the dump providers except shared-memory one. I have added a comment on the file
on how to implement the AddOwnership function in this way.

The final dumps will look like this:
Browser Creates:
 "shared_memory/seg1"  Owns "global/shared_memory/seg1"
  "global/shared_memory/seg1"
 "discardable/seg1" Owns "shared_memory/seg1"

Renderer Creates:
 "shared_memory/seg1" Owns "global/shared_memory/seg1" with importance 2!
  "global/shared_memory/seg1"
 "discardable/seg1" Owns "shared_memory/seg1"


Output:
Browser:
shared_memory          effective_size     size
  -> 10.1          ->  0                  4MB
(shared with renderer)

discardable
  -> segment_1     ->  0                  0? (never used the memory)

Renderer:
shared_memory          effective_size     size
  -> 10.1          ->  0                  4MB
(shared with renderer)

discardable
  -> segment_1     ->  4MB                4MB


I think this kind of edges are possible ONLY if the browser discadable segments
do not have size. If it did have size, then there are 2 dumps that will own the
browser shared_memory segment (the renderer "shared_memory" dump (because of
importance in edge) and the browser "discardable/segment_1" dump). This is also
not allowed in the UI. 
Positives: This would solve the issue with discardable where the host does not
know the client pid anymore!
https://bugs.chromium.org/p/chromium/issues/detail?id=661257
Reduces complexity in all other dump providers.

I think the shared_memory should always attribute all the memory usage to
renderers and not browser. That is, if 2 renderers and browser shares a segment,
then the memory should be attributed 50% to each renderer and 0% to browser. I
am assuming it is always the case that the browser allocates memory for renderer
/ gpu and not really use it on it's own.

There is also an issue of weak dumps since the actual size of discardable
segments are known only by the client discardable interface.




To avoid the above issues, I think a better solution would be to not create
global dump edges in "shared_memory" dumps. All the shared_memory dumps are
owned by some other dumps in process (either by gpu or discardable). So, the
global dumps will be appropriately created by these components and shared_memory
dumps only creates in-process dumps. Now the issue would be in case where none
of the components report the shared_memory dump, these dumps will be double
counted. Can we detect somehow and add global edges only for these dumps?

In any case only one of the dump providers (either shared_memory or discardable)
should be allowed to add global dump edges. Other one should only be adding
in-process edges.

https://codereview.chromium.org/2535213002/diff/330001/base/memory/shared_mem...
File base/memory/shared_memory_tracker.cc (right):

https://codereview.chromium.org/2535213002/diff/330001/base/memory/shared_mem...
base/memory/shared_memory_tracker.cc:14: const char* kSharedMemoryDumpName =
"sharedmemory";
shared_memory? other dump provider add names with '_'.

https://codereview.chromium.org/2535213002/diff/330001/base/memory/shared_mem...
base/memory/shared_memory_tracker.cc:21: bool
SharedMemoryTracker::AddOwnershipEdgeToSharedGlobalDump(
I think this should be changed to:
SharedMemoryTracker::AddSuballocation(
    trace_event::ProcessMemoryDump* pmd,
    const trace_event::MemoryAllocatorDumpGuid& source,
    const SharedMemoryHandle& shared_memory_handle,
    size_t mapped_size) {
  auto* in_process_dump_name = base::StringPrintf("%s/%s",
kSharedMemoryDumpName, id_str.c_str());
  pmd->AddSubalocation(source, in_process_dump_name);
}

I am assuming that the mapped_size is not same as the size of the dump given by
line 57 in this file (usage.second).


In case if the usage.second is same as the mapped_size always, then this
function should be:
SharedMemoryTracker::AddOwnershipEdge(
    trace_event::ProcessMemoryDump* pmd,
    const trace_event::MemoryAllocatorDumpGuid& source,
    const SharedMemoryHandle& shared_memory_handle) {
  pmd->AddOwnershipEdge(source,
      base::trace_event::MemoryAllocatorDumpGuid(dump_name));
}

https://codereview.chromium.org/2535213002/diff/330001/components/discardable...
File components/discardable_memory/common/discardable_shared_memory_heap.cc
(right):

https://codereview.chromium.org/2535213002/diff/330001/components/discardable...
components/discardable_memory/common/discardable_shared_memory_heap.cc:428:
base::trace_event::MemoryAllocatorDumpGuid shared_segment_guid =
We no longer need the logic of global memory dumps in the discardable dump
provider. The connection between the 2 shared memory segments is made by the
sharedmemory's MemoryDumpProvider using global dump guid(s). So, we just need to
add a sub-allocation or ownership edge to the in-process dump. We can remove all
code related to the global dump creations here.

https://codereview.chromium.org/2535213002/diff/330001/components/discardable...
components/discardable_memory/common/discardable_shared_memory_heap.cc:446:
shared_memory->handle(), shared_memory->mapped_size());
Okay the right thing to do here is to keep the hierarchy of allocation and add
global dump edges only for the "sharedmemory" dumps and discardable should just
sub-allocate from the "sharedmemory/xx" objects.
So the code should look like:

base::SharedMemoryTracker::AddSuballocation(segment_dump->guid(),
shared_memory->handle(), shared_memory->mapped_size());

Also please do not add edges between 2 global dumps. The UI still does not know
how to deal with this.

[hajimehoshi, 2017-01-24 10:49:14]

On 2017/01/24 04:39:50, ssid wrote:
> Primiano, please double check if I am making sense here. I think we need a doc
> to discuss these issues since I am not even clear if what I think is correct.
> 
> Okay this looks like a great change. I only looked at the ownership edge
parts.
> The main thing here is we do not need to create global dump in any of the
other
> dump providers. We only need to sub-allocate or add ownership edge to the
local
> dump that was already created by the shared-memory dump provider. So, if we
are
> going to do this, we can remove all the logic related to global dumps from all
> the dump providers except shared-memory one. I have added a comment on the
file
> on how to implement the AddOwnership function in this way.
> 
> The final dumps will look like this:
> Browser Creates:
>  "shared_memory/seg1"  Owns "global/shared_memory/seg1"
>   "global/shared_memory/seg1"
>  "discardable/seg1" Owns "shared_memory/seg1"
> 
> Renderer Creates:
>  "shared_memory/seg1" Owns "global/shared_memory/seg1" with importance 2!
>   "global/shared_memory/seg1"
>  "discardable/seg1" Owns "shared_memory/seg1"
> 
> 
> Output:
> Browser:
> shared_memory          effective_size     size
>   -> 10.1          ->  0                  4MB
> (shared with renderer)
> 
> discardable
>   -> segment_1     ->  0                  0? (never used the memory)
> 
> Renderer:
> shared_memory          effective_size     size
>   -> 10.1          ->  0                  4MB
> (shared with renderer)
> 
> discardable
>   -> segment_1     ->  4MB                4MB
> 
> 
> I think this kind of edges are possible ONLY if the browser discadable
segments
> do not have size. If it did have size, then there are 2 dumps that will own
the
> browser shared_memory segment (the renderer "shared_memory" dump (because of
> importance in edge) and the browser "discardable/segment_1" dump). This is
also
> not allowed in the UI. 
> Positives: This would solve the issue with discardable where the host does not
> know the client pid anymore!
> https://bugs.chromium.org/p/chromium/issues/detail?id=661257
> Reduces complexity in all other dump providers.
> 
> I think the shared_memory should always attribute all the memory usage to
> renderers and not browser. That is, if 2 renderers and browser shares a
segment,
> then the memory should be attributed 50% to each renderer and 0% to browser. I
> am assuming it is always the case that the browser allocates memory for
renderer
> / gpu and not really use it on it's own.
> 
> There is also an issue of weak dumps since the actual size of discardable
> segments are known only by the client discardable interface.
> 
> 
> 
> 
> To avoid the above issues, I think a better solution would be to not create
> global dump edges in "shared_memory" dumps. All the shared_memory dumps are
> owned by some other dumps in process (either by gpu or discardable). So, the
> global dumps will be appropriately created by these components and
shared_memory
> dumps only creates in-process dumps. Now the issue would be in case where none
> of the components report the shared_memory dump, these dumps will be double
> counted. Can we detect somehow and add global edges only for these dumps?
> 
> In any case only one of the dump providers (either shared_memory or
discardable)
> should be allowed to add global dump edges. Other one should only be adding
> in-process edges.


Thank you for the advice!

Hmm, what we (Primiano and I) wanted to show shared_memory usage correctly on
memory-infra, and to avoid duplication, creating ownership from a global dump
(e.g. global segment GUID) to another global dump (global/shared/... which I
introduce in this CL) seemed a good idea. I am also not clear about this, and
possibly I didn't implement what we actually want in this CL. Primiano, what do
you think?


> 
>
https://codereview.chromium.org/2535213002/diff/330001/base/memory/shared_mem...
> File base/memory/shared_memory_tracker.cc (right):
> 
>
https://codereview.chromium.org/2535213002/diff/330001/base/memory/shared_mem...
> base/memory/shared_memory_tracker.cc:14: const char* kSharedMemoryDumpName =
> "sharedmemory";
> shared_memory? other dump provider add names with '_'.
> 
>
https://codereview.chromium.org/2535213002/diff/330001/base/memory/shared_mem...
> base/memory/shared_memory_tracker.cc:21: bool
> SharedMemoryTracker::AddOwnershipEdgeToSharedGlobalDump(
> I think this should be changed to:
> SharedMemoryTracker::AddSuballocation(
>     trace_event::ProcessMemoryDump* pmd,
>     const trace_event::MemoryAllocatorDumpGuid& source,
>     const SharedMemoryHandle& shared_memory_handle,
>     size_t mapped_size) {
>   auto* in_process_dump_name = base::StringPrintf("%s/%s",
> kSharedMemoryDumpName, id_str.c_str());
>   pmd->AddSubalocation(source, in_process_dump_name);
> }
> 
> I am assuming that the mapped_size is not same as the size of the dump given
by
> line 57 in this file (usage.second).
> 
> 
> In case if the usage.second is same as the mapped_size always, then this
> function should be:
> SharedMemoryTracker::AddOwnershipEdge(
>     trace_event::ProcessMemoryDump* pmd,
>     const trace_event::MemoryAllocatorDumpGuid& source,
>     const SharedMemoryHandle& shared_memory_handle) {
>   pmd->AddOwnershipEdge(source,
>       base::trace_event::MemoryAllocatorDumpGuid(dump_name));
> }
> 
>
https://codereview.chromium.org/2535213002/diff/330001/components/discardable...
> File components/discardable_memory/common/discardable_shared_memory_heap.cc
> (right):
> 
>
https://codereview.chromium.org/2535213002/diff/330001/components/discardable...
> components/discardable_memory/common/discardable_shared_memory_heap.cc:428:
> base::trace_event::MemoryAllocatorDumpGuid shared_segment_guid =
> We no longer need the logic of global memory dumps in the discardable dump
> provider. The connection between the 2 shared memory segments is made by the
> sharedmemory's MemoryDumpProvider using global dump guid(s). So, we just need
to
> add a sub-allocation or ownership edge to the in-process dump. We can remove
all
> code related to the global dump creations here.
> 
>
https://codereview.chromium.org/2535213002/diff/330001/components/discardable...
> components/discardable_memory/common/discardable_shared_memory_heap.cc:446:
> shared_memory->handle(), shared_memory->mapped_size());
> Okay the right thing to do here is to keep the hierarchy of allocation and add
> global dump edges only for the "sharedmemory" dumps and discardable should
just
> sub-allocate from the "sharedmemory/xx" objects.
> So the code should look like:
> 
> base::SharedMemoryTracker::AddSuballocation(segment_dump->guid(),
> shared_memory->handle(), shared_memory->mapped_size());
> 
> Also please do not add edges between 2 global dumps. The UI still does not
know
> how to deal with this.

[ssid, 2017-01-24 23:36:37]

Had a discussion with primiano@ about this issue. I was wrong about 2 dumps
owning the browser dump. That is actually supported by the UI.
So, This is how the dumps and edges will finally look like:

Browser:

Discardable::OnMemoryDump()
  std::string discardable_brow = StringPrintf(
      "discardable_brow_%d/seg1", id);
  auto discardable_brow1 = pmd->CreateAllocatorDump(
      discardable_brow);
  discardable_brow1->AddScalar("size", "bytes", 1024 * 1024);
  SharedMemory::AddOwnershipEdge(discardable_brow1);

SharedMemory::OnMemoryDump()
  std::string shared_memory_brow = StringPrintf(
      "shared_memory_brow_%d/seg1", id);
  auto shared_memory_brow1 = pmd->CreateAllocatorDump(
      shared_memory_brow);
  shared_memory_brow1->AddScalar("size", "bytes", 1024 * 1024);
  auto shared_memory_brow_global =
      pmd->CreateSharedGlobalAllocatorDump(MemoryAllocatorDumpGuid(id));
  pmd->AddOwnershipEdge(
      shared_memory_brow1->guid(), shared_memory_brow_global->guid());

SharedMemory::AddOwnershipEdge()
  pmd->AddOwnershipEdge(
      discardable_brow1->guid(), shared_memory_brow1->guid());

Renderer:

Discardable::OnMemoryDump()
  std::string discardable_rend = StringPrintf(
      "discardable_rend_%d/seg1", id);
  auto discardable_rend1 = pmd->CreateAllocatorDump(
      discardable_rend);
  discardable_rend1->AddScalar("size", "bytes", 800 * 1024);
  SharedMemory::AddOwnershipEdge(discardable_rend1);

SharedMemory::OnMemoryDump()
  std::string shared_memory_rend = StringPrintf(
      "shared_memory_rend_%d/seg1", id);
  auto shared_memory_rend1 = pmd->CreateAllocatorDump(
      shared_memory_rend);
  shared_memory_rend1->AddScalar("size", "bytes", 800 * 1024);
  auto shared_memory_rend_global = 
      pmd->CreateSharedGlobalAllocatorDump(MemoryAllocatorDumpGuid(id));
  shared_memory_rend_global->AddScalar("size", "bytes", 800 * 1024);
  pmd->AddOwnershipEdge(shared_memory_rend1->guid(),
                        shared_memory_rend_global->guid(),
                        2 /* importance */);

SharedMemory::AddOwnershipEdge()
  pmd->AddOwnershipEdge(
      discardable_rend1->guid(), shared_memory_rend1->guid());



The code above has different sizes in browser and renderer side dumps. This is
because we try to dump the resident size in the dump. The browser does not know
the resident size of the segment since it just mmaps large segments for the
renderer and renderer segments it uses in parts. The "size" of the dump
providers represent resident sizes. So, I think we should not be adding size
fields to the browser side dumps. But, we can come back to this issue later.

I think let's start with just shared memory dumps without edges from other dump
providers. and a second CL that actually removes the global dump edges from
other dump providers and moves it to the shared memory dumps only.

[hajimehoshi, 2017-01-25 07:24:10]

Description was changed from

==========
[WIP] Add SharedMemoryDumpProfiler to dump base::SharedMemory usage

This CL adds SharedMemoryDumpProfiler to dump base::SharedMemory usage.

Remaining tasks:
* Confirm there is no regression in terms of performance
* Add tests

BUG=604726
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
[WIP] Add SharedMemoryTracker to dump base::SharedMemory usage

This CL adds SharedMemoryTracker to dump base::SharedMemory usage.

Remaining tasks:
* Confirm there is no regression in terms of performance
* Add tests

BUG=604726
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[hajimehoshi, 2017-03-06 09:24:14]

Description was changed from

==========
[WIP] Add SharedMemoryTracker to dump base::SharedMemory usage

This CL adds SharedMemoryTracker to dump base::SharedMemory usage.

Remaining tasks:
* Confirm there is no regression in terms of performance
* Add tests

BUG=604726
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
[WIP] Add SharedMemoryTracker to dump base::SharedMemory usage

This CL adds SharedMemoryTracker to dump base::SharedMemory usage.

Remaining tasks:
* Confirm there is no regression in terms of performance
* Add tests

BUG=604726
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[hajimehoshi, 2017-03-08 07:26:52]

Description was changed from

==========
[WIP] Add SharedMemoryTracker to dump base::SharedMemory usage

This CL adds SharedMemoryTracker to dump base::SharedMemory usage.

Remaining tasks:
* Confirm there is no regression in terms of performance
* Add tests

BUG=604726
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
[WIP] Add SharedMemoryTracker to dump base::SharedMemory usage

This CL adds SharedMemoryTracker to dump base::SharedMemory usage.

Remaining tasks:
* Confirm there is no regression in terms of performance
* Add tests

BUG=604726
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[hajimehoshi, 2017-03-10 12:00:33]

Patchset #15 (id:390001) has been deleted

[hajimehoshi, 2017-03-22 09:25:20]

Patchset #15 (id:410001) has been deleted

[hajimehoshi, 2017-03-22 09:29:12]

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

[commit-bot: I haz the power, 2017-03-22 09:29:43]

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, 2017-03-22 09:38:47]

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

[commit-bot: I haz the power, 2017-03-22 09:38:49]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[hajimehoshi, 2017-03-23 05:14:45]

Patchset #15 (id:430001) has been deleted

[hajimehoshi, 2017-03-24 08:37:45]

Patchset #16 (id:470001) has been deleted

[hajimehoshi, 2017-03-24 12:09:02]

Patchset #16 (id:490001) has been deleted

[hajimehoshi, 2017-03-27 10:05:01]

As https://codereview.chromium.org/2654073002/ has been landed, I fixed this CL
to add ownerships correctly. The current implementation is only for POSIX. I'll
start to create a separate small CL from this. PTAL

https://codereview.chromium.org/2535213002/diff/510001/components/discardable...
File components/discardable_memory/service/discardable_shared_memory_manager.cc
(left):

https://codereview.chromium.org/2535213002/diff/510001/components/discardable...
components/discardable_memory/service/discardable_shared_memory_manager.cc:342:
->AddScalar("resident_size",
I have no idea what is the difference between 'resident_size' here and 'size'
and how to treat this with my fix.

[hajimehoshi, 2017-03-29 11:50:35]

ping

[Primiano Tucci (use gerrit), 2017-03-29 23:59:19]

On 2017/03/29 11:50:35, hajimehoshi wrote:
> ping

Is this cl to review? The title says wip and some pieces seem to have landed
separately?

[hajimehoshi, 2017-03-30 05:02:54]

On 2017/03/29 23:59:19, Primiano Tucci wrote:
> On 2017/03/29 11:50:35, hajimehoshi wrote:
> > ping
> 
> Is this cl to review? The title says wip and some pieces seem to have landed
> separately?

I reflected ssid@'s comment and I wanted to check if the general direction is
OK, there are still some works to do yet though.

As you said, same things are in the other small CL
(https://codereview.chromium.org/2775423003/), so let's continue there. Thanks.

[hajimehoshi, 2017-03-30 08:32:04]

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

[commit-bot: I haz the power, 2017-03-30 08:32:19]

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, 2017-03-30 08:42:13]

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

[commit-bot: I haz the power, 2017-03-30 08:42:14]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[hajimehoshi, 2017-04-05 08:46:32]

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

[commit-bot: I haz the power, 2017-04-05 08:46:43]

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

[hajimehoshi, 2017-04-05 08:50:15]

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

[commit-bot: I haz the power, 2017-04-05 08:50:30]

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, 2017-04-05 09:28:20]

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

[commit-bot: I haz the power, 2017-04-05 09:28:21]

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_...)

[hajimehoshi, 2017-04-06 08:47:32]

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

[commit-bot: I haz the power, 2017-04-06 08:47:52]

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, 2017-04-06 09:38:45]

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

[commit-bot: I haz the power, 2017-04-06 09:38:48]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[hajimehoshi, 2017-04-07 08:08:23]

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

[commit-bot: I haz the power, 2017-04-07 08:08:46]

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

[hajimehoshi, 2017-04-07 08:09:05]

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

[commit-bot: I haz the power, 2017-04-07 08:09:18]

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, 2017-04-07 09:53:27]

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

[commit-bot: I haz the power, 2017-04-07 09:53:29]

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

[hajimehoshi, 2017-04-11 11:06:15]

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

[commit-bot: I haz the power, 2017-04-11 11:06:25]

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, 2017-04-11 11:08:45]

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

[commit-bot: I haz the power, 2017-04-11 11:08:47]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[hajimehoshi, 2017-04-11 11:59:54]

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

[commit-bot: I haz the power, 2017-04-11 12:00:08]

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, 2017-04-11 14:06:38]

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

[commit-bot: I haz the power, 2017-04-11 14:06:40]

Dry run: This issue passed the CQ dry run.

[hajimehoshi, 2017-04-25 10:30:39]

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

[commit-bot: I haz the power, 2017-04-25 10:30:56]

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, 2017-04-25 10:50:57]

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

[commit-bot: I haz the power, 2017-04-25 10:51:00]

Dry run: Try jobs failed on following builders:
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[hajimehoshi, 2017-04-27 06:52:48]

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

[commit-bot: I haz the power, 2017-04-27 06:53:02]

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, 2017-04-27 07:03:48]

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

[commit-bot: I haz the power, 2017-04-27 07:03:49]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hajimehoshi, 2017-04-28 10:57:28]

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

[commit-bot: I haz the power, 2017-04-28 10:57:44]

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, 2017-04-28 11:08:34]

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

[commit-bot: I haz the power, 2017-04-28 11:08:35]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hajimehoshi, 2017-04-28 11:45:26]

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

[commit-bot: I haz the power, 2017-04-28 11:45:50]

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, 2017-04-28 11:56:07]

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

[commit-bot: I haz the power, 2017-04-28 11:56:09]

Dry run: Try jobs failed on following builders:
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[hajimehoshi, 2017-05-17 11:47:27]

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

[commit-bot: I haz the power, 2017-05-17 11:47:45]

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, 2017-05-17 11:59:31]

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

[commit-bot: I haz the power, 2017-05-17 11:59:32]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[hajimehoshi, 2017-05-18 07:07:19]

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

[commit-bot: I haz the power, 2017-05-18 07:07:54]

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, 2017-05-18 07:12:03]

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

[commit-bot: I haz the power, 2017-05-18 07:12:04]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[hajimehoshi, 2017-05-18 07:26:22]

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

[commit-bot: I haz the power, 2017-05-18 07:26:54]

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, 2017-05-18 07:43:56]

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

[commit-bot: I haz the power, 2017-05-18 07:43:57]

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

[hajimehoshi, 2017-05-18 08:42:21]

Patchset #33 (id:850001) has been deleted

[hajimehoshi, 2017-05-18 08:57:26]

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

[commit-bot: I haz the power, 2017-05-18 08:57:45]

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, 2017-05-18 13:16:29]

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

[commit-bot: I haz the power, 2017-05-18 13:16:30]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[hajimehoshi, 2017-05-24 09:19:10]

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

[commit-bot: I haz the power, 2017-05-24 09:19:38]

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, 2017-05-24 09:22:02]

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

[commit-bot: I haz the power, 2017-05-24 09:22:03]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hajimehoshi, 2017-05-24 11:05:27]

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

[commit-bot: I haz the power, 2017-05-24 11:05:46]

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, 2017-05-24 13:01:50]

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

[commit-bot: I haz the power, 2017-05-24 13:01:52]

Dry run: This issue passed the CQ dry run.