[tracing] Simplify and improve thread-hopping logic of memory-infra

[tracing] Simplify and improve thread-hopping logic of memory-infra

By contract MemoryDumpManager invokes the MemoryDumpProvider's
OnMemoryDump() method on the thread specified (by the MDP) in the
RegisterDumpProvider() call.
At the same time, it guarantees that at most one MDP is enabled at any
time. This is to avoid to have the MDPs dealing with thread-safety.

Assuming to have the following situation:
MDP_1 @ Thread_A, MDP_2 @ Thread_B, MDP_3 @ Thread_A, MDP_4 @ Thread_B

The way ProcessMemoryDump would have acted before this change is:
  CreateProcessDump():
    PostTask(Thread_A, ContinueAsyncProcessDump(MDP_1))
    PostTask(Thread_B, ContinueAsyncProcessDump(MDP_2))
    PostTask(Thread_A, ContinueAsyncProcessDump(MDP_3))
    PostTask(Thread_B, ContinueAsyncProcessDump(MDP_4))
  On each ContinueAsyncProcessDump() in each thread:
    AutoLock(lock_)
      MDP_X->OnMemoryDump()
      if (last_dump)
        FinalizeAndAddToTrace()

This strategy has two problems:
 1) performance-wise: it is suboptimal, as it performs more thread hops
    than necessary (ABAB instead of just AB).
 2) It is deadlock prone: essentially this spawns N tasks on various
    threads and then linearizes them on the |lock_|. This can cause
    deadlocks if other tasks on the message loops have dependencies.
    This is the case of crbug.com/505725, which is fixed by this CL.

The new strategy is conceptually simpler, smarter and avoids
inter-locking the message loops, as follows:
- The MDPs are kept in order according to their thread affinity.
- When CreateProcessDump() is invoked, only one task (the first in the
  set) is posted.
- Each task, which corresponds to an invocation of
  ContinueAsyncProcessDump() invokes the dump provider without taking
  the lock_. When done posts the next task (or finalizes the dump).
By posting one task at a time we still guarantee that the OnMemoryDump
calls are linearized (per PMD) even if the lock is not taken.
Furthermore, given the order of the MDP set, the thread hopping is
reduced to the minimum.

The only tricky part of this CL is dealing with changes of the dump
providers (i.e. calls to (Un)RegisterDumpProvider) while an
asynchronous process dump is in progress. In this (very unlikely)
case the choice is to abort the current dump.

BUG=497144, 505725
Committed: https://crrev.com/ca7a49016df9b30f326811246d70f7472dea4d66
Cr-Commit-Position: refs/heads/master@{#337580}

[Primiano Tucci (use gerrit), 2015-07-02 15:36:33]

primiano@chromium.org changed reviewers:
+ dsinclair@chromium.org, petrcermak@chromium.org

[Primiano Tucci (use gerrit), 2015-07-02 15:37:26]

Dan, Petr, can I ask all your four eyes  on this?
I will write tests asynchronously but I'd like to get a review in the meanwhile,
as this fixes two nasty bugs.
Many thanks,
Primiano

[Primiano Tucci (use gerrit), 2015-07-02 19:27:03]

Patchset #3 (id:40001) has been deleted

[petrcermak, 2015-07-03 11:04:17]

This is some really great work!!! Just a couple of mostly style comments...

Thanks,
Petr

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:174: << " Please file a crbug.";
nit: Double space here (not your patch but I thought I'd point it out anyway)

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:177: // UnregisterDumpProvider is called
while the trace is enabled.
nit: It seems to me that this comment is outdated. Even if the trace is enabled,
the unregistration needs to happen, right?

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:231: // PMD, regardless of status of the
|lock_|. |lock_| here is used purely to
nit: s/here is used/is used here/

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:239: // 2) Invoke the dump provider's
OnMemoryDump() (% skip).
nit: I'd say "unless skipped" instead of "% skip" just to make it a little
clearer.

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:241: //  - unregister the dump provider
if failed too many times consecutively.
nit: "if IT failed"

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:243: //  - If this was the last hop add
to the trace and finalize (invoke callback).
nit2: I think you should say *what* is added to the trace.

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:243: //  - If this was the last hop add
to the trace and finalize (invoke callback).
nit1: there should be a comma after "If this was the last hop"

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:247: // Initalizes the
ThreadLocalEventBuffer, to guarantee that the TRACE_EVENTs
nit: No need for a comma here.

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:256: enum { NONE, THREAD_GONE,
DUMPER_FAILED } log_warning = NONE;
I think that this definition should be moved to line 310. This way, it looks
like it belongs to the block below, but it never touches it.

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:259: // In the unlikely event that a
dump provider was unregister while dumping
s/unregister/unregistered/

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:259: // In the unlikely event that a
dump provider was unregister while dumping
nit: There should be a comma before "abort the dump" as well.

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:261: // Registration of a new MDP,
instead, is safe as per std::set<> contract.
nit: I think that "on the other hand" would work better than "instead" here.

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:281: // It's time to hop on another
thread.
nit: s/on/onto/

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:293: FROM_HERE,
Bind(&MemoryDumpManager::ContinueAsyncProcessDump,
Just a thought: Could you store the Bind in a local variable so that it wouldn't
destroy the arguments (and you wouldn't need to copy them):

auto bind = Bind(...)
const bool did_post_task = mdp_info->task_runner->PostTask(FROM_HERE, bind);

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:298: // The thread is gone. At this
point the best thing we can do is disabling
nit: you say "is disablING ... and abort ...". It should probably be "is TO
disable ... and abort ...".

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:334: "its thread is gone";
nit: missing full stop at the end of the message.

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:336: LOG(ERROR) << "The memory dumper
failed, possibly due to sandboxing "
nit: "The memory dumper" -> "A memory dump provider" (to have consistent LOG
messages)

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:342:
FinalizeDumpAndAddToTrace(pmd_async_state.Pass());
I think you could do "return FinalizeDumpAndAddToTrace(pmd_async_state.Pass());"
to match the style of line 263.

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:375:
pmd_async_state->callback.Run(pmd_async_state->req_args.dump_guid, true);
nit: Could you please add "/* success */" after "true" like on line 391?

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:389: // invoking the callback while
holing the lock_.
s/holing/holding/

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.h (right):

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.h:105: // deliberately copyable, in order
to allow to be used as std::set value.
nit: I think it should be either "to allow IT to be used", or "to BE allowED to
be used"

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.h:112: // Define a total order based on the
affinity thread (i.e. |task_runner|),
Don't you mean "thread affinity" (swap word order)? In that case, there should
be no preceding "the" and the "(i.e. |task_runner|)" should probably be in the
middle of the statement "thread (i.e. |task_runner|) affinity", which is quite
unreadable, or most probably "thread affinity (i.e. |task_runner| affinity)".

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.h:127: // Holds the state of a process
memory dump that needs to be carry over across
s/carry/carried/

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.h:128: // threads in order to fullfill an
asynchronous CreateProcessDump() request.
s/fullfill/fulfil/ (I find the correct spelling strange to be honest but it is
what it is).

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.h:145: // An iterator to the next dump
provider that should be invoked.
nit: Most of the fields are described as "The ...", so I'd suggest doing the
same here and for callback.

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
File base/trace_event/memory_dump_manager_unittest.cc (right):

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:300: // dumping and expect it
to take part to the already active tracing session.
s/take part to/take part in/

[Primiano Tucci (use gerrit), 2015-07-03 15:05:45]

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:174: << " Please file a crbug.";
On 2015/07/03 11:04:15, petrcermak wrote:
> nit: Double space here (not your patch but I thought I'd point it out anyway)

Done.

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:177: // UnregisterDumpProvider is called
while the trace is enabled.
On 2015/07/03 11:04:16, petrcermak wrote:
> nit: It seems to me that this comment is outdated. Even if the trace is
enabled,
> the unregistration needs to happen, right?

Correct, there is only one list now. Thanks :)

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:231: // PMD, regardless of status of the
|lock_|. |lock_| here is used purely to
On 2015/07/03 11:04:16, petrcermak wrote:
> nit: s/here is used/is used here/

Done.

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:239: // 2) Invoke the dump provider's
OnMemoryDump() (% skip).
On 2015/07/03 11:04:16, petrcermak wrote:
> nit: I'd say "unless skipped" instead of "% skip" just to make it a little
> clearer.

Done.

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:241: //  - unregister the dump provider
if failed too many times consecutively.
On 2015/07/03 11:04:16, petrcermak wrote:
> nit: "if IT failed"

Done.

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:243: //  - If this was the last hop add
to the trace and finalize (invoke callback).
On 2015/07/03 11:04:16, petrcermak wrote:
> nit2: I think you should say *what* is added to the trace.

Done.

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:243: //  - If this was the last hop add
to the trace and finalize (invoke callback).
On 2015/07/03 11:04:16, petrcermak wrote:
> nit2: I think you should say *what* is added to the trace.

Done.

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:247: // Initalizes the
ThreadLocalEventBuffer, to guarantee that the TRACE_EVENTs
On 2015/07/03 11:04:15, petrcermak wrote:
> nit: No need for a comma here.

Done.

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:256: enum { NONE, THREAD_GONE,
DUMPER_FAILED } log_warning = NONE;
On 2015/07/03 11:04:15, petrcermak wrote:
> I think that this definition should be moved to line 310. This way, it looks
> like it belongs to the block below, but it never touches it.

actually don't need it anymore at all. Just need one warning.

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:259: // In the unlikely event that a
dump provider was unregister while dumping
On 2015/07/03 11:04:15, petrcermak wrote:
> s/unregister/unregistered/

Done.

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:261: // Registration of a new MDP,
instead, is safe as per std::set<> contract.
On 2015/07/03 11:04:16, petrcermak wrote:
> nit: I think that "on the other hand" would work better than "instead" here.

Done.

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:281: // It's time to hop on another
thread.
On 2015/07/03 11:04:16, petrcermak wrote:
> nit: s/on/onto/

Done.

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:293: FROM_HERE,
Bind(&MemoryDumpManager::ContinueAsyncProcessDump,
On 2015/07/03 11:04:16, petrcermak wrote:
> Just a thought: Could you store the Bind in a local variable so that it
wouldn't
> destroy the arguments (and you wouldn't need to copy them):
> 
> auto bind = Bind(...)
> const bool did_post_task = mdp_info->task_runner->PostTask(FROM_HERE, bind);

Discussed this offline. Can't. THe problem is that the Bind takes ownership of
the state, which includes the task runner

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:298: // The thread is gone. At this
point the best thing we can do is disabling
On 2015/07/03 11:04:16, petrcermak wrote:
> nit: you say "is disablING ... and abort ...". It should probably be "is TO
> disable ... and abort ...".

Done.

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:334: "its thread is gone";
On 2015/07/03 11:04:16, petrcermak wrote:
> nit: missing full stop at the end of the message.

removed the entire warning.

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:336: LOG(ERROR) << "The memory dumper
failed, possibly due to sandboxing "
On 2015/07/03 11:04:15, petrcermak wrote:
> nit: "The memory dumper" -> "A memory dump provider" (to have consistent LOG
> messages)

Done.

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:342:
FinalizeDumpAndAddToTrace(pmd_async_state.Pass());
On 2015/07/03 11:04:16, petrcermak wrote:
> I think you could do "return
FinalizeDumpAndAddToTrace(pmd_async_state.Pass());"
> to match the style of line 263.

Indeed. Thanks!

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:375:
pmd_async_state->callback.Run(pmd_async_state->req_args.dump_guid, true);
On 2015/07/03 11:04:15, petrcermak wrote:
> nit: Could you please add "/* success */" after "true" like on line 391?

NOOOOOOOOOOOOOO an extra new line.
Pfff ok. Done

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:389: // invoking the callback while
holing the lock_.
On 2015/07/03 11:04:15, petrcermak wrote:
> s/holing/holding/

Holy lock! Done

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.h (right):

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.h:105: // deliberately copyable, in order
to allow to be used as std::set value.
On 2015/07/03 11:04:16, petrcermak wrote:
> nit: I think it should be either "to allow IT to be used", or "to BE allowED
to
> be used"

Done.

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.h:112: // Define a total order based on the
affinity thread (i.e. |task_runner|),
On 2015/07/03 11:04:16, petrcermak wrote:
> Don't you mean "thread affinity" (swap word order)? In that case, there should
> be no preceding "the" and the "(i.e. |task_runner|)" should probably be in the
> middle of the statement "thread (i.e. |task_runner|) affinity", which is quite
> unreadable, or most probably "thread affinity (i.e. |task_runner| affinity)".

^__^. Sold for thread (i.e. |task_runner|) affinity

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.h:127: // Holds the state of a process
memory dump that needs to be carry over across
On 2015/07/03 11:04:17, petrcermak wrote:
> s/carry/carried/

Done.

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.h:128: // threads in order to fullfill an
asynchronous CreateProcessDump() request.
On 2015/07/03 11:04:17, petrcermak wrote:
> s/fullfill/fulfil/ (I find the correct spelling strange to be honest but it is
> what it is).

Done.

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.h:145: // An iterator to the next dump
provider that should be invoked.
On 2015/07/03 11:04:16, petrcermak wrote:
> nit: Most of the fields are described as "The ...", so I'd suggest doing the
> same here and for callback.

Done.

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
File base/trace_event/memory_dump_manager_unittest.cc (right):

https://codereview.chromium.org/1222153004/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:300: // dumping and expect it
to take part to the already active tracing session.
On 2015/07/03 11:04:17, petrcermak wrote:
> s/take part to/take part in/

Done.

[petrcermak, 2015-07-03 15:20:09]

LGTM. Thanks!

Petr

[dsinclair, 2015-07-06 17:40:25]

lgtm w/ nit.

https://codereview.chromium.org/1222153004/diff/80001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/1222153004/diff/80001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:331: "(crbug.com/461788), disabling it
for current process. "
nit: s/disabling it/disabling memory dumper/

I thought you were talking about disabling the sandbox for a moment.

[Primiano Tucci (use gerrit), 2015-07-07 08:50:31]

The CQ bit was checked by primiano@chromium.org

[Primiano Tucci (use gerrit), 2015-07-07 08:50:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
dsinclair@chromium.org, petrcermak@chromium.org
Link to the patchset: https://codereview.chromium.org/1222153004/#ps100001
(title: "Re dsinclair #7")

[commit-bot: I haz the power, 2015-07-07 08:50:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1222153004/100001

[commit-bot: I haz the power, 2015-07-07 10:28:50]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-07-07 10:29:44]

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