memory-infra: Move dump level check to observer and rename session state

memory-infra: Move dump level check to observer and rename session state

- Rename session_state to heap_profiler_serialization_state in MemoryDumpManager.
- Move IsDumpModeAllowed check from MemoryDumpManager to MemoryTracingObserver.

This further decouples MemoryDumpManager from tracing.

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

[hjd, 2017-05-05 11:41:43]

Description was changed from

==========
memory-infra: Move dump level check to observer and rename session state
==========

to

==========
memory-infra: Move dump level check to observer and rename session state

- Rename session_state to heap_profiler_serialization_state in
MemoryDumpManager.
- Move IsDumpModeAllowed check from MemoryDumpManager to MemoryTracingObserver.

This further decouples MemoryDumpManager from tracing.

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

[hjd, 2017-05-05 11:41:43]

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

[hjd, 2017-05-05 11:42:28]

ptal, thanks 😊

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

https://codereview.chromium.org/2861133002/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:1261:
RegisterDumpProvider(&mdp, ThreadTaskRunnerHandle::Get());
We now need an mdp to hit the code path where the dump will be rejected now
(since rejection happens just before serialization.

https://codereview.chromium.org/2861133002/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:1285: run_loop.Run();
Previously this was racy, we relied on the 6 background dumps executing before
the light and detailed dumps.

[Primiano Tucci (use gerrit), 2017-05-05 13:38:25]

Excellent, although I'd also rename the class (and the file name) in this CL.

Just minor comments below.

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

https://codereview.chromium.org/2861133002/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:773:
scoped_refptr<MemoryDumpSessionState> heap_profiler_serialization_state =
sg but can we move this to its own function in the next cls?

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

https://codereview.chromium.org/2861133002/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager.h:153: // Returns the
MemoryDumpSessionState object, which is shared by all the
I'd remove this comment and update the one below above the field in the private:
section

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

https://codereview.chromium.org/2861133002/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:166:
pmd->CreateAllocatorDump("example")->AddScalar(
do you need this or is this the leftover from some local debugging?

https://codereview.chromium.org/2861133002/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:413: // Checks that the
SharedSessionState object is acqually shared over time.
update the comment and the name of the TEST_F

https://codereview.chromium.org/2861133002/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:1261:
RegisterDumpProvider(&mdp, ThreadTaskRunnerHandle::Get());
On 2017/05/05 11:42:27, hjd wrote:
> We now need an mdp to hit the code path where the dump will be rejected now
> (since rejection happens just before serialization.

Acknowledged. Maybe add a comment to explain this in the code?

https://codereview.chromium.org/2861133002/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:1285: run_loop.Run();
On 2017/05/05 11:42:27, hjd wrote:
> Previously this was racy, we relied on the 6 background dumps executing before
> the light and detailed dumps.

Well I think that became just racy now. Before we wouldn't have started the
dumps, so we would have seen only the background dumps. Instead now they would
be interleaved (because we nack only after reaching the serialization point), so
this is now required.

https://codereview.chromium.org/2861133002/diff/20001/base/trace_event/memory...
File base/trace_event/memory_dump_session_state.h (right):

https://codereview.chromium.org/2861133002/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_session_state.h:5: #ifndef
BASE_TRACE_EVENT_MEMORY_DUMP_SESSION_STATE_H_
shouldn't we rename this to HeapProfilerSerializationState?
it feels a bit inconsistent to rename the variable but not the class.

https://codereview.chromium.org/2861133002/diff/20001/base/trace_event/memory...
File base/trace_event/memory_tracing_observer.cc (right):

https://codereview.chromium.org/2861133002/diff/20001/base/trace_event/memory...
base/trace_event/memory_tracing_observer.cc:68: memory_dump_config_ = nullptr;
.reset()? I thought you can't do this with unique_ptrs?

https://codereview.chromium.org/2861133002/diff/20001/base/trace_event/memory...
base/trace_event/memory_tracing_observer.cc:108: if (memory_dump_config_ ==
nullptr)
use the bool operator, i.e. if (!memory_dump_config_) . Ithink there was a
discussion about this on chromium-dev long time ago (I can't even recall the
rationale honestly, but I think that this was the TL;DR, maybe for code size?)

[hjd, 2017-05-05 14:50:21]

The CQ bit was checked by hjd@chromium.org

[hjd, 2017-05-05 14:50:22]

The CQ bit was unchecked by hjd@chromium.org

[hjd, 2017-05-05 14:50:24]

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

[commit-bot: I haz the power, 2017-05-05 14:50:44]

Dry run: CQ is trying da patch.

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

[hjd, 2017-05-05 14:50:54]

Updated to rename type, ptal thanks!

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

https://codereview.chromium.org/2861133002/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:166:
pmd->CreateAllocatorDump("example")->AddScalar(
On 2017/05/05 13:38:25, Primiano Tucci wrote:
> do you need this or is this the leftover from some local debugging?

Yeah, removed. Thanks!

https://codereview.chromium.org/2861133002/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:413: // Checks that the
SharedSessionState object is acqually shared over time.
On 2017/05/05 13:38:25, Primiano Tucci wrote:
> update the comment and the name of the TEST_F

Done.

https://codereview.chromium.org/2861133002/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:1285: run_loop.Run();
On 2017/05/05 13:38:25, Primiano Tucci wrote:
> On 2017/05/05 11:42:27, hjd wrote:
> > Previously this was racy, we relied on the 6 background dumps executing
before
> > the light and detailed dumps.
> 
> Well I think that became just racy now. Before we wouldn't have started the
> dumps, so we would have seen only the background dumps. Instead now they would
> be interleaved (because we nack only after reaching the serialization point),
so
> this is now required.

On master if I run the test on its own it hangs forever about 50% of the time.
(Although after your explanation I'm not sure why.)

https://codereview.chromium.org/2861133002/diff/20001/base/trace_event/memory...
File base/trace_event/memory_dump_session_state.h (right):

https://codereview.chromium.org/2861133002/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_session_state.h:5: #ifndef
BASE_TRACE_EVENT_MEMORY_DUMP_SESSION_STATE_H_
On 2017/05/05 13:38:25, Primiano Tucci wrote:
> shouldn't we rename this to HeapProfilerSerializationState?
> it feels a bit inconsistent to rename the variable but not the class.

I felt bad for dskiba's CL. Done.

https://codereview.chromium.org/2861133002/diff/20001/base/trace_event/memory...
File base/trace_event/memory_tracing_observer.cc (right):

https://codereview.chromium.org/2861133002/diff/20001/base/trace_event/memory...
base/trace_event/memory_tracing_observer.cc:68: memory_dump_config_ = nullptr;
On 2017/05/05 13:38:25, Primiano Tucci wrote:
> .reset()? I thought you can't do this with unique_ptrs?

Done.

https://codereview.chromium.org/2861133002/diff/20001/base/trace_event/memory...
base/trace_event/memory_tracing_observer.cc:108: if (memory_dump_config_ ==
nullptr)
On 2017/05/05 13:38:25, Primiano Tucci wrote:
> use the bool operator, i.e. if (!memory_dump_config_) . Ithink there was a
> discussion about this on chromium-dev long time ago (I can't even recall the
> rationale honestly, but I think that this was the TL;DR, maybe for code size?)

Done. I love that we use a static typed language where the compiler can remove
the cost of abstractions...

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

LGTM thanks

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

https://codereview.chromium.org/2861133002/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.h:222: // The trace-global session state.
update this comment (actually just move the one from the getter above to here)

[commit-bot: I haz the power, 2017-05-05 16:03:32]

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

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

Dry run: This issue passed the CQ dry run.

[hjd, 2017-05-05 16:10:33]

thanks!

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

https://codereview.chromium.org/2861133002/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.h:222: // The trace-global session state.
On 2017/05/05 15:59:23, Primiano Tucci wrote:
> update this comment (actually just move the one from the getter above to here)

Done.

https://codereview.chromium.org/2861133002/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.h:222: // The trace-global session state.
On 2017/05/05 15:59:23, Primiano Tucci wrote:
> update this comment (actually just move the one from the getter above to here)

Done.

[hjd, 2017-05-05 16:14:03]

hjd@chromium.org changed reviewers:
+ thakis@chromium.org

[hjd, 2017-05-05 16:15:50]

+thakis ptal for 2 line change in base/BUILD.gn. Thanks!

[Nico, 2017-05-05 16:19:02]

lgtm

[Primiano Tucci (use gerrit), 2017-05-05 16:59:24]

The CQ bit was checked by primiano@chromium.org

[Primiano Tucci (use gerrit), 2017-05-05 16:59:24]

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

[commit-bot: I haz the power, 2017-05-05 16:59:58]

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-05 18:09:31]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494003564249930,
"parent_rev": "914d3011abd8040a608aa533781c1ce6bbc40399", "commit_rev":
"b154afcbc07fcd0b31b1c59ef3c6c30c02bed13a"}

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

Description was changed from

==========
memory-infra: Move dump level check to observer and rename session state

- Rename session_state to heap_profiler_serialization_state in
MemoryDumpManager.
- Move IsDumpModeAllowed check from MemoryDumpManager to MemoryTracingObserver.

This further decouples MemoryDumpManager from tracing.

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

to

==========
memory-infra: Move dump level check to observer and rename session state

- Rename session_state to heap_profiler_serialization_state in
MemoryDumpManager.
- Move IsDumpModeAllowed check from MemoryDumpManager to MemoryTracingObserver.

This further decouples MemoryDumpManager from tracing.

BUG=703184

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

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

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

[ssid, 2017-05-05 18:18:34]

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

https://codereview.chromium.org/2861133002/diff/80001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:411:
!IsDumpModeAllowed(level_of_detail)) {
Hm , if we remove this check and also remove is_enabled(on other cl), this
optimization cannot be made anymore. ie, if some code requests a detailed dump
when tracing is not enabled, we will only realize that dump was not needed after
creating a global dump. This brings back the need for is_enabled? or we are just
going to do the work and ignore it?

[hjd, 2017-05-08 10:48:36]

On 2017/05/05 18:18:34, ssid wrote:
>
https://codereview.chromium.org/2861133002/diff/80001/base/trace_event/memory...
> File base/trace_event/memory_dump_manager.cc (left):
> 
>
https://codereview.chromium.org/2861133002/diff/80001/base/trace_event/memory...
> base/trace_event/memory_dump_manager.cc:411:
> !IsDumpModeAllowed(level_of_detail)) {
> Hm , if we remove this check and also remove is_enabled(on other cl), this
> optimization cannot be made anymore. ie, if some code requests a detailed dump
> when tracing is not enabled, we will only realize that dump was not needed
after
> creating a global dump. This brings back the need for is_enabled? or we are
just
> going to do the work and ignore it?

Yep I think so, this shouldn't actually happen often if things are working
correctly right?
When the trace observer lives only in the service we can have a new
requestGlobalDumpForTracing which reintroduces the check.
Lets discuss in the chat.

[Primiano Tucci (use gerrit), 2017-05-08 10:59:01]

On 2017/05/08 10:48:36, hjd wrote:
> On 2017/05/05 18:18:34, ssid wrote:
> >
>
https://codereview.chromium.org/2861133002/diff/80001/base/trace_event/memory...
> > File base/trace_event/memory_dump_manager.cc (left):
> > 
> >
>
https://codereview.chromium.org/2861133002/diff/80001/base/trace_event/memory...
> > base/trace_event/memory_dump_manager.cc:411:
> > !IsDumpModeAllowed(level_of_detail)) {
> > Hm , if we remove this check and also remove is_enabled(on other cl), this
> > optimization cannot be made anymore. ie, if some code requests a detailed
dump
> > when tracing is not enabled, we will only realize that dump was not needed
> after
> > creating a global dump. This brings back the need for is_enabled? or we are
> just
> > going to do the work and ignore it?
> 
> Yep I think so, this shouldn't actually happen often if things are working
> correctly right?

Yeah I agree. having the early check avoids only wasting CPU for something that
we'll drop on the floor at the end. but as hjd@ says, this should be pretty
unlikely to happen, so if we waste some CPU for an unlikely event I think it's
fine.