[tracing] Fix MemoryDumpManager periodic scheduler and improve tests

[tracing] Fix MemoryDumpManager periodic scheduler and improve tests

This a cleanup CL in preparation of next changes to memory-infra.
- Add much more coverage to the TraceConfig-related tests as this is
  very critical to avoid regressions in either the chrome://tracing
  UI and telemetry w.r.t. periodic dumps.
- Refactor the tests to use sane and readable in-line expectations
  instead of the awkward back and forths between test fixtures
  and MockDumpProvvider.
- Minor fix in the loogic which schedules periodic dumps in the
  MemoryDumpManager. If ligh_period=1ms and heavy_period=4ms,
  the sequence sholud be HLLLHLLL, not HLLLLHLLLL as it was
  before this change.
- Normalize the "virtual_size" column in the malloc dump provider.

BUG=518823, 499731
Committed: https://crrev.com/49b700716fb5d9a306237df1ed00e162d89b3a4c
Cr-Commit-Position: refs/heads/master@{#347964}

[Primiano Tucci (use gerrit), 2015-09-09 11:04:18]

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

[Primiano Tucci (use gerrit), 2015-09-09 11:04:18]

Guys I'd need a review on this. Lot of refactoring and cleanup going on.
Kudos to +ruuda who enlightened me on using GTest with lambdas and make them
more readable.

[petrcermak, 2015-09-09 11:48:30]

Looks good overall. Here's a couple of comments.

Thanks,
Petr

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.cc:126: if
(!skip_core_dumpers_auto_registration_for_testing_) {
nit: I thought we preferred early-out if-statements when possible (the original
solution). I think it was more readable before.

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager_unittest.cc (left):

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:100: DCHECK(delegate_);
Is there a particular reason for removing this?

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager_unittest.cc (right):

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:34: // GTest matchers for
MemoryDumpRequestArgs arguments.
I think that matcher the definitions are grouped in incorrect order here
(judging by the return statements + it's strange that you would only check high
details for one class and low details for another class).

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:40: return
arg.dump_args.level_of_detail == MemoryDumpArgs::LevelOfDetail::HIGH;
Any chance this could reuse the IsHighDetail matcher to reduce code duplication?
(also applies to line 49).

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:53: // requests locally to the
MemoryDUmpManager instead of performing IPC dances.
nit: s/MemoryDUmpManager/MemoryDumpManager/

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:75: class MockDumpProvider :
public MemoryDumpProvider {
nit: Wouldn't 'MockMemoryDumpProvider' be better? It will still fit on one line
anyway ;-)

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:112: void EnableTracing(const
char* category) {
nit: Wouldn't 'EnableTracingWithCategories' be clearer (to match the naming of
EnableTracingWithTraceConfig)? This way, it looks like EnableTracing is more
general than EnableTracingWithTraceConfig, which is not the case (in fact, it's
the other way round).

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:118: TraceConfig
tc(trace_config);
nit: I don't think there's any need to actually name this (and not use it
directly in the call below). The reason I'm saying this is that I think it would
be more readable if EnableTracing and EnableTracingWithTraceConfig had the same
"shape".

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:169: EXPECT_CALL(*delegate_,
RequestGlobalMemoryDump(_, _));
Add .Times(1)? It might be implicit, but it's used in tests quite a lot
(https://code.google.com/p/chromium/codesearch#search/&q=times%5C(1%5C))

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:183: EXPECT_CALL(*delegate_,
RequestGlobalMemoryDump(_, _));
Add .Times or .WillOnce? (applies to all "EXPECT_CALL(*delegate," assertions in
this file).

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:338: threads.push_back(new
Thread("test thread"));
Wouldn't it be nicer to define the thread and then push it (so that you wouldn't
have to access it through threads.back() all the time)?

auto thread = new Thread("test thread");
threads.push_back(thread);
...

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:393:
TEST_F(MemoryDumpManagerTest, DisableFailingDumpers) {
Just a thought here: What's the expected behavior when there are "gaps" between
failures (e.g. FAIL, SUCCEED, FAIL, SUCCEED, ...)? Do we test it?

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:457: // Like
RegisterDumperWhileDumping, but suddenly unregister the dump provider.
super-nit: Maybe add "instead" to the description: "..., but instead suddenly
unregister ..."?

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:499: threads.push_back(new
TestIOThread(TestIOThread::kAutoStart));
Same comment as above:

auto thread = new TestIOThread(TestIOThread::kAutoStart);
threads.push_back(thread);
...

might be more readable

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:602:
"{\"included_categories\":[\"%s\"]}", MemoryDumpManager::kTraceCategory);
It might be better to keep the JSON strings as constants (for readability).

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:638: // be performed in the
right sequence.
nit: "correct order" sounds more English to me than "right sequence"

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:645: testing::InSequence
sequence;
It might be a good idea to nest lines 645-654 in a block (to make it clear what
the InSequence variable refers to).

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:656: // Swallow all the final
the spurious calls until tracing gets disabled.
nit: s/the final the/final/ (I don't think you need any articles in this
sentence at all)

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:656: // Swallow all the final
the spurious calls until tracing gets disabled.
nit: Don't you mean "can be disabled"?

[Primiano Tucci (use gerrit), 2015-09-09 13:28:44]

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.cc:126: if
(!skip_core_dumpers_auto_registration_for_testing_) {
On 2015/09/09 11:48:29, petrcermak wrote:
> nit: I thought we preferred early-out if-statements when possible (the
original
> solution). I think it was more readable before.

True. But in the next CL i'm going to put code below (which needs to happen
after) and that should not be conditioned by this if.

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager_unittest.cc (left):

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:100: DCHECK(delegate_);
On 2015/09/09 11:48:29, petrcermak wrote:
> Is there a particular reason for removing this?

yes. before it was a responsiblity of the TEST_F to set a delegate, and they
could forget.
Now it is harder, as the delegate is set during Setup. This DCHECK becomes just
boilerplate here.

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager_unittest.cc (right):

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:34: // GTest matchers for
MemoryDumpRequestArgs arguments.
On 2015/09/09 11:48:30, petrcermak wrote:
> I think that matcher the definitions are grouped in incorrect order here
> (judging by the return statements + it's strange that you would only check
high
> details for one class and low details for another class).

right. fixed.

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:40: return
arg.dump_args.level_of_detail == MemoryDumpArgs::LevelOfDetail::HIGH;
On 2015/09/09 11:48:30, petrcermak wrote:
> Any chance this could reuse the IsHighDetail matcher to reduce code
duplication?
> (also applies to line 49).

Nope, couldn't find any way

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:53: // requests locally to the
MemoryDUmpManager instead of performing IPC dances.
On 2015/09/09 11:48:29, petrcermak wrote:
> nit: s/MemoryDUmpManager/MemoryDumpManager/

Done.

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:75: class MockDumpProvider :
public MemoryDumpProvider {
On 2015/09/09 11:48:29, petrcermak wrote:
> nit: Wouldn't 'MockMemoryDumpProvider' be better? It will still fit on one
line
> anyway ;-)

Ok. you're just lucky that didn't cause any wrapping :P

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:112: void EnableTracing(const
char* category) {
On 2015/09/09 11:48:29, petrcermak wrote:
> nit: Wouldn't 'EnableTracingWithCategories' be clearer (to match the naming of
> EnableTracingWithTraceConfig)? This way, it looks like EnableTracing is more
> general than EnableTracingWithTraceConfig, which is not the case (in fact,
it's
> the other way round).

Agree. EnableTracingWithLegacyCategories, it is.

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:118: TraceConfig
tc(trace_config);
On 2015/09/09 11:48:30, petrcermak wrote:
> nit: I don't think there's any need to actually name this (and not use it
> directly in the call below). The reason I'm saying this is that I think it
would
> be more readable if EnableTracing and EnableTracingWithTraceConfig had the
same
> "shape".

Agree. didn't realize. thanks. done

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:169: EXPECT_CALL(*delegate_,
RequestGlobalMemoryDump(_, _));
On 2015/09/09 11:48:30, petrcermak wrote:
> Add .Times(1)? It might be implicit, but it's used in tests quite a lot
> (https://code.google.com/p/chromium/codesearch#search/&q=times%5C(1%5C))

Done.

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:183: EXPECT_CALL(*delegate_,
RequestGlobalMemoryDump(_, _));
On 2015/09/09 11:48:29, petrcermak wrote:
> Add .Times or .WillOnce? (applies to all "EXPECT_CALL(*delegate," assertions
in
> this file).

Done everywhere

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:338: threads.push_back(new
Thread("test thread"));
On 2015/09/09 11:48:30, petrcermak wrote:
> Wouldn't it be nicer to define the thread and then push it (so that you
wouldn't
> have to access it through threads.back() all the time)?
> 
> auto thread = new Thread("test thread");
> threads.push_back(thread);
> ...

Done.

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:393:
TEST_F(MemoryDumpManagerTest, DisableFailingDumpers) {
On 2015/09/09 11:48:29, petrcermak wrote:
> Just a thought here: What's the expected behavior when there are "gaps"
between
> failures (e.g. FAIL, SUCCEED, FAIL, SUCCEED, ...)? Do we test it?

Done.

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:457: // Like
RegisterDumperWhileDumping, but suddenly unregister the dump provider.
On 2015/09/09 11:48:30, petrcermak wrote:
> super-nit: Maybe add "instead" to the description: "..., but instead suddenly
> unregister ..."?

Done.

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:499: threads.push_back(new
TestIOThread(TestIOThread::kAutoStart));
On 2015/09/09 11:48:29, petrcermak wrote:
> Same comment as above:
> 
> auto thread = new TestIOThread(TestIOThread::kAutoStart);
> threads.push_back(thread);
> ...
> 
> might be more readable

Nah this would add extra lines. IMHO this one is legible as it is.
THe other one already had extra lines, was just non-sense using back(). But not
here.

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:602:
"{\"included_categories\":[\"%s\"]}", MemoryDumpManager::kTraceCategory);
On 2015/09/09 11:48:30, petrcermak wrote:
> It might be better to keep the JSON strings as constants (for readability).

Discussed offline. It is the case right now.

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:638: // be performed in the
right sequence.
On 2015/09/09 11:48:30, petrcermak wrote:
> nit: "correct order" sounds more English to me than "right sequence"

Done.

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:645: testing::InSequence
sequence;
On 2015/09/09 11:48:29, petrcermak wrote:
> It might be a good idea to nest lines 645-654 in a block (to make it clear
what
> the InSequence variable refers to).

I'm going to move these lines in the next CL.

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:656: // Swallow all the final
the spurious calls until tracing gets disabled.
On 2015/09/09 11:48:29, petrcermak wrote:
> nit: s/the final the/final/ (I don't think you need any articles in this
> sentence at all)

Done.

https://codereview.chromium.org/1306843006/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:656: // Swallow all the final
the spurious calls until tracing gets disabled.
On 2015/09/09 11:48:30, petrcermak wrote:
> nit: Don't you mean "can be disabled"?

No. I mean that between the time we decide to disable tracing, and when it gets
actually disabled a spurious extra dump might happen.

[petrcermak, 2015-09-09 17:22:25]

LGTM.

Thanks and sorry for the delay,
Petr

[Primiano Tucci (use gerrit), 2015-09-09 17:23:09]

The CQ bit was checked by primiano@chromium.org

[commit-bot: I haz the power, 2015-09-09 17:23:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1306843006/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1306843006/20001

[commit-bot: I haz the power, 2015-09-09 18:33:38]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-09-09 18:34:29]

Patchset 2 (id:??) landed as
https://crrev.com/49b700716fb5d9a306237df1ed00e162d89b3a4c
Cr-Commit-Position: refs/heads/master@{#347964}

[commit-bot: I haz the power, 2015-09-23 12:01:24]

Patchset 2 (id:??) landed as
https://crrev.com/49b700716fb5d9a306237df1ed00e162d89b3a4c
Cr-Commit-Position: refs/heads/master@{#347964}