memory instrumentation service: minor cleanups

memory instrumentation service: minor cleanups

Minor cleanups lefts from the previous CL in preparation
of larger upcoming refactoring.

BUG=703184
Review-Url: https://codereview.chromium.org/2891113003
Cr-Commit-Position: refs/heads/master@{#473436}
Committed: https://chromium.googlesource.com/chromium/src/+/4d1e2198851b4731e2c19fa6d36fcb0329f806fd

[Primiano Tucci (use gerrit), 2017-05-18 23:11:49]

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

[Primiano Tucci (use gerrit), 2017-05-18 23:12:07]

Description was changed from

==========
memory instrumentation service: minor cleanups

Minor cleanups lefts from the previous CL

BUG=703184
==========

to

==========
memory instrumentation service: minor cleanups

Minor cleanups lefts from the previous CL in preparation
of larger upcoming refactoring.

BUG=703184
==========

[commit-bot: I haz the power, 2017-05-18 23:13:33]

Dry run: CQ is trying da patch.

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

[Primiano Tucci (use gerrit), 2017-05-18 23:37:29]

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

[commit-bot: I haz the power, 2017-05-18 23:38:49]

Dry run: CQ is trying da patch.

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

[Primiano Tucci (use gerrit), 2017-05-18 23:49:03]

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

[commit-bot: I haz the power, 2017-05-18 23:49:43]

Dry run: CQ is trying da patch.

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

[Primiano Tucci (use gerrit), 2017-05-18 23:53:26]

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

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

Dry run: CQ is trying da patch.

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

[Primiano Tucci (use gerrit), 2017-05-19 00:02:51]

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

[commit-bot: I haz the power, 2017-05-19 00:05:22]

Dry run: CQ is trying da patch.

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

[Primiano Tucci (use gerrit), 2017-05-19 00:10:44]

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

[commit-bot: I haz the power, 2017-05-19 00:11:45]

Dry run: CQ is trying da patch.

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

[Primiano Tucci (use gerrit), 2017-05-19 00:31:02]

primiano@chromium.org changed reviewers:
+ erikchen@chromium.org, fmeawad@chromium.org

[fmeawad, 2017-05-19 00:37:25]

lgtm

[DmitrySkiba, 2017-05-19 00:40:21]

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

[DmitrySkiba, 2017-05-19 00:40:21]

https://codereview.chromium.org/2891113003/diff/100001/services/resource_coor...
File services/resource_coordinator/memory/coordinator/coordinator_impl.h
(right):

https://codereview.chromium.org/2891113003/diff/100001/services/resource_coor...
services/resource_coordinator/memory/coordinator/coordinator_impl.h:104:
std::map<mojom::ProcessLocalDumpManager*, std::unique_ptr<ClientInfo>>
What is the reason for not storing ClientInfo by value?

[Primiano Tucci (use gerrit), 2017-05-19 00:54:05]

https://codereview.chromium.org/2891113003/diff/100001/services/resource_coor...
File services/resource_coordinator/memory/coordinator/coordinator_impl.h
(right):

https://codereview.chromium.org/2891113003/diff/100001/services/resource_coor...
services/resource_coordinator/memory/coordinator/coordinator_impl.h:104:
std::map<mojom::ProcessLocalDumpManager*, std::unique_ptr<ClientInfo>>
On 2017/05/19 00:40:21, DmitrySkiba wrote:
> What is the reason for not storing ClientInfo by value?

because:
1) the ProcessLocalDumpManagerPtr is non copiable (is like a a unique-ptr)
2) I could make ClientInfo move-only, but that would imply:
  - more code, less readability
  - the deep string move of all the strings in the Identity struct. (and no,
given our fragmented story of c++ libs  std::move(std::string) doesn't the right
thing on all platforms)

[DmitrySkiba, 2017-05-19 01:19:50]

https://codereview.chromium.org/2891113003/diff/100001/services/resource_coor...
File services/resource_coordinator/memory/coordinator/coordinator_impl.cc
(right):

https://codereview.chromium.org/2891113003/diff/100001/services/resource_coor...
services/resource_coordinator/memory/coordinator/coordinator_impl.cc:155: size_t
num_deleted = clients_.erase(process_manager);
As I understand, this deletes process_manager, so all code below works with a
dead pointer (including dead pointer in pending_clients_for_current_dump_). It
might be a good idea to move this line to the end, just to protect from future
surprises.

https://codereview.chromium.org/2891113003/diff/100001/services/resource_coor...
File services/resource_coordinator/memory/coordinator/coordinator_impl.h
(right):

https://codereview.chromium.org/2891113003/diff/100001/services/resource_coor...
services/resource_coordinator/memory/coordinator/coordinator_impl.h:104:
std::map<mojom::ProcessLocalDumpManager*, std::unique_ptr<ClientInfo>>
On 2017/05/19 00:54:05, Primiano (slow - traveling) wrote:
> On 2017/05/19 00:40:21, DmitrySkiba wrote:
> > What is the reason for not storing ClientInfo by value?
> 
> because:
> 1) the ProcessLocalDumpManagerPtr is non copiable (is like a a unique-ptr)
> 2) I could make ClientInfo move-only, but that would imply:
>   - more code, less readability
>   - the deep string move of all the strings in the Identity struct. (and no,
> given our fragmented story of c++ libs  std::move(std::string) doesn't the
right
> thing on all platforms) 

I see your point (although ClientInfo(ClientInfo&&) = default doesn't seems like
a lot of code). But I'm curious about the last part - on which platform
std::string doesn't do the right thing when moved?

[commit-bot: I haz the power, 2017-05-19 01:49:41]

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

[commit-bot: I haz the power, 2017-05-19 01:49:42]

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

[erikchen, 2017-05-19 16:21:37]

lgtm

https://codereview.chromium.org/2891113003/diff/100001/services/resource_coor...
File services/resource_coordinator/memory/coordinator/coordinator_impl.h
(right):

https://codereview.chromium.org/2891113003/diff/100001/services/resource_coor...
services/resource_coordinator/memory/coordinator/coordinator_impl.h:70:
QueuedMemoryDumpRequest(const base::trace_event::MemoryDumpRequestArgs args,
should these be passed by const&?

[Primiano Tucci (use gerrit), 2017-05-19 16:30:21]

On 2017/05/19 01:19:50, DmitrySkiba wrote:
>
https://codereview.chromium.org/2891113003/diff/100001/services/resource_coor...
> File services/resource_coordinator/memory/coordinator/coordinator_impl.cc
> (right):
> 
>
https://codereview.chromium.org/2891113003/diff/100001/services/resource_coor...
> services/resource_coordinator/memory/coordinator/coordinator_impl.cc:155:
size_t
> num_deleted = clients_.erase(process_manager);
> As I understand, this deletes process_manager, so all code below works with a
> dead pointer (including dead pointer in pending_clients_for_current_dump_). It
> might be a good idea to move this line to the end, just to protect from future
> surprises.
> 
>
https://codereview.chromium.org/2891113003/diff/100001/services/resource_coor...
> File services/resource_coordinator/memory/coordinator/coordinator_impl.h
> (right):
> 
>
https://codereview.chromium.org/2891113003/diff/100001/services/resource_coor...
> services/resource_coordinator/memory/coordinator/coordinator_impl.h:104:
> std::map<mojom::ProcessLocalDumpManager*, std::unique_ptr<ClientInfo>>
> On 2017/05/19 00:54:05, Primiano (slow - traveling) wrote:
> > On 2017/05/19 00:40:21, DmitrySkiba wrote:
> > > What is the reason for not storing ClientInfo by value?
> > 
> > because:
> > 1) the ProcessLocalDumpManagerPtr is non copiable (is like a a unique-ptr)
> > 2) I could make ClientInfo move-only, but that would imply:
> >   - more code, less readability
> >   - the deep string move of all the strings in the Identity struct. (and no,
> > given our fragmented story of c++ libs  std::move(std::string) doesn't the
> right
> > thing on all platforms) 
> 
> I see your point (although ClientInfo(ClientInfo&&) = default doesn't seems
like
> a lot of code). But I'm curious about the last part - on which platform
> std::string doesn't do the right thing when moved?

whichever uses inline storage for short strings (don't remember if that was
libcxx or sdlibc++)

[Primiano Tucci (use gerrit), 2017-05-19 16:31:39]

https://codereview.chromium.org/2891113003/diff/100001/services/resource_coor...
File services/resource_coordinator/memory/coordinator/coordinator_impl.h
(right):

https://codereview.chromium.org/2891113003/diff/100001/services/resource_coor...
services/resource_coordinator/memory/coordinator/coordinator_impl.h:70:
QueuedMemoryDumpRequest(const base::trace_event::MemoryDumpRequestArgs args,
On 2017/05/19 16:21:37, erikchen wrote:
> should these be passed by const&?

aha, well spotted. Done.

[Primiano Tucci (use gerrit), 2017-05-19 16:31:43]

The CQ bit was checked by primiano@chromium.org

[Primiano Tucci (use gerrit), 2017-05-19 16:31:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
erikchen@chromium.org, fmeawad@chromium.org
Link to the patchset: https://codereview.chromium.org/2891113003/#ps120001
(title: "const&")

[commit-bot: I haz the power, 2017-05-19 16:33:16]

CQ is trying da patch.

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

[DmitrySkiba, 2017-05-19 16:36:10]

The CQ bit was unchecked by dskiba@chromium.org

[Primiano Tucci (use gerrit), 2017-05-19 16:54:54]

https://codereview.chromium.org/2891113003/diff/100001/services/resource_coor...
File services/resource_coordinator/memory/coordinator/coordinator_impl.cc
(right):

https://codereview.chromium.org/2891113003/diff/100001/services/resource_coor...
services/resource_coordinator/memory/coordinator/coordinator_impl.cc:155: size_t
num_deleted = clients_.erase(process_manager);
On 2017/05/19 01:19:50, DmitrySkiba wrote:
> As I understand, this deletes process_manager, so all code below works with a
> dead pointer (including dead pointer in pending_clients_for_current_dump_). It
> might be a good idea to move this line to the end, just to protect from future
> surprises.

sorry, missed this comment.
makes perfect sense. done.

[Primiano Tucci (use gerrit), 2017-05-19 16:55:30]

The CQ bit was checked by primiano@chromium.org

[Primiano Tucci (use gerrit), 2017-05-19 16:55:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
erikchen@chromium.org, fmeawad@chromium.org
Link to the patchset: https://codereview.chromium.org/2891113003/#ps140001
(title: "dskiba suggestion")

[commit-bot: I haz the power, 2017-05-19 16:56:08]

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-19 18:39:12]

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

[commit-bot: I haz the power, 2017-05-19 18:39:13]

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

[Primiano Tucci (use gerrit), 2017-05-19 22:02:59]

The CQ bit was checked by primiano@chromium.org

[commit-bot: I haz the power, 2017-05-19 22:04: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, 2017-05-20 00:15:02]

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

[commit-bot: I haz the power, 2017-05-20 00:15:03]

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

[Primiano Tucci (use gerrit), 2017-05-20 00:23:00]

The CQ bit was checked by primiano@chromium.org

[Primiano Tucci (use gerrit), 2017-05-20 00:23:27]

The CQ bit was unchecked by primiano@chromium.org

[commit-bot: I haz the power, 2017-05-20 00:23:33]

CQ is trying da patch.

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

[Primiano Tucci (use gerrit), 2017-05-20 00:26:09]

The CQ bit was checked by primiano@chromium.org

[Primiano Tucci (use gerrit), 2017-05-20 00:26:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
erikchen@chromium.org, fmeawad@chromium.org
Link to the patchset: https://codereview.chromium.org/2891113003/#ps160001
(title: "remove dcheck")

[commit-bot: I haz the power, 2017-05-20 00:26:52]

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-20 04:02:57]

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

[commit-bot: I haz the power, 2017-05-20 04:02:58]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Primiano Tucci (use gerrit), 2017-05-20 13:35:24]

The CQ bit was checked by primiano@chromium.org

[commit-bot: I haz the power, 2017-05-20 13:35:31]

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-20 14:15:21]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1495287324255930,
"parent_rev": "5553b9f830b3f917d2500762a9455cb1f28dd0b1", "commit_rev":
"4d1e2198851b4731e2c19fa6d36fcb0329f806fd"}

[commit-bot: I haz the power, 2017-05-20 14:15:35]

Description was changed from

==========
memory instrumentation service: minor cleanups

Minor cleanups lefts from the previous CL in preparation
of larger upcoming refactoring.

BUG=703184
==========

to

==========
memory instrumentation service: minor cleanups

Minor cleanups lefts from the previous CL in preparation
of larger upcoming refactoring.

BUG=703184

Review-Url: https://codereview.chromium.org/2891113003
Cr-Commit-Position: refs/heads/master@{#473436}
Committed:
https://chromium.googlesource.com/chromium/src/+/4d1e2198851b4731e2c19fa6d36f...
==========

[commit-bot: I haz the power, 2017-05-20 14:15:37]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/4d1e2198851b4731e2c19fa6d36f...