Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1995573003:
[tracing] Introduce BACKGROUND mode in MemoryInfra (Closed)
|
Created:
4 years, 7 months ago by ssid Modified:
4 years, 6 months ago CC:
chromium-reviews, wfh+watch_chromium.org, blink-reviews, dglazkov+blink, tracing+reviews_chromium.org, kinuko+watch, blink-reviews-api_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[tracing] Introduce BACKGROUND mode in MemoryInfra
This CL makes following changes:
1. Adds a BACKGROUND mode in the dump triggers in trace config.
1. Periodic dumps can be disabled on passing 0 as interval.
2. On BACKGROUND mode only whitelisted dump providers are invoked.
3. Clean up periodic dump timer set up.
4. MemoryDumpSessionState is initialized with config in the renderer
also.
BUG=613198
Committed: https://crrev.com/13ebc734eab7950938afbfac677080604d972d91
Cr-Commit-Position: refs/heads/master@{#396927}
Patch Set 1 : with periodic again. #
Total comments: 24
Patch Set 2 : Address comments and fix tests. #
Total comments: 5
Patch Set 3 : Use nullptr to mark end. #
Messages
Total messages: 49 (30 generated)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Description was changed from ========== [tracing] Introduce BACKGROUND mode in MemoryInfra Adds a BACKGROUND mode in trace config. On BACKGROUND mode we whitelist dump providers. BUG= ========== to ========== [tracing] Introduce BACKGROUND mode in MemoryInfra Adds a BACKGROUND mode in the trace config. On BACKGROUND mode periodic dumps are disabled and only whitelisted dump providers are invoked. BUG=613198 ==========
Patchset #4 (id:100001) has been deleted
Description was changed from ========== [tracing] Introduce BACKGROUND mode in MemoryInfra Adds a BACKGROUND mode in the trace config. On BACKGROUND mode periodic dumps are disabled and only whitelisted dump providers are invoked. BUG=613198 ========== to ========== [tracing] Introduce BACKGROUND mode in MemoryInfra This CL makes following changes: 1. Adds a BACKGROUND mode in the dump triggers in trace config. 1. Periodic dumps can be disabled on passing 0 as interval. 2. On BACKGROUND mode only whitelisted dump providers are invoked. 3. Clean up periodic dump timer set up. 4. MemoryDumpSessionState is initialized with config in the renderer also. BUG=613198 ==========
Patchset #4 (id:120001) has been deleted
ssid@chromium.org changed reviewers: + primiano@chromium.org
ptal, thanks. This only whitelists dump provider on background mode. Next cl will add code to sanitize the dump names. https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (left): https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:635: AutoLock lock(lock_); This unnecessarily locks the whole function. I cannot call RequestGlobalDump when already inside lock. So, restructued it to use lock only when necessary. This assumes that the following does not need lock: periodic_dump_helper_ - already accessed in OnTraceLogDisabled without lock. So, not really useful to have a lock here. If there is a race between enabling and disabling, then it could also happen that OnTraceLogDisabled can be called even before OntraceLogEnabled and the timer runs for ever. So, this can't be avoided anyway. https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:685: session_state_->SetMemoryDumpConfig(trace_config.memory_dump_config()); I think this needs to be set before the check for is_coordinator. The renderer process won't see the config otherwise.
Did a first pass. Overall the approach looks great. The only major doubt is the "0 disables everything", see comment below. Will do a 2nd pass tomorrow. Thanks a lot for this. https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (left): https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:635: AutoLock lock(lock_); On 2016/05/23 18:17:03, ssid wrote: > This unnecessarily locks the whole function. I cannot call RequestGlobalDump > when already inside lock. So, restructued it to use lock only when necessary. > Where is the call to RequestGlobalDump? In the timer? > periodic_dump_helper_ - already accessed in OnTraceLogDisabled without lock. So, > not really useful to have a lock here. If there is a race between enabling and > disabling, then it could also happen that OnTraceLogDisabled can be called even > before OntraceLogEnabled and the timer runs for ever. So, this can't be avoided > anyway. Nope, you are right. If OnTraceLogEnabled and Disabled can be called at the same time this is the end and we should just go home :) https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:56: "WhitelistedTestDumpProvider", // For testing. I think a better approach is to: - Make this an empty array (for the moment, with a TODO that you will fill it in next CLs) - Introduce a static private field in MDM (a pointer to array) called background_dump_providers_whitelist. - Have the MDM ctor set dump_provider_whitelist = kDumpProviderWhitelist - Have an instance method MDM::GetInstance()->SetBackgroundDumpProviderWhitelistForTesting(pointer to array) - Have the test set this to its own array. Does it make sense? https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:114: class MemoryDumpManager::PeriodicGlobalDumpInvokeHelper { nit: I think this should go after the constants below (MaxConsecutiveFailuresCount and friends) https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:131: PeriodicGlobalDumpInvokeHelper( In general the constructor should not do actions, mostly for sake of readability. If I see new DumpInvokeHelper() I don't expect that to do anything. Can you just make the ctor of this trivial and turn this into a Start() method? https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:153: if (!min_timer_period_ms) Hmm this is a bit contra-intuitive. THis means that if you say: heavy: 5, light: 0, both heavy and light will be disabled :/ What is the rationale of this? https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:741: session_state->SetMemoryDumpConfig(trace_config.memory_dump_config()); nit: seems a bit more logical to have this after sessions_state = ... above on line 718. Or is there a reason to have this after the if(heap_profiling) https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:769: if (!triggers_list.empty()) { maybe just always pass the triggers and to this check in the helper? Also I think you can just pass trace_config.memory_dump_config().triggers as argument, no need for the intermedia dir, especially if you introduce the start method. https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:148: class PeriodicGlobalDumpInvokeHelper; I think just PeriodicDumpHelper is enough (which also matches the name of the variable below) https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:203: // True if the dump provider is not whitelited for background mode. nit: whitelisted https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:204: const bool disabled_for_background_mode; in order to make this less confusing (initially I thought "why we have disabled and disabled_for_background"?) I think this should really be named: whitelisted_for_background_mode; https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... File base/trace_event/memory_dump_request_args.h (right): https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_request_args.h:31: // MemoryDumpProvider instances must guarantee that level of detail does not This comment is not true anymore. Better, this now only applies to LIGHT vs DETAILED, but BACKGROUND does not have to guarantee that the totals match. So move this below after background and reword it.
Thanks, made suggested changes, except the whitelist being replaced for testing. Added a workaround for the tests. Ptal, thanks https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (left): https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:635: AutoLock lock(lock_); On 2016/05/26 17:20:53, Primiano Tucci wrote: > On 2016/05/23 18:17:03, ssid wrote: > > This unnecessarily locks the whole function. I cannot call RequestGlobalDump > > when already inside lock. So, restructued it to use lock only when necessary. > > > > Where is the call to RequestGlobalDump? In the timer? We currently do the first dump after 250ms of starting recording. I was trying to see if triggering the first dump as soon as trace log is enabled works. But it doesn't because the processes do not yet get the tracing enabled message. So, in that case it hit a deadlock since it was trying to lock twice. I made this just to be safe in case we need to trigger dumps here in future. https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:56: "WhitelistedTestDumpProvider", // For testing. On 2016/05/26 17:20:53, Primiano Tucci wrote: > I think a better approach is to: > - Make this an empty array (for the moment, with a TODO that you will fill it in > next CLs) > - Introduce a static private field in MDM (a pointer to array) called > background_dump_providers_whitelist. > - Have the MDM ctor set dump_provider_whitelist = kDumpProviderWhitelist > - Have an instance method > MDM::GetInstance()->SetBackgroundDumpProviderWhitelistForTesting(pointer to > array) > - Have the test set this to its own array. > > Does it make sense? Yeah the problem was that i cannot get the array size when i loop through it and i had to store either size or a vector. If the issue is just the test name, i have added an extra field in dump manager for the testing case. The other option would be to use BlinkGC as dump provider for unittests https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:114: class MemoryDumpManager::PeriodicGlobalDumpInvokeHelper { On 2016/05/26 17:20:53, Primiano Tucci wrote: > nit: I think this should go after the constants below > (MaxConsecutiveFailuresCount and friends) Added to the last. https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:131: PeriodicGlobalDumpInvokeHelper( On 2016/05/26 17:20:53, Primiano Tucci wrote: > In general the constructor should not do actions, mostly for sake of > readability. > If I see new DumpInvokeHelper() I don't expect that to do anything. > Can you just make the ctor of this trivial and turn this into a Start() method? Makes sense. https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:153: if (!min_timer_period_ms) On 2016/05/26 17:20:53, Primiano Tucci wrote: > Hmm this is a bit contra-intuitive. THis means that if you say: > heavy: 5, light: 0, both heavy and light will be disabled :/ > What is the rationale of this? Yeah, I was thinking it is useful to specify mode without period. But realized it is useless. https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:741: session_state->SetMemoryDumpConfig(trace_config.memory_dump_config()); On 2016/05/26 17:20:53, Primiano Tucci wrote: > nit: seems a bit more logical to have this after sessions_state = ... above on > line 718. > Or is there a reason to have this after the if(heap_profiling) Done. https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:769: if (!triggers_list.empty()) { On 2016/05/26 17:20:53, Primiano Tucci wrote: > maybe just always pass the triggers and to this check in the helper? > Also I think you can just pass trace_config.memory_dump_config().triggers as > argument, no need for the intermedia dir, especially if you introduce the start > method. Done. https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:148: class PeriodicGlobalDumpInvokeHelper; On 2016/05/26 17:20:53, Primiano Tucci wrote: > I think just PeriodicDumpHelper is enough (which also matches the name of the > variable below) I think after including start and stop it is more like PeriodicGlobalDumpTimer. Global because it says it is a global timer not per process. Also declared it here since test needs isRunning function. https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:203: // True if the dump provider is not whitelited for background mode. On 2016/05/26 17:20:53, Primiano Tucci wrote: > nit: whitelisted Done. https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:204: const bool disabled_for_background_mode; On 2016/05/26 17:20:53, Primiano Tucci wrote: > in order to make this less confusing (initially I thought "why we have disabled > and disabled_for_background"?) I think this should really be named: > whitelisted_for_background_mode; Done thanks https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... File base/trace_event/memory_dump_request_args.h (right): https://codereview.chromium.org/1995573003/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_request_args.h:31: // MemoryDumpProvider instances must guarantee that level of detail does not On 2016/05/26 17:20:54, Primiano Tucci wrote: > This comment is not true anymore. Better, this now only applies to LIGHT vs > DETAILED, but BACKGROUND does not have to guarantee that the totals match. > So move this below after background and reword it. Done.
Patchset #5 (id:160001) has been deleted
Patchset #5 (id:160001) has been deleted
LGTM % final comments. I prefer that dump_provider_whitelisted_for_testing_ was still an array (look at the comment, use a null terminated list) so you can actually test that the whitelisting works in presence of more entries. https://codereview.chromium.org/1995573003/diff/180001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1995573003/diff/180001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:105: const char* whitelisted_for_testing) { gh this is rather hacky. See my comment above about having the list to be null terminated. https://codereview.chromium.org/1995573003/diff/180001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:109: static const size_t kListSize = arraysize(kDumpProviderWhitelist); Instead of arraysize you can just expect the list to be null terminated (with a nullptr entry at the end). This would solve the size problem. https://codereview.chromium.org/1995573003/diff/180001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:281: bool disabled_for_background_mode = you forgot to update the name of this one, which now is the other way round from the name. https://codereview.chromium.org/1995573003/diff/180001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:702: // most one periodic trigger per dump mode. All intervals should be an integer I think this comment should be moved to the dump timer class https://codereview.chromium.org/1995573003/diff/180001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:789: : periodic_dumps_count_(0) {} why only this one is initialized here. Either you init all the pods here, or none of them and do that in the Start() This feels a bit inconsistent. https://codereview.chromium.org/1995573003/diff/180001/base/trace_event/memor... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/1995573003/diff/180001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:289: void RequestPeriodicGlobalDump(); This should be private: right? https://codereview.chromium.org/1995573003/diff/180001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:292: std::unique_ptr<RepeatingTimer> timer_; What is the advantage of having a unique_ptr here? Is it just to avoid the include? If so just make it a member of the class and avoid the timer. The include was already there :) https://codereview.chromium.org/1995573003/diff/180001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:296: }; +DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1995573003/diff/180001/base/trace_event/memor... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1995573003/diff/180001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:60: void RegisterDumpProvider( WHy did you re-split this function? I thought we had that in the past and somebody simplified it. It doesn't seem you have a large number of cases for this.
Patchset #5 (id:180001) has been deleted
I am so sorry, I deleted the patch you reviewed by mistake. I can't find any way to get it back. :( On 2016/05/27 14:55:53, Primiano Tucci wrote: > LGTM % final comments. > I prefer that dump_provider_whitelisted_for_testing_ was still an array (look at > the comment, use a null terminated list) so you can actually test that the > whitelisting works in presence of more entries. Done. > https://codereview.chromium.org/1995573003/diff/180001/base/trace_event/memor... > File base/trace_event/memory_dump_manager.cc (right): > > https://codereview.chromium.org/1995573003/diff/180001/base/trace_event/memor... > base/trace_event/memory_dump_manager.cc:105: const char* > whitelisted_for_testing) { > gh this is rather hacky. See my comment above about having the list to be null > terminated. Done. > https://codereview.chromium.org/1995573003/diff/180001/base/trace_event/memor... > base/trace_event/memory_dump_manager.cc:109: static const size_t kListSize = > arraysize(kDumpProviderWhitelist); > Instead of arraysize you can just expect the list to be null terminated (with a > nullptr entry at the end). This would solve the size problem. Done. > https://codereview.chromium.org/1995573003/diff/180001/base/trace_event/memor... > base/trace_event/memory_dump_manager.cc:281: bool disabled_for_background_mode = > you forgot to update the name of this one, which now is the other way round from > the name. Done. > https://codereview.chromium.org/1995573003/diff/180001/base/trace_event/memor... > base/trace_event/memory_dump_manager.cc:702: // most one periodic trigger per > dump mode. All intervals should be an integer > I think this comment should be moved to the dump timer class Done. > https://codereview.chromium.org/1995573003/diff/180001/base/trace_event/memor... > base/trace_event/memory_dump_manager.cc:789: : periodic_dumps_count_(0) {} > why only this one is initialized here. Either you init all the pods here, or > none of them and do that in the Start() > This feels a bit inconsistent. Removed all initialization. > https://codereview.chromium.org/1995573003/diff/180001/base/trace_event/memor... > File base/trace_event/memory_dump_manager.h (right): > > https://codereview.chromium.org/1995573003/diff/180001/base/trace_event/memor... > base/trace_event/memory_dump_manager.h:289: void RequestPeriodicGlobalDump(); > This should be private: Fixed. > https://codereview.chromium.org/1995573003/diff/180001/base/trace_event/memor... > base/trace_event/memory_dump_manager.h:292: std::unique_ptr<RepeatingTimer> > timer_; > What is the advantage of having a unique_ptr here? Is it just to avoid the > include? > If so just make it a member of the class and avoid the timer. The include was > already there :) I just didn't want to allocate the class espcially for renderers. But it seems small. > https://codereview.chromium.org/1995573003/diff/180001/base/trace_event/memor... > base/trace_event/memory_dump_manager.h:296: }; > +DISALLOW_COPY_AND_ASSIGN Done. > https://codereview.chromium.org/1995573003/diff/180001/base/trace_event/memor... > File base/trace_event/memory_dump_manager_unittest.cc (right): > > https://codereview.chromium.org/1995573003/diff/180001/base/trace_event/memor... > base/trace_event/memory_dump_manager_unittest.cc:60: void RegisterDumpProvider( > WHy did you re-split this function? > I thought we had that in the past and somebody simplified it. > It doesn't seem you have a large number of cases for this. This one has to set a different name for dump provider. I have to added a new function here because the test can't do set_dumper_registrations_ignored_for_testing in the test. I didn't replace the existing functions because there are already 14 and 9 calls for these functions.
ssid@chromium.org changed reviewers: + haraken@chromium.org, rbyers@chromium.org
+rbyers for one line change in WebKit/public/platform/WebMemoryDumpProvider.h +haraken for web_memory_dump_provider_adapter.cc PTAL, Thanks
WebKit/Source LGTM.
Sorry I just realized that these files were removed already. Rebased it, no change in webkit. thanks.
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/1995573003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995573003/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
rbyers@chromium.org changed reviewers: - rbyers@chromium.org
On 2016/05/27 19:03:49, ssid wrote: > Sorry I just realized that these files were removed already. Rebased it, no > change in webkit. > thanks. No worries, removing myself from reviewers then
Patchset #10 (id:300001) has been deleted
dskiba@google.com changed reviewers: + dskiba@google.com
https://codereview.chromium.org/1995573003/diff/320001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1995573003/diff/320001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:58: "", // End of list marker. nit: usually nullptr is used as EOL marker. for() condition in IsNameInList() also simplifies.
still lg https://codereview.chromium.org/1995573003/diff/320001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1995573003/diff/320001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:58: "", // End of list marker. On 2016/05/31 07:06:01, Dmitry Skiba wrote: > nit: usually nullptr is used as EOL marker. for() condition in IsNameInList() > also simplifies. +1 https://codereview.chromium.org/1995573003/diff/320001/base/trace_event/memor... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1995573003/diff/320001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:75: const MemoryDumpProvider::Options& options) { probably more readable if you just add a 4th parameter |name| here and by default assign to "TestDumpProvider", e.g.: RegisterDUmpProvider(mdp, task_runner, options, const char* name = "TestDumpProvider")
Thanks, fixed. https://codereview.chromium.org/1995573003/diff/320001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1995573003/diff/320001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:58: "", // End of list marker. On 2016/05/31 07:30:55, Primiano Tucci wrote: > On 2016/05/31 07:06:01, Dmitry Skiba wrote: > > nit: usually nullptr is used as EOL marker. for() condition in IsNameInList() > > also simplifies. > > +1 Done. https://codereview.chromium.org/1995573003/diff/320001/base/trace_event/memor... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1995573003/diff/320001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:75: const MemoryDumpProvider::Options& options) { On 2016/05/31 07:30:55, Primiano Tucci wrote: > probably more readable if you just add a 4th parameter |name| here and by > default assign to "TestDumpProvider", e.g.: > > RegisterDUmpProvider(mdp, task_runner, options, const char* name = > "TestDumpProvider") Done.
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/1995573003/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995573003/340001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/1995573003/#ps340001 (title: "Use nullptr to mark end.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1995573003/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995573003/340001
Message was sent while issue was closed.
Description was changed from ========== [tracing] Introduce BACKGROUND mode in MemoryInfra This CL makes following changes: 1. Adds a BACKGROUND mode in the dump triggers in trace config. 1. Periodic dumps can be disabled on passing 0 as interval. 2. On BACKGROUND mode only whitelisted dump providers are invoked. 3. Clean up periodic dump timer set up. 4. MemoryDumpSessionState is initialized with config in the renderer also. BUG=613198 ========== to ========== [tracing] Introduce BACKGROUND mode in MemoryInfra This CL makes following changes: 1. Adds a BACKGROUND mode in the dump triggers in trace config. 1. Periodic dumps can be disabled on passing 0 as interval. 2. On BACKGROUND mode only whitelisted dump providers are invoked. 3. Clean up periodic dump timer set up. 4. MemoryDumpSessionState is initialized with config in the renderer also. BUG=613198 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== [tracing] Introduce BACKGROUND mode in MemoryInfra This CL makes following changes: 1. Adds a BACKGROUND mode in the dump triggers in trace config. 1. Periodic dumps can be disabled on passing 0 as interval. 2. On BACKGROUND mode only whitelisted dump providers are invoked. 3. Clean up periodic dump timer set up. 4. MemoryDumpSessionState is initialized with config in the renderer also. BUG=613198 ========== to ========== [tracing] Introduce BACKGROUND mode in MemoryInfra This CL makes following changes: 1. Adds a BACKGROUND mode in the dump triggers in trace config. 1. Periodic dumps can be disabled on passing 0 as interval. 2. On BACKGROUND mode only whitelisted dump providers are invoked. 3. Clean up periodic dump timer set up. 4. MemoryDumpSessionState is initialized with config in the renderer also. BUG=613198 Committed: https://crrev.com/13ebc734eab7950938afbfac677080604d972d91 Cr-Commit-Position: refs/heads/master@{#396927} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/13ebc734eab7950938afbfac677080604d972d91 Cr-Commit-Position: refs/heads/master@{#396927}
Message was sent while issue was closed.
Patchset #5 (id:200001) has been deleted
Message was sent while issue was closed.
Patchset #3 (id:80001) has been deleted
Message was sent while issue was closed.
Patchset #1 (id:1) has been deleted
Message was sent while issue was closed.
Patchset #1 (id:60001) has been deleted
Message was sent while issue was closed.
Patchset #4 (id:260001) has been deleted
Message was sent while issue was closed.
Patchset #4 (id:280001) has been deleted
Message was sent while issue was closed.
Patchset #3 (id:240001) has been deleted
Message was sent while issue was closed.
Patchset #2 (id:220001) has been deleted |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
