[Memory-UMA] Add ProcessMap, an Identity to Pid map

[Memory-UMA] Add ProcessMap, an Identity to Pid map

PROTOTYPE, DO NOT LAND

BUG=703184

[erikchen, 2017-05-12 17:56:14]

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

[erikchen, 2017-05-12 17:56:15]

https://codereview.chromium.org/2875673003/diff/40001/services/resource_coord...
File services/resource_coordinator/memory/coordinator/coordinator_impl.cc
(right):

https://codereview.chromium.org/2875673003/diff/40001/services/resource_coord...
services/resource_coordinator/memory/coordinator/coordinator_impl.cc:120:
process_manager.get(), std::move(process_manager)));
Given that we're already maintaining a map from mojom::ProcessLocalDumpManager*
to <something else>, wouldn't it be simplest to also add the identity to this
map?

That way Line 180 can be:
Look up in identity in process_managers_, then look up pid from identity in the
process_map_. This in turn makes process_map_ simpler and eliminates a redundant
structure.

https://codereview.chromium.org/2875673003/diff/40001/services/resource_coord...
services/resource_coordinator/memory/coordinator/coordinator_impl.cc:211: if
(content::ServiceManagerConnection::GetForProcess()) {
Given that you have a DCHECK on line 114, this should be

ServiceManagerConnection*c = content::ServiceManagerConnection::GetForProcess();
DCHECK(c);

https://codereview.chromium.org/2875673003/diff/40001/services/resource_coord...
services/resource_coordinator/memory/coordinator/coordinator_impl.cc:212:
service_manager::mojom::ServiceManagerPtr service_manager;
does this work? Don't you need to keep service_manager alive in order for the
callbacks to go through?

https://codereview.chromium.org/2875673003/diff/40001/services/resource_coord...
File services/resource_coordinator/memory/coordinator/coordinator_impl.h
(right):

https://codereview.chromium.org/2875673003/diff/40001/services/resource_coord...
services/resource_coordinator/memory/coordinator/coordinator_impl.h:96:
ProcessMap* process_map_;
unique_ptr

https://codereview.chromium.org/2875673003/diff/40001/services/resource_coord...
File services/resource_coordinator/memory/coordinator/process_map.cc (right):

https://codereview.chromium.org/2875673003/diff/40001/services/resource_coord...
services/resource_coordinator/memory/coordinator/process_map.cc:18:
binding_.WaitForIncomingMethodCall();
maybe I'm misreading, but doesn' this block the thread?
https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/binding.h?type=...

https://codereview.chromium.org/2875673003/diff/40001/services/resource_coord...
File services/resource_coordinator/memory/coordinator/process_map.h (right):

https://codereview.chromium.org/2875673003/diff/40001/services/resource_coord...
services/resource_coordinator/memory/coordinator/process_map.h:21: class
ProcessMap : public service_manager::mojom::ServiceManagerListener {
This class deserves a comment, maybe something like:

Maintains an unordered_map from mojom::ProcessLocalDumpManager* to
service_manager::Identity, and another from service_manager::Identity to
base::ProcessId pid;. This allows |pid| lookup by
mojom::ProcessLocalDumpManager.

Two questions:
1) Should instances_ be an unordered_map instead of a vector? Requires defining
a hash function, etc.
2) Should there be a cache from mojom::ProcessLocalDumpManager* to |pid|?
Probably not?

https://codereview.chromium.org/2875673003/diff/40001/services/resource_coord...
services/resource_coordinator/memory/coordinator/process_map.h:23:
ProcessMap(service_manager::mojom::ServiceManagerListenerRequest request);
explicit

https://codereview.chromium.org/2875673003/diff/40001/services/resource_coord...
services/resource_coordinator/memory/coordinator/process_map.h:26: void
AddProcess(mojom::ProcessLocalDumpManager* process_manager,
this makes the assumption that the destructor of a registered
mojom::ProcessLocalDumpManager is guaranteed to invoke RemoveProcess. Is this
the case? I can't tell.

[fmeawad, 2017-05-12 23:08:14]

https://codereview.chromium.org/2875673003/diff/40001/services/resource_coord...
File services/resource_coordinator/memory/coordinator/coordinator_impl.cc
(right):

https://codereview.chromium.org/2875673003/diff/40001/services/resource_coord...
services/resource_coordinator/memory/coordinator/coordinator_impl.cc:120:
process_manager.get(), std::move(process_manager)));
On 2017/05/12 17:56:15, erikchen wrote:
> Given that we're already maintaining a map from
mojom::ProcessLocalDumpManager*
> to <something else>, wouldn't it be simplest to also add the identity to this
> map?
> 
> That way Line 180 can be:
> Look up in identity in process_managers_, then look up pid from identity in
the
> process_map_. This in turn makes process_map_ simpler and eliminates a
redundant
> structure.

Done.

https://codereview.chromium.org/2875673003/diff/40001/services/resource_coord...
services/resource_coordinator/memory/coordinator/coordinator_impl.cc:211: if
(content::ServiceManagerConnection::GetForProcess()) {
On 2017/05/12 17:56:15, erikchen wrote:
> Given that you have a DCHECK on line 114, this should be
> 
> ServiceManagerConnection*c =
content::ServiceManagerConnection::GetForProcess();
> DCHECK(c);

Done.

https://codereview.chromium.org/2875673003/diff/40001/services/resource_coord...
services/resource_coordinator/memory/coordinator/coordinator_impl.cc:212:
service_manager::mojom::ServiceManagerPtr service_manager;
On 2017/05/12 17:56:15, erikchen wrote:
> does this work? Don't you need to keep service_manager alive in order for the
> callbacks to go through?

Unless I am reading this code:
https://cs.chromium.org/chromium/src/mash/task_viewer/task_viewer.cc?l=318
incorrectly, then no.

https://codereview.chromium.org/2875673003/diff/40001/services/resource_coord...
File services/resource_coordinator/memory/coordinator/coordinator_impl.h
(right):

https://codereview.chromium.org/2875673003/diff/40001/services/resource_coord...
services/resource_coordinator/memory/coordinator/coordinator_impl.h:96:
ProcessMap* process_map_;
On 2017/05/12 17:56:15, erikchen wrote:
> unique_ptr

Done.

https://codereview.chromium.org/2875673003/diff/40001/services/resource_coord...
File services/resource_coordinator/memory/coordinator/process_map.h (right):

https://codereview.chromium.org/2875673003/diff/40001/services/resource_coord...
services/resource_coordinator/memory/coordinator/process_map.h:21: class
ProcessMap : public service_manager::mojom::ServiceManagerListener {
On 2017/05/12 17:56:15, erikchen wrote:
> This class deserves a comment, maybe something like:
> 
> Maintains an unordered_map from mojom::ProcessLocalDumpManager* to
> service_manager::Identity, and another from service_manager::Identity to
> base::ProcessId pid;. This allows |pid| lookup by
> mojom::ProcessLocalDumpManager.
> 
> Two questions:
> 1) Should instances_ be an unordered_map instead of a vector? Requires
defining
> a hash function, etc.
> 2) Should there be a cache from mojom::ProcessLocalDumpManager* to |pid|?
> Probably not?

1) Yes
2) Should the cache live here? since we moved the identity out, does it still
make sense?

https://codereview.chromium.org/2875673003/diff/40001/services/resource_coord...
services/resource_coordinator/memory/coordinator/process_map.h:23:
ProcessMap(service_manager::mojom::ServiceManagerListenerRequest request);
On 2017/05/12 17:56:15, erikchen wrote:
> explicit

Done.

https://codereview.chromium.org/2875673003/diff/40001/services/resource_coord...
services/resource_coordinator/memory/coordinator/process_map.h:26: void
AddProcess(mojom::ProcessLocalDumpManager* process_manager,
On 2017/05/12 17:56:15, erikchen wrote:
> this makes the assumption that the destructor of a registered
> mojom::ProcessLocalDumpManager is guaranteed to invoke RemoveProcess. Is this
> the case? I can't tell.
> 

No longer needed since it was moved into the other map

https://codereview.chromium.org/2875673003/diff/60001/services/resource_coord...
File services/resource_coordinator/memory/coordinator/coordinator_impl.cc
(right):

https://codereview.chromium.org/2875673003/diff/60001/services/resource_coord...
services/resource_coordinator/memory/coordinator/coordinator_impl.cc:217:
content::ServiceManagerConnection::GetForProcess()
Service cannot use content, this call need to be moved out of here.