Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1313613002:
[tracing] MemoryDumpManager uses TraceConfig to set the periodic dump timers (Closed)
Description[tracing] MemoryDumpManager uses TraceConfig to set the periodic dump timers
The memory dump trigger config was added to TraceConfig in
crrev.com/1306753005. This CL updates the MemoryDumpManager to use
the TraceConfig to set the periodic timers for the memory dumps.
BUG=513692
Committed: https://crrev.com/08adbe40a5533b1aecf565af7b0ed964896de9ae
Cr-Commit-Position: refs/heads/master@{#347570}
Patch Set 1 #Patch Set 2 : Rebase. #Patch Set 3 : rebase. #Patch Set 4 : rebsae. #
Total comments: 13
Patch Set 5 : Fixes. #
Total comments: 26
Patch Set 6 : Fixes. #Patch Set 7 : Nits. #Patch Set 8 : Nits. #
Total comments: 8
Patch Set 9 : Fix the rate. #Patch Set 10 : Using sequence. #
Total comments: 8
Patch Set 11 : Nits. #Patch Set 12 : Nit. #Patch Set 13 : Set delegate once. #
Messages
Total messages: 24 (6 generated)
ssid@chromium.org changed reviewers: + primiano@chromium.org
PTAL, thanks.
https://codereview.chromium.org/1313613002/diff/50001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1313613002/diff/50001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:56: for (const TraceConfig::MemoryDumpTriggerConfig& config : All this seems way too complex. I don't like the idea of re-reading and copying the TraceConfig at each dump. I think you here want to do something way simpler, which is: Just add a g_heavy_dumps_rate and set it below when you parse the traceconfig. In this method just check if (g_heavy_dumps_rate > 0 && )g_periodic_dumps_count % g_heavy_dumps_rate) == 0) do heavy dump else do light dump https://codereview.chromium.org/1313613002/diff/50001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:403: TRACE_EVENT_CATEGORY_GROUP_ENABLED(kTraceCategory, &enabled); You don't need this check anymore right? https://codereview.chromium.org/1313613002/diff/50001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:444: if (!config_list.size()) if (config_list.empty()) ? https://codereview.chromium.org/1313613002/diff/50001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:454: // It does not make sense to have dumps at varying intervals. Is not that it doesnt' make sense. The deal is that we don't want to support it, which is different. Please adjust the comment... and the code according to the comment above. https://codereview.chromium.org/1313613002/diff/50001/base/trace_event/memory... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1313613002/diff/50001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:34: "\"disabled-by-default-memory-infra\"" I don't think you want the "disabled-by-default-memory-infra" here right? https://codereview.chromium.org/1313613002/diff/50001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:71: class CoordinatorMemoryDumpManagerDelegateForTesting Can you Just add a static public boolean to MemoryDumpManagerDelegateForTesting? it's simpler to read. So you don't need to change anything and just do MemoryDumpManagerDelegateForTesting::is_coordinator_process = true below https://codereview.chromium.org/1313613002/diff/50001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:103: void EnableTracing(const char* category) { I think at this point you don't need the char* argument anymore and can make this just EnableTracing() https://codereview.chromium.org/1313613002/diff/50001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:110: void EnableTracingWithTriggers() { Rename this to EnableTracingWithTraceConfig(const char* trace_config) And I'd remove the delegate_.reset(new CoordinatorMemoryDumpManagerDelegateForTesting()). Just in the test fixture below do: MemoryDumpManagerDelegateForTesting::is_coordinator_process = true; https://codereview.chromium.org/1313613002/diff/50001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:207: if (on_memory_dump_calls_ % 2 == 0 || on_memory_dump_calls_ % 3 == 0) I don't think you need all this logic, just use the gtest expectations below. https://codereview.chromium.org/1313613002/diff/50001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:627: TEST_F(MemoryDumpManagerTest, TraceConfigTriggers) { The name here (TraceConfigTriggers) should reflect what this fixture is testing. Also please add a comment explaining what this test does. I'd probably name it SchedulePeriodicDumpsFromTraceConfig https://codereview.chromium.org/1313613002/diff/50001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:638: // 50 ms is the shortest interval given in the kMemoryDumpTraceConfigString. Please don't have tests based on timings as they are extremely flaky. I'd just make this test as follows: Make the dump provider post the QuitClosure on the Nth dump, and just check that you receive the right amount of light and heavy dumps https://codereview.chromium.org/1313613002/diff/50001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:645: } Also you should have a test to make sure that if you use the old-style traceconfig ctor you still get the periodic dumps.
Made changes, PTAL. https://codereview.chromium.org/1313613002/diff/50001/base/trace_event/memory... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1313613002/diff/50001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:645: } On 2015/08/26 10:25:07, Primiano Tucci wrote: > Also you should have a test to make sure that if you use the old-style > traceconfig ctor you still get the periodic dumps. This test is not needed here since, all we test here is if we get the correct TraceConfig and follow it. This test was added in TraceConfig change.
I am super sorry for the delay. Looks great, just few comments. Can we just have a final check (and update the TraceConfig doc so we don't have to reason again about this) before landing this? https://codereview.chromium.org/1313613002/diff/70001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1313613002/diff/70001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:55: if (++g_periodic_dumps_count == g_heavy_dumps_rate) since g_heavy_dumps_rate can be 0 this should become if (g_heavy_dumps_rate > 0 && ++g_periodic_dumps_count == g_heavy_dumps_rate), otherwise you will end up doing one heavy dump every 4BLN (when the uint32 wraps over) if heavy dumps are disabled. Unlikely but also undesiderable :) https://codereview.chromium.org/1313613002/diff/70001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:431: g_periodic_dumps_count = 0; Please add a comment here saying: // Enable periodic dumps. At the moment the periodic support is limited to at most one low-detail periodic dump and at most one high-detail periodic dump. If both are specified the high-detail period must be an integer multiple of the low-level one. https://codereview.chromium.org/1313613002/diff/70001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:439: uint32 timer_period_ms = std::numeric_limits<uint32>::max(); nit: rename to min_timer_period_ms; https://codereview.chromium.org/1313613002/diff/70001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:440: uint32 heavy_dump_period_ms = 0; Also add a DCHECK_LE(config_list.size(), 2) https://codereview.chromium.org/1313613002/diff/70001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:442: if (config.level_of_detail == MemoryDumpArgs::LevelOfDetail::HIGH) Also add a: if (config.periodic_interval_ms == 0) { DCHECK(false); continue; } https://codereview.chromium.org/1313613002/diff/70001/base/trace_event/memory... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1313613002/diff/70001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:34: "\"disabled-by-default-memory-infra\"" I'd probably move this as an inline string in the TEST_F SchedulePeriodicDumpsFromTraceConfig, so the test becomes more readable (and make EnableTracingWithTraceConfig take a const char* arg) https://codereview.chromium.org/1313613002/diff/70001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:40: "\"periodic_interval_ms\":1" Hmm I'm scared that this is too fast. There is the risk that some dumps will be rejected (and the test fail) if the memory dump manager has not finished the previous dump. I'd probably make these 100 and 300 ms https://codereview.chromium.org/1313613002/diff/70001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:98: void EnableTracing(const char* category) { Please add a comment here saying that this enalbes tracing using the legacy category filter string. https://codereview.chromium.org/1313613002/diff/70001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:213: MessageLoop::current()->task_runner()->PostTask(FROM_HERE, quit_closure_); s/MessageLoop::current()->task_runner()/TaskRunnerHandle::get()->PostTask/ https://codereview.chromium.org/1313613002/diff/70001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:614: g_high_detail_args, callback); Thanks for fixing these :) https://codereview.chromium.org/1313613002/diff/70001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:622: MockDumpProviderForPeriodicDump mdp(10, run_loop.QuitClosure()); either make this 10 a constant (i.e. const int kDumpCallsToQuit = 10) or just add aninline comment (i.e. 10 /* dump_calls_to_quit */) https://codereview.chromium.org/1313613002/diff/70001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:626: DCHECK(g_high_detail_args == g_high_detail_args); uh? https://codereview.chromium.org/1313613002/diff/70001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:628: .Times(4) just make these 2+3 = 5 (instead of 6+4 = 10) so the test will be faster and have the same coverage.
PTAL, Thanks https://codereview.chromium.org/1313613002/diff/70001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1313613002/diff/70001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:55: if (++g_periodic_dumps_count == g_heavy_dumps_rate) On 2015/09/03 08:39:51, Primiano Tucci wrote: > since g_heavy_dumps_rate can be 0 this should become > > if (g_heavy_dumps_rate > 0 && ++g_periodic_dumps_count == g_heavy_dumps_rate), > otherwise you will end up doing one heavy dump every 4BLN (when the uint32 wraps > over) if heavy dumps are disabled. Unlikely but also undesiderable :) Done. https://codereview.chromium.org/1313613002/diff/70001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:431: g_periodic_dumps_count = 0; On 2015/09/03 08:39:50, Primiano Tucci wrote: > Please add a comment here saying: > > // Enable periodic dumps. At the moment the periodic support is limited to at > most one low-detail periodic dump and at most one high-detail periodic dump. If > both are specified the high-detail period must be an integer multiple of the > low-level one. Done. https://codereview.chromium.org/1313613002/diff/70001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:439: uint32 timer_period_ms = std::numeric_limits<uint32>::max(); On 2015/09/03 08:39:50, Primiano Tucci wrote: > nit: rename to min_timer_period_ms; Done. https://codereview.chromium.org/1313613002/diff/70001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:440: uint32 heavy_dump_period_ms = 0; On 2015/09/03 08:39:50, Primiano Tucci wrote: > Also add a DCHECK_LE(config_list.size(), 2) Done. https://codereview.chromium.org/1313613002/diff/70001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:442: if (config.level_of_detail == MemoryDumpArgs::LevelOfDetail::HIGH) On 2015/09/03 08:39:50, Primiano Tucci wrote: > Also add a: > > if (config.periodic_interval_ms == 0) { > DCHECK(false); > continue; > } Done. https://codereview.chromium.org/1313613002/diff/70001/base/trace_event/memory... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1313613002/diff/70001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:34: "\"disabled-by-default-memory-infra\"" On 2015/09/03 08:39:51, Primiano Tucci wrote: > I'd probably move this as an inline string in the TEST_F > SchedulePeriodicDumpsFromTraceConfig, so the test becomes more readable > (and make EnableTracingWithTraceConfig take a const char* arg) Done. https://codereview.chromium.org/1313613002/diff/70001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:40: "\"periodic_interval_ms\":1" On 2015/09/03 08:39:51, Primiano Tucci wrote: > Hmm I'm scared that this is too fast. There is the risk that some dumps will be > rejected (and the test fail) if the memory dump manager has not finished the > previous dump. > I'd probably make these 100 and 300 ms I asked sami about this and he suggested this is the usual time that is set for periodic timer tests. Also, if the test takes longer people might revert the test since the unittests should be very short. He suggested 1ms should be good enough. https://codereview.chromium.org/1313613002/diff/70001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:98: void EnableTracing(const char* category) { On 2015/09/03 08:39:51, Primiano Tucci wrote: > Please add a comment here saying that this enalbes tracing using the legacy > category filter string. Done. https://codereview.chromium.org/1313613002/diff/70001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:213: MessageLoop::current()->task_runner()->PostTask(FROM_HERE, quit_closure_); On 2015/09/03 08:39:51, Primiano Tucci wrote: > s/MessageLoop::current()->task_runner()/TaskRunnerHandle::get()->PostTask/ Done. https://codereview.chromium.org/1313613002/diff/70001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:622: MockDumpProviderForPeriodicDump mdp(10, run_loop.QuitClosure()); On 2015/09/03 08:39:51, Primiano Tucci wrote: > either make this 10 a constant (i.e. const int kDumpCallsToQuit = 10) > or just add aninline comment (i.e. 10 /* dump_calls_to_quit */) Done. https://codereview.chromium.org/1313613002/diff/70001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:626: DCHECK(g_high_detail_args == g_high_detail_args); On 2015/09/03 08:39:51, Primiano Tucci wrote: > uh? sorry, removed! https://codereview.chromium.org/1313613002/diff/70001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:628: .Times(4) On 2015/09/03 08:39:51, Primiano Tucci wrote: > just make these 2+3 = 5 (instead of 6+4 = 10) so the test will be faster and > have the same coverage. Done.
skyostil@chromium.org changed reviewers: + skyostil@chromium.org
https://codereview.chromium.org/1313613002/diff/70001/base/trace_event/memory... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1313613002/diff/70001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:40: "\"periodic_interval_ms\":1" On 2015/09/03 16:30:18, ssid wrote: > On 2015/09/03 08:39:51, Primiano Tucci wrote: > > Hmm I'm scared that this is too fast. There is the risk that some dumps will > be > > rejected (and the test fail) if the memory dump manager has not finished the > > previous dump. > > I'd probably make these 100 and 300 ms > > I asked sami about this and he suggested this is the usual time that is set for > periodic timer tests. Also, if the test takes longer people might revert the > test since the unittests should be very short. He suggested 1ms should be good > enough. Right, the idea was that we should try to keep unit test execution time on the order of milliseconds instead of hundreds of ms. For instance in this test it seems like we don't really care about the actual data in the dump so maybe there's a way to mock that out.
> Right, the idea was that we should try to keep unit test execution time on the order of milliseconds instead of hundreds of ms. Right, agree, bad my original suggestion. Ssid and I had a chat later, I realized only later by looking at the code that by mocking at the delegate level (as ssid is doing in the latest patchset) there is no timing issue anymore (i.e. they can be 1 ms without risking any flakiness). Thanks ;) https://codereview.chromium.org/1313613002/diff/130001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1313613002/diff/130001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:52: g_periodic_dumps_count == 0 ? MemoryDumpArgs::LevelOfDetail::HIGH sorry realized only now. Isn't this going to do always HIGH dumps if g_heavy_dumps_rate == 0? I think you need to fix the math here :) I think that the g_heavy_dumps_rate > 0 goes above (on line 52) not below ;) https://codereview.chromium.org/1313613002/diff/130001/base/trace_event/memor... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1313613002/diff/130001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:69: CreateProcessDump(args, callback); No need to create any process dump https://codereview.chromium.org/1313613002/diff/130001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:70: if (dump_call_count_ % expected_high_dump_ratio_ == 0) { I don't think you need all this logic here. You can handle the sequencing with the EXPECT_CALL + predicate matching in the TEST_F. In other words I think you need a super simple mock delegate (with just the MOCK method) and then, in the TEST_F EXPECT_CALL(*delegate.get(), RequestGlobalMemoryDump(g_high_detail_args, _)).times(1) EXPECT_CALL(*delegate.get(), RequestGlobalMemoryDump(g_low_detail_args, _)).times(2) EXPECT_CALL(*delegate.get(), RequestGlobalMemoryDump(g_high_detail_args, _)).times(1) EXPECT_CALL(*delegate.get(), RequestGlobalMemoryDump(g_low_detail_args, _)).times(2) EXPECT_CALL(*delegate.get(), RequestGlobalMemoryDump(g_high_detail_args, _)).WillOnce(Invoke(... lambda for posting the closure)
https://codereview.chromium.org/1313613002/diff/130001/base/trace_event/memor... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1313613002/diff/130001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:48: : public MemoryDumpManagerDelegateForTesting { Ah also why doesn't this inherit just from MemoryDumpManagerDelegate ?
PTAL. https://codereview.chromium.org/1313613002/diff/130001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1313613002/diff/130001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:52: g_periodic_dumps_count == 0 ? MemoryDumpArgs::LevelOfDetail::HIGH On 2015/09/03 22:44:13, Primiano Tucci wrote: > sorry realized only now. Isn't this going to do always HIGH dumps if > g_heavy_dumps_rate == 0? I think you need to fix the math here :) > I think that the g_heavy_dumps_rate > 0 goes above (on line 52) not below ;) Done. https://codereview.chromium.org/1313613002/diff/130001/base/trace_event/memor... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1313613002/diff/130001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:48: : public MemoryDumpManagerDelegateForTesting { On 2015/09/03 22:44:51, Primiano Tucci wrote: > Ah also why doesn't this inherit just from MemoryDumpManagerDelegate ? Because the delegate objects are owned by MemoryDumpManagerTest. It cannot have a scoped ptr of MemoryDumpManagerDelegate, since it is virtual. So, it holds objects of MemoryDumpManagerDelegateForTesting instead. Also, there is one extra ,method that needs to be copied. https://codereview.chromium.org/1313613002/diff/130001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:69: CreateProcessDump(args, callback); On 2015/09/03 22:44:13, Primiano Tucci wrote: > No need to create any process dump Done. https://codereview.chromium.org/1313613002/diff/130001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:70: if (dump_call_count_ % expected_high_dump_ratio_ == 0) { On 2015/09/03 22:44:13, Primiano Tucci wrote: > I don't think you need all this logic here. You can handle the sequencing with > the EXPECT_CALL + predicate matching in the TEST_F. > In other words I think you need a super simple mock delegate (with just the MOCK > method) and then, in the TEST_F > > EXPECT_CALL(*delegate.get(), RequestGlobalMemoryDump(g_high_detail_args, > _)).times(1) > EXPECT_CALL(*delegate.get(), RequestGlobalMemoryDump(g_low_detail_args, > _)).times(2) > EXPECT_CALL(*delegate.get(), RequestGlobalMemoryDump(g_high_detail_args, > _)).times(1) > EXPECT_CALL(*delegate.get(), RequestGlobalMemoryDump(g_low_detail_args, > _)).times(2) > EXPECT_CALL(*delegate.get(), RequestGlobalMemoryDump(g_high_detail_args, > _)).WillOnce(Invoke(... lambda for posting the closure) > No, I cannot do that. I can define only one expect_call for a function. That i taken as the total number of calls the function gets in the exec of the test. If I define multiple times, the test will surely fail at multiple places saying the function is called 5 times but it is said as 2 / 1 here. So I have to define something like called with high args 2 times and called with low args 3 times. But for this I need to have an "==" defined in MemoryDumpRequestArgs. This struct holds MemoryDumpArgs, which means I need an == defined in here as well. Instead of defining multiple "==" in production code, I made the test logic complicated. Also, this tests for the correct order of dump calls, not just the number of high and low calls.
> No, I cannot do that. I can define only one expect_call for a function. That i
> taken as the total number of calls the function gets in the exec of the test.
If
> I define multiple times, the test will surely fail at multiple places saying
the
> function is called 5 times but it is said as 2 / 1 here.
> So I have to define something like called with high args 2 times and called
with
> low args 3 times.
> But for this I need to have an "==" defined in MemoryDumpRequestArgs. This
> struct holds MemoryDumpArgs, which means I need an == defined in here as well.
> Instead of defining multiple "==" in production code, I made the test logic
> complicated. Also, this tests for the correct order of dump calls, not just
the
> number of high and low calls.
I had some fun, here's how you can do that without overriding == and simplifying
the delegate to two lines:
MATCHER(IsHigh, "") { return arg.dump_args.level_of_detail ==
MemoryDumpArgs::LevelOfDetail::HIGH; }
..ditto for IsLowDetailed
TEST_F(MemoryDumpManagerTest, SchedulePeriodicDumpsFromTraceConfig) {
const char kMemoryDumpTraceConfigString[] = .....
RunLoop run_loop;
scoped_ptr<MemoryDumpManagerDelegateForPeriodicDumpTest> delegate(
new MemoryDumpManagerDelegateForPeriodicDumpTest(
5 /* dump_calls_to_quit */, 3 /* expected_high_dump_ratio */,
run_loop.QuitClosure()));
::testing::InSequence blah;
EXPECT_CALL(*delegate.get(), RequestGlobalMemoryDump(IsHigh(), _)).Times(1);
EXPECT_CALL(*delegate.get(), RequestGlobalMemoryDump(_, _)).Times(1);
EXPECT_CALL(*delegate.get(), RequestGlobalMemoryDump(_, _)).Times(1);
EXPECT_CALL(*delegate.get(), RequestGlobalMemoryDump(_, _)).Times(1);
EXPECT_CALL(*delegate.get(), RequestGlobalMemoryDump(_, _)).WillOnce(Invoke(
[&](const MemoryDumpRequestArgs& args,const MemoryDumpCallback& callback) {
TraceLog::GetInstance()->SetDisabled();
ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
run_loop.QuitClosure());
}));
SetDelegate(delegate.Pass());
EnableTracingWithTraceConfig(kMemoryDumpTraceConfigString);
run_loop.Run();
}
Thanks made changes.
LGTM with some final minor comments! Many thanks for the patience. https://codereview.chromium.org/1313613002/diff/170001/base/trace_event/memor... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1313613002/diff/170001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:50: ~MemoryDumpManagerDelegateForPeriodicDumpTest() override {} I think you need to move the dtor in the one up ? (or probably don't need this at all?) https://codereview.chromium.org/1313613002/diff/170001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:94: void SetDelegate(scoped_ptr<MemoryDumpManagerDelegateForTesting> delegate) { plz swap the order or this one and the one below. Right now you have EnableTracing SetDelegate EnableTracingWithTraceConfig I think it's a bit tidier if you do EnableTracing EnableTracingWithTraceConfig SetDelegate https://codereview.chromium.org/1313613002/diff/170001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:586: MATCHER(IsHigh, "") { Nit: IsHighDetail (same for Low) https://codereview.chromium.org/1313613002/diff/170001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:624: .WillOnce(Invoke([&](const MemoryDumpRequestArgs& args, oh sorry forgot that we don't allow implicit captures in lambda (I did it in my experiment because I was too lazy). I think the only capture that you need here is just the quit closure. I.e. store the quit closure above with a auto quit_closure = run_loop.QuitClosure; and then pass it as [&quit_closure] to the lambda instead of [&] and pass it below to the PostTask,
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1313613002/210001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1313613002/210001
Done thanks. https://codereview.chromium.org/1313613002/diff/170001/base/trace_event/memor... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1313613002/diff/170001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:50: ~MemoryDumpManagerDelegateForPeriodicDumpTest() override {} On 2015/09/04 14:25:29, Primiano Tucci wrote: > I think you need to move the dtor in the one up ? (or probably don't need this > at all?) Done. https://codereview.chromium.org/1313613002/diff/170001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:94: void SetDelegate(scoped_ptr<MemoryDumpManagerDelegateForTesting> delegate) { On 2015/09/04 14:25:29, Primiano Tucci wrote: > plz swap the order or this one and the one below. Right now you have > > EnableTracing > SetDelegate > EnableTracingWithTraceConfig > > I think it's a bit tidier if you do > EnableTracing > EnableTracingWithTraceConfig > SetDelegate moved it before enable since it should be called before enabling. https://codereview.chromium.org/1313613002/diff/170001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:586: MATCHER(IsHigh, "") { On 2015/09/04 14:25:29, Primiano Tucci wrote: > Nit: IsHighDetail (same for Low) Done. https://codereview.chromium.org/1313613002/diff/170001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:624: .WillOnce(Invoke([&](const MemoryDumpRequestArgs& args, On 2015/09/04 14:25:29, Primiano Tucci wrote: > oh sorry forgot that we don't allow implicit captures in lambda (I did it in my > experiment because I was too lazy). I think the only capture that you need here > is just the quit closure. > I.e. store the quit closure above with a > auto quit_closure = run_loop.QuitClosure; > and then pass it as [&quit_closure] to the lambda instead of [&] and pass it > below to the PostTask, Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ssid@chromium.org
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/1313613002/#ps230001 (title: "Set delegate once.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1313613002/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1313613002/230001
Message was sent while issue was closed.
Committed patchset #13 (id:230001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/08adbe40a5533b1aecf565af7b0ed964896de9ae Cr-Commit-Position: refs/heads/master@{#347570} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
