[memory-infra] Add method to override importance of ownership edges

[memory-infra] Add method to override importance of ownership edges

Introduce ProcessMemoryDump::AddOverridableOwnershipEdge which adds
edges that can be overriden by another edge by called AddOwnershipEdge
with a different importance.

BUG=604726
Review-Url: https://codereview.chromium.org/2911263003
Cr-Commit-Position: refs/heads/master@{#477554}
Committed: https://chromium.googlesource.com/chromium/src/+/dd9170d74a1072243071753b7fa078e6794e534c

[ssid, 2017-05-31 04:08:27]

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

[ssid, 2017-05-31 04:08:27]

NOT for review. See if this makes sense from the discussion. Sending to clarify
what is the plan.

[hajimehoshi, 2017-05-31 05:45:51]

https://codereview.chromium.org/2911263003/diff/1/base/trace_event/process_me...
File base/trace_event/process_memory_dump.h (right):

https://codereview.chromium.org/2911263003/diff/1/base/trace_event/process_me...
base/trace_event/process_memory_dump.h:57:
std::unordered_map<MemoryAllocatorDumpGuid, MemoryAllocatorDumpEdge>;
Just std::map might be better (See /base/containers/README.md).

BTW, what would happen if there are two or more edges from one source to
different targets?

[ssid, 2017-06-02 23:14:47]

https://codereview.chromium.org/2911263003/diff/1/base/trace_event/process_me...
File base/trace_event/process_memory_dump.h (right):

https://codereview.chromium.org/2911263003/diff/1/base/trace_event/process_me...
base/trace_event/process_memory_dump.h:57:
std::unordered_map<MemoryAllocatorDumpGuid, MemoryAllocatorDumpEdge>;
On 2017/05/31 05:45:51, hajimehoshi wrote:
> Just std::map might be better (See /base/containers/README.md).
> 
Done

> BTW, what would happen if there are two or more edges from one source to
> different targets?
This is not supported. A dump cannot own 2 dumps.

[ssid, 2017-06-02 23:14:57]

Description was changed from

==========
[WIP] [memory-infra] Add method to override importance of ownership edges

Introduce ProcessMemoryDump::AddOverridableOwnershipEdge which adds
edges that can be overriden by another edge by called AddOwnershipEdge
with a different importance.

BUG=
==========

to

==========
[memory-infra] Add method to override importance of ownership edges

Introduce ProcessMemoryDump::AddOverridableOwnershipEdge which adds
edges that can be overriden by another edge by called AddOwnershipEdge
with a different importance.

BUG=
==========

[ssid, 2017-06-02 23:15:26]

Description was changed from

==========
[memory-infra] Add method to override importance of ownership edges

Introduce ProcessMemoryDump::AddOverridableOwnershipEdge which adds
edges that can be overriden by another edge by called AddOwnershipEdge
with a different importance.

BUG=
==========

to

==========
[memory-infra] Add method to override importance of ownership edges

Introduce ProcessMemoryDump::AddOverridableOwnershipEdge which adds
edges that can be overriden by another edge by called AddOwnershipEdge
with a different importance.

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

[ssid, 2017-06-02 23:18:29]

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

[ssid, 2017-06-02 23:18:47]

Primiano, ptal thanks

[commit-bot: I haz the power, 2017-06-02 23:19:12]

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-06-03 02:21:36]

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

[commit-bot: I haz the power, 2017-06-03 02:21:36]

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-06-05 10:06:24]

non-owner lgtm

[Primiano Tucci (use gerrit), 2017-06-05 15:23:39]

Makes sense % 1 bug in the impl, I believe (see note below).
I honestly wonder whether we are just making this too defensive and we should
just allow to override edges by default?
Are there downsides for that?

https://codereview.chromium.org/2911263003/diff/20001/base/trace_event/proces...
File base/trace_event/process_memory_dump.cc (right):

https://codereview.chromium.org/2911263003/diff/20001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:330: value->SetString("source",
edge.first.ToString());
since the edge has still the |source| I think this code would stay more readable
if you:

- name the argument of the loop an explict iterator (for const auto& it : ...)
- add a const MemoryAllocatorDumpEdge& edge = it.second; here
- keep doing edge.(like it was before)

https://codereview.chromium.org/2911263003/diff/20001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:342:
DCHECK(allocator_dumps_edges_.find(source) == allocator_dumps_edges_.end() ||
allocator_dump_edges.count(source) == 0 is more concise (and performance I
think, as it doesn't have to return  an iterator)

https://codereview.chromium.org/2911263003/diff/20001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:363:
DCHECK(!allocator_dumps_edges_[source].overridable);
isn't this branch forgetting to actually override the entry? 
I mean, how is this supposed to work to override an edge that has been already
created?

https://codereview.chromium.org/2911263003/diff/20001/base/trace_event/proces...
File base/trace_event/process_memory_dump.h (right):

https://codereview.chromium.org/2911263003/diff/20001/base/trace_event/proces...
base/trace_event/process_memory_dump.h:58: // the target.
"to the target" part  in the comment  is false :) (but the code makes sense).
I think you just want to say "indexed by source allocator GUID"

[ssid, 2017-06-05 17:03:29]

On 2017/06/05 15:23:39, Primiano Tucci wrote:
> Makes sense % 1 bug in the impl, I believe (see note below).
> I honestly wonder whether we are just making this too defensive and we should
> just allow to override edges by default?
> Are there downsides for that?


I did not like the idea of all edges being overridable because we will never
know which edges make it to final trace in all the cases.
In this model we specify which edges can be overridden and any edge that is
created using a normal AddOwnershipEdge function will override it. It is easier
to justify in the latter case if multiple dump providers create edge then we do
not create "weak" edge if normal one exists. Also because we have the concept of
"weak" dumps which work similarly, so familiar.

The part about being defensive is just so that we do not end up in situation
where people use the api in wrong way that it's not meant to be and we work for
ages to figure out how to get it right after code lands (like the case of edges
between global dumps). That is why I also made this new API private (hope you
noticed). This private api will only be used by
ProcessMemoryDump::CreateSharedMemoryOwnershipEdge which will be added by next
cl. If we need this for later, we can make it public.

https://codereview.chromium.org/2911263003/diff/20001/base/trace_event/proces...
File base/trace_event/process_memory_dump.cc (right):

https://codereview.chromium.org/2911263003/diff/20001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:330: value->SetString("source",
edge.first.ToString());
On 2017/06/05 15:23:39, Primiano Tucci wrote:
> since the edge has still the |source| I think this code would stay more
readable
> if you:
> 
> - name the argument of the loop an explict iterator (for const auto& it : ...)
> - add a const MemoryAllocatorDumpEdge& edge = it.second; here
> - keep doing edge.(like it was before)

Fixed.

https://codereview.chromium.org/2911263003/diff/20001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:342:
DCHECK(allocator_dumps_edges_.find(source) == allocator_dumps_edges_.end() ||
On 2017/06/05 15:23:39, Primiano Tucci wrote:
> allocator_dump_edges.count(source) == 0 is more concise (and performance I
> think, as it doesn't have to return  an iterator)

Done.

https://codereview.chromium.org/2911263003/diff/20001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:363:
DCHECK(!allocator_dumps_edges_[source].overridable);
On 2017/06/05 15:23:39, Primiano Tucci wrote:
> isn't this branch forgetting to actually override the entry? 
> I mean, how is this supposed to work to override an edge that has been already
> created?

Actually I went for the model where we specify the edges that can be overridden
rather than a API that does "OverrideOwnershipEdge". I just didn't want to make
all edges overridable and we will be confused when reading the code. Added
comment here.

https://codereview.chromium.org/2911263003/diff/20001/base/trace_event/proces...
File base/trace_event/process_memory_dump.h (right):

https://codereview.chromium.org/2911263003/diff/20001/base/trace_event/proces...
base/trace_event/process_memory_dump.h:58: // the target.
On 2017/06/05 15:23:39, Primiano Tucci wrote:
> "to the target" part  in the comment  is false :) (but the code makes sense).
> I think you just want to say "indexed by source allocator GUID"

Yeah, I was looking for this phrase!

[Primiano Tucci (use gerrit), 2017-06-06 18:50:50]

lgtm with minor comments. you rock!

https://codereview.chromium.org/2911263003/diff/60001/base/trace_event/proces...
File base/trace_event/process_memory_dump.cc (right):

https://codereview.chromium.org/2911263003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:345: << "Cannot add multiple ownership
edges between same allocator dumps";
I think the dcheck itself is quite self-explicative.

https://codereview.chromium.org/2911263003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:360: if
(allocator_dumps_edges_.find(source) == allocator_dumps_edges_.end()) {
count()

https://codereview.chromium.org/2911263003/diff/60001/base/trace_event/proces...
File base/trace_event/process_memory_dump_unittest.cc (right):

https://codereview.chromium.org/2911263003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump_unittest.cc:206:
pmd->AddOverridableOwnershipEdge(child2_dump->guid(), shm_dump2->guid(),
can you have some coverage for the overridable being >importance than the actual
one,?

[ssid, 2017-06-07 01:16:22]

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

[ssid, 2017-06-07 01:16:33]

thanks fixed.

https://codereview.chromium.org/2911263003/diff/60001/base/trace_event/proces...
File base/trace_event/process_memory_dump.cc (right):

https://codereview.chromium.org/2911263003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:345: << "Cannot add multiple ownership
edges between same allocator dumps";
On 2017/06/06 18:50:50, Primiano Tucci wrote:
> I think the dcheck itself is quite self-explicative.

removed.

https://codereview.chromium.org/2911263003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:360: if
(allocator_dumps_edges_.find(source) == allocator_dumps_edges_.end()) {
On 2017/06/06 18:50:49, Primiano Tucci wrote:
> count()

ouch, sorry.

https://codereview.chromium.org/2911263003/diff/60001/base/trace_event/proces...
File base/trace_event/process_memory_dump_unittest.cc (right):

https://codereview.chromium.org/2911263003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump_unittest.cc:206:
pmd->AddOverridableOwnershipEdge(child2_dump->guid(), shm_dump2->guid(),
On 2017/06/06 18:50:50, Primiano Tucci wrote:
> can you have some coverage for the overridable being >importance than the
actual
> one,?

Done.

[commit-bot: I haz the power, 2017-06-07 01:16:47]

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-06-07 03:45:12]

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

[commit-bot: I haz the power, 2017-06-07 03:45:14]

Dry run: This issue passed the CQ dry run.

[ssid, 2017-06-07 05:12:24]

The CQ bit was checked by ssid@chromium.org

[ssid, 2017-06-07 05:12:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
hajimehoshi@chromium.org, primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/2911263003/#ps80001
(title: "Fixes.")

[commit-bot: I haz the power, 2017-06-07 05:12:45]

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-06-07 05:17:42]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1496812344244910,
"parent_rev": "ac940cb6160ade6899f326ddde2d180b14e02f4f", "commit_rev":
"dd9170d74a1072243071753b7fa078e6794e534c"}

[commit-bot: I haz the power, 2017-06-07 05:17:52]

Description was changed from

==========
[memory-infra] Add method to override importance of ownership edges

Introduce ProcessMemoryDump::AddOverridableOwnershipEdge which adds
edges that can be overriden by another edge by called AddOwnershipEdge
with a different importance.

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

to

==========
[memory-infra] Add method to override importance of ownership edges

Introduce ProcessMemoryDump::AddOverridableOwnershipEdge which adds
edges that can be overriden by another edge by called AddOwnershipEdge
with a different importance.

BUG=604726

Review-Url: https://codereview.chromium.org/2911263003
Cr-Commit-Position: refs/heads/master@{#477554}
Committed:
https://chromium.googlesource.com/chromium/src/+/dd9170d74a1072243071753b7fa0...
==========

[commit-bot: I haz the power, 2017-06-07 05:17:59]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/dd9170d74a1072243071753b7fa0...