|
|
Description[tracing] Implement polling in MemoryDumpManager
Implement polling for memory totals when peak triggers are given in
trace config. It also triggers dumps if there is a big increase in
memory total.
BUG=607533
Review-Url: https://codereview.chromium.org/2582453002
Cr-Commit-Position: refs/heads/master@{#451386}
Committed: https://chromium.googlesource.com/chromium/src/+/5047017bdca8641764919bcbb25b60b8d89c5684
Patch Set 1 #
Total comments: 18
Patch Set 2 : reorganize. #Patch Set 3 : Keep config to MDM. #Patch Set 4 : nit. #
Total comments: 22
Patch Set 5 : fixes. #Patch Set 6 : Nit. #
Total comments: 28
Patch Set 7 : Address comments. #
Total comments: 2
Patch Set 8 : doc link and fix test. #
Messages
Total messages: 58 (42 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
ssid@chromium.org changed reviewers: + primiano@chromium.org
This CL adds polling based on config. This does not implement any peak detection, but just triggers dumps if memory is high. This logic will be updated later. This CL is mainly to get the triggering logic out of MDM. If we have to place all the logic in MDM, then the file gets too large, and it feels like both periodic and peak triggers should be in one place, since it is very similar. WDYT?
Ok I did a first pass. I like a lot the idea of splitting out the class that schedules all the trigger. I think is well encapsulated and splits out a good part of the complexity. I added some comments here and there. My major top-level comment is that there is one part that I find a bit confusing: is the fact that we pass the traceconfig trigger list around and ask the trigger class to do things that really are about the MDM. I would personally have a clearer API in the new class and NOT have it depend on the TraceConfig (also to keep our layering easier to follow), and keep some minimal parsing of the traceconfig in the MDM. So I imagine the MDM keeps reading the traceconfig but just blindly calls method on the triggers shceduler class. Take a look and see what you think https://codereview.chromium.org/2582453002/diff/80001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2582453002/diff/80001/base/BUILD.gn#newcode967 base/BUILD.gn:967: "trace_event/memory_dump_trigger.cc", remember also to update GN's bootstrap.py otherwise we keep breaking these folks https://codereview.chromium.org/2582453002/diff/80001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2582453002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:380: // supports polling. can you expand a bit here about why? To me feels a bit more natural that, at the moment we start meminfra and read the config, we setup the polling. Eventually the first polls will be no-ops because we don't have MDPs yet. But I think the right layering here should be MDM saying "I am not ready for polling yet" (using the bool retval of Poll maybe) https://codereview.chromium.org/2582453002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:384: ThreadTaskRunnerHandle::Get()); this one confuses me a bit: how do you know at this point if the mdconfig contains only a peak trigger or several triggers? https://codereview.chromium.org/2582453002/diff/80001/base/trace_event/memory... File base/trace_event/memory_dump_trigger.cc (right): https://codereview.chromium.org/2582453002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_trigger.cc:18: uint32_t g_periodic_dumps_count; why these are globals and not instance fields? Don't like too much the idea that some fields are per-instance and some are globals. I know that at the end we'll have only one instnace, but I think would be cleaner if they were all instance fields. globals often cause headaches in testing https://codereview.chromium.org/2582453002/diff/80001/base/trace_event/memory... File base/trace_event/memory_dump_trigger.h (right): https://codereview.chromium.org/2582453002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_trigger.h:24: class BASE_EXPORT MemoryDumpTrigger { I think a better name for this class would be MemoryDumpScheduler. If you think about it, this is what this class really does: decide when to trigger dumps depending on the various configuration. https://codereview.chromium.org/2582453002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_trigger.h:33: void SetupPeakTriggers( This one I find a bit confusing. You require to pass a trigger_list but then expect that the trigger_list really contains exactly one trigger of type==PEAK_MEMORY_USAGE. It smells a bit about bad layering. What I would expect here instead is: void SetPeakTrigger(uint32_t min_time_between_dumps_ms, MDLevelOfDetail) https://codereview.chromium.org/2582453002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_trigger.h:43: void Disable(); I'd be a bit more explicit and call this either ClearAllTriggers() or DisableAllTriggers() Disable() makes me doubt whether one can keep using this class afterwards. https://codereview.chromium.org/2582453002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_trigger.h:46: bool IsPeriodicDumpTimerRunning() const; "Timer running" here is an implementation detail. I think ArePeriodicDumpsEnabled or something similar is cleaner https://codereview.chromium.org/2582453002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_trigger.h:63: bool is_coordinator_; Hmm I don't think this should be here, at most in the MDM (in fact you make very little use of this here) If you think about that, coordinator is a impl detail of MDM. This class has a specific responsibility: when instructed it does all the timer and peak scheduling. It should be responsibility of the caller (MDM) to figure out if we need such responsibilities in the current process or not.
Patchset #2 (id:100001) has been deleted
I have changed this CL to have more cleaner api. I still think trace config should be parsed by scheduler. Please ignore the tests for now. I will work on splitting the MDM_unittest into 2 files. Please take another look at the overall design and let me know what you think. Thanks! On 2017/01/18 16:16:07, Primiano Tucci wrote: > Ok I did a first pass. > I like a lot the idea of splitting out the class that schedules all the trigger. > I think is well encapsulated and splits out a good part of the complexity. > I added some comments here and there. My major top-level comment is that there > is one part that I find a bit confusing: is the fact that we pass the > traceconfig trigger list around and ask the trigger class to do things that > really are about the MDM. Okay thinking about it more I think MDM should not know about trace config. It is the job of scheduler to know about the config and trigger dumps according to config. I am planning to follow up this CL by clearing all use of trace config from MDM and move it into scheduler. Currently the bit left is the heap profiler filter configuration that is set my MDM. What I am thinking is: 1. MDM does only register providers and invoke providers. It also needs to handle the heap profiling bits. 2. Scheduler will have TraceLogEnabledStateObserver and invokes MDM according to config. 3. Session state will be created and owned by Scheduler and it will be given to MDM for creating PMD and setting deduplicators. This would reduce the complexity of MDM. In this CL the schduler is still owned by MDM and the lifetime of the scheduler is the duration of the tracing session only. Maybe in future CL I can change this to use MDM. > I would personally have a clearer API in the new class and NOT have it depend on > the TraceConfig (also to keep our layering easier to follow), and keep some > minimal parsing of the traceconfig in the MDM. > So I imagine the MDM keeps reading the traceconfig but just blindly calls method > on the triggers shceduler class. > Take a look and see what you think This sounds difficult since I have to parse the trace config to get interval for each mode. I would have to create a struct that is similar to trace config that will look like vector of this: struct Trigger { int interval; LevelOfDetail mode; }; This would again be similar to trace config. https://codereview.chromium.org/2582453002/diff/80001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2582453002/diff/80001/base/BUILD.gn#newcode967 base/BUILD.gn:967: "trace_event/memory_dump_trigger.cc", On 2017/01/18 16:16:06, Primiano Tucci wrote: > remember also to update GN's bootstrap.py otherwise we keep breaking these folks Done. https://codereview.chromium.org/2582453002/diff/80001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2582453002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:380: // supports polling. On 2017/01/18 16:16:06, Primiano Tucci wrote: > can you expand a bit here about why? To me feels a bit more natural that, at the > moment we start meminfra and read the config, we setup the polling. > Eventually the first polls will be no-ops because we don't have MDPs yet. > But I think the right layering here should be MDM saying "I am not ready for > polling yet" (using the bool retval of Poll maybe) If I did that then we would be unnecessarily setting timer every 25ms and polling nothing. This is a major concern in Linux where we will never register dump providers with polling in renderer. So, we shouldn't be start polling unless we have dump providers. I changed it to Notify instead of SetupPeakTriggers. Does that look better? https://codereview.chromium.org/2582453002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:384: ThreadTaskRunnerHandle::Get()); On 2017/01/18 16:16:06, Primiano Tucci wrote: > this one confuses me a bit: how do you know at this point if the mdconfig > contains only a peak trigger or several triggers? So, now it does not matter. The scheduler knows if it should setup polling. I did not want to read the config at two places (OnTraceLogEnabled and here) to start polling. that is why I checked if poling config exists in scheduler itself. https://codereview.chromium.org/2582453002/diff/80001/base/trace_event/memory... File base/trace_event/memory_dump_trigger.cc (right): https://codereview.chromium.org/2582453002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_trigger.cc:18: uint32_t g_periodic_dumps_count; On 2017/01/18 16:16:06, Primiano Tucci wrote: > why these are globals and not instance fields? Don't like too much the idea that > some fields are per-instance and some are globals. > I know that at the end we'll have only one instnace, but I think would be > cleaner if they were all instance fields. globals often cause headaches in > testing I changed all of these to instance field of specific classes. I kept them here because it spammed the header file. But it looks fine with 2 classes now. https://codereview.chromium.org/2582453002/diff/80001/base/trace_event/memory... File base/trace_event/memory_dump_trigger.h (right): https://codereview.chromium.org/2582453002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_trigger.h:24: class BASE_EXPORT MemoryDumpTrigger { On 2017/01/18 16:16:06, Primiano Tucci wrote: > I think a better name for this class would be MemoryDumpScheduler. If you think > about it, this is what this class really does: decide when to trigger dumps > depending on the various configuration. Makes sense. I didn't find a better name. https://codereview.chromium.org/2582453002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_trigger.h:33: void SetupPeakTriggers( On 2017/01/18 16:16:07, Primiano Tucci wrote: > This one I find a bit confusing. You require to pass a trigger_list but then > expect that the trigger_list really contains exactly one trigger of > type==PEAK_MEMORY_USAGE. It smells a bit about bad layering. > What I would expect here instead is: > void SetPeakTrigger(uint32_t min_time_between_dumps_ms, MDLevelOfDetail) 2 reasons: 1. Maybe in future we will need more than one peak trigger. 2. It is consistent with the other function that sets up periodic dump triggers. But, yes it's a bit weird to pass a list of config here. I changed it to just one. https://codereview.chromium.org/2582453002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_trigger.h:43: void Disable(); On 2017/01/18 16:16:07, Primiano Tucci wrote: > I'd be a bit more explicit and call this either ClearAllTriggers() or > DisableAllTriggers() > Disable() makes me doubt whether one can keep using this class afterwards. Done. https://codereview.chromium.org/2582453002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_trigger.h:46: bool IsPeriodicDumpTimerRunning() const; On 2017/01/18 16:16:07, Primiano Tucci wrote: > "Timer running" here is an implementation detail. I think > ArePeriodicDumpsEnabled or something similar is cleaner Done. https://codereview.chromium.org/2582453002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_trigger.h:63: bool is_coordinator_; On 2017/01/18 16:16:07, Primiano Tucci wrote: > Hmm I don't think this should be here, at most in the MDM (in fact you make very > little use of this here) > If you think about that, coordinator is a impl detail of MDM. > This class has a specific responsibility: when instructed it does all the timer > and peak scheduling. > It should be responsibility of the caller (MDM) to figure out if we need such > responsibilities in the current process or not. Yes I think it's fair to keep this in MDM. I moved it here because there was no other use of this bool in MDM other than to not set periodic timer.
Patchset #3 (id:140001) has been deleted
Patchset #3 (id:160001) has been deleted
Description was changed from ========== [tracing] Implement polling in MemoryDumpManager Implement polling for memory totals when peak triggers are given in trace config. It also triggers dumps if there is a big increase in memory total. BUG=607533 ========== to ========== [tracing] Implement polling in MemoryDumpManager Implement polling for memory totals when peak triggers are given in trace config. It also triggers dumps if there is a big increase in memory total. BUG=607533 ==========
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/v2/patch-status/codereview.chromium.or...
Primiano, PTAL. It seems difficult to write separate unit tests for this scheduler. WDYT?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Some comments, will do a final pass tomorrow. Design looks great now, thanks a lot for reworking on this. https://codereview.chromium.org/2582453002/diff/200001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2582453002/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:436: << "All polling MDPs cannot be unregistered, since it will cause polling " i'd remove the "since... " part here. https://codereview.chromium.org/2582453002/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:695: AutoLock lock(lock_); I think I wrote this in another cl but forgot the discussion. I think there is a tiny window where you can hit this DCHECK even if everything is still legit. the window would be: - OnTraceLogDisabled - the dump_thread_ is std::move'd in the local dump_thread variable. - dump thread is being stopped (but still active) but dump_thread_ is nullptr now. - this function gets called on the dump thread -> crash https://codereview.chromium.org/2582453002/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:790: const TraceConfig::MemoryDumpConfig memory_dump_config = since you don't need to copy this anymore, you can jsut make this a const MDConfig& and keep just a cheap reference https://codereview.chromium.org/2582453002/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:821: WrapUnique(new MemoryDumpScheduler(this, dump_thread->task_runner()))); no need to WrapUnique if you do this explicitly. Essentially either you do: unique_ptr<X> var(new X(...)); or auto var = MakeUnique(new X(...)) so you can safely remove the WrapUnique here https://codereview.chromium.org/2582453002/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:822: DCHECK_LE(memory_dump_config.triggers.size(), 3u); shouldn't this dcheck be in the scheduler? https://codereview.chromium.org/2582453002/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:824: DCHECK(session_state->IsDumpModeAllowed(trigger.level_of_detail)); shoudln't this be: if (!allowed) { NOTREACHED(); continue; } to avoid accidentally doing non-allowed dumps in release builds, where dcheck has no effect? https://codereview.chromium.org/2582453002/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:867: std::unique_ptr<MemoryDumpScheduler> scheduler = nullptr; no need for the = nullptr part, it should be implicit. https://codereview.chromium.org/2582453002/diff/200001/base/trace_event/memor... File base/trace_event/memory_dump_scheduler.h (right): https://codereview.chromium.org/2582453002/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.h:39: void NotifyPeriodicTriggerSupported(); thinking more, should these methods just be called: EnablePeriodicTriggers() and EnablePollingTriggers() ? https://codereview.chromium.org/2582453002/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.h:52: class PeriodicDumpScheduler { aren't these sub-classes a bit too much overkill? https://codereview.chromium.org/2582453002/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.h:62: void NotifySupported(); just Enable() https://codereview.chromium.org/2582453002/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.h:109: void NotifySupported(); ditto
please see replies inline https://codereview.chromium.org/2582453002/diff/200001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2582453002/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:436: << "All polling MDPs cannot be unregistered, since it will cause polling " On 2017/01/26 02:52:22, Primiano Tucci wrote: > i'd remove the "since... " part here. Done. https://codereview.chromium.org/2582453002/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:695: AutoLock lock(lock_); On 2017/01/26 02:52:22, Primiano Tucci wrote: > I think I wrote this in another cl but forgot the discussion. > I think there is a tiny window where you can hit this DCHECK even if everything > is still legit. > the window would be: > - OnTraceLogDisabled > - the dump_thread_ is std::move'd in the local dump_thread variable. > - dump thread is being stopped (but still active) but dump_thread_ is nullptr > now. > - this function gets called on the dump thread -> crash Yes that is true. That is why I am holding a lock here to access dump_thread_ and we have a condition that says if(dump_thread_) If you feel that this check is not necessary I can remove it. I felt that we need to have this check since MDM no longer calls this method and a different class calls it. https://codereview.chromium.org/2582453002/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:790: const TraceConfig::MemoryDumpConfig memory_dump_config = On 2017/01/26 02:52:22, Primiano Tucci wrote: > since you don't need to copy this anymore, you can jsut make this a const > MDConfig& and keep just a cheap reference Done. https://codereview.chromium.org/2582453002/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:821: WrapUnique(new MemoryDumpScheduler(this, dump_thread->task_runner()))); On 2017/01/26 02:52:23, Primiano Tucci wrote: > no need to WrapUnique if you do this explicitly. > Essentially either you do: > unique_ptr<X> var(new X(...)); > or > auto var = MakeUnique(new X(...)) > > so you can safely remove the WrapUnique here Done. https://codereview.chromium.org/2582453002/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:822: DCHECK_LE(memory_dump_config.triggers.size(), 3u); On 2017/01/26 02:52:23, Primiano Tucci wrote: > shouldn't this dcheck be in the scheduler? We cannot easily have this in scheduler since we need to keep track of how many triggers are being added. That would add an extra state in scheduler. Felt it was useless. It also makes sense to be here because the AddTrigger() api has a restriction of at max 3 triggers. https://codereview.chromium.org/2582453002/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:824: DCHECK(session_state->IsDumpModeAllowed(trigger.level_of_detail)); On 2017/01/26 02:52:23, Primiano Tucci wrote: > shoudln't this be: > if (!allowed) { > NOTREACHED(); > continue; > } > > to avoid accidentally doing non-allowed dumps in release builds, where dcheck > has no effect? Done. https://codereview.chromium.org/2582453002/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:867: std::unique_ptr<MemoryDumpScheduler> scheduler = nullptr; On 2017/01/26 02:52:23, Primiano Tucci wrote: > no need for the = nullptr part, it should be implicit. Done. https://codereview.chromium.org/2582453002/diff/200001/base/trace_event/memor... File base/trace_event/memory_dump_scheduler.h (right): https://codereview.chromium.org/2582453002/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.h:39: void NotifyPeriodicTriggerSupported(); On 2017/01/26 02:52:23, Primiano Tucci wrote: > thinking more, should these methods just be called: > EnablePeriodicTriggers() and EnablePollingTriggers() ? Yes I was thinking the same. But felt enable is a stronger command to be given here. By that I mean, EnablePeriodicTriggers() would enable the timer only if periodic triggers were added. Else it would just do nothing. Now, in MDM we cannot keep track of whether periodic triggers were added (need to read trace config again). So, notify is a better option. https://codereview.chromium.org/2582453002/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.h:52: class PeriodicDumpScheduler { On 2017/01/26 02:52:23, Primiano Tucci wrote: > aren't these sub-classes a bit too much overkill? The main reason for having them is because I wanted to keep the members separate. Otherwise there are too many members in this class and we could easily miss access some state of periodic dumps in a peak detection function. The other reason is the names of all members were getting too long. periodic_heavy_dumps_rate and periodic_heavy_dump_period_ms. Instead it's better to have it in one class and use shorter names. Especially when we have better peak detection logic we will have more states with long names. Also avoids communication between the two schedulers. https://codereview.chromium.org/2582453002/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.h:62: void NotifySupported(); On 2017/01/26 02:52:23, Primiano Tucci wrote: > just Enable() Same reply as above. Enable() does not enable unless triggers were added. No reason to keep track of periodic triggers or peak triggers in current session in the scheduler class.
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
https://codereview.chromium.org/2582453002/diff/200001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2582453002/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:790: const TraceConfig::MemoryDumpConfig memory_dump_config = On 2017/01/26 21:48:57, ssid wrote: > On 2017/01/26 02:52:22, Primiano Tucci wrote: > > since you don't need to copy this anymore, you can jsut make this a const > > MDConfig& and keep just a cheap reference > > Done. Actually this would need keep alive a copy of trace config here.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Primiano, ping.
sorry for the delay on this. Everything looks good (% some minor comment here and there), just one thing: honestly I am not fully sold on the two nested classes in the scheduler. See the more detailed comment below, it makes the code quite hard to read. I think you can siimplify it by using only two structs to keep the fields https://codereview.chromium.org/2582453002/diff/240001/base/trace_event/memor... File base/trace_event/memory_dump_scheduler.cc (right): https://codereview.chromium.org/2582453002/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.cc:103: DCHECK_EQ(0u, light_dump_period_ms % min_timer_period_ms_); I'd move these checks to AddTrigger, so the stacktrace will shop up at the time we read them form the config, and not only later when we enable this. here you can leave just a check min_timer_period > 0, or similar https://codereview.chromium.org/2582453002/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.cc:157: DCHECK(!polling_task_runner_) << "Polling was not disabled safely"; remove the string comment, the code in the dcheck seems clear enough https://codereview.chromium.org/2582453002/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.cc:160: void MemoryDumpScheduler::PeakDumpScheduler::AddTrigger( shouldn't this method just be called Configure() or Initialize(). You can't support more than 1 trigger here really https://codereview.chromium.org/2582453002/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.cc:166: dump_mode_ = level_of_detail; just call the variable level_of_detail_ ? never been a huge fan of variables that change name along the road https://codereview.chromium.org/2582453002/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.cc:169: is_setup_ = true; s/is_setup_/is_initialized_/ (or is_configured_) https://codereview.chromium.org/2582453002/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.cc:175: num_polls_from_last_dump_ = 0; can you add some logic (either a check or a return) to prevent that this doesn't et accidentally called twice, firing 2x timers https://codereview.chromium.org/2582453002/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.cc:202: TRACE_COUNTER1(MemoryDumpManager::kTraceCategory, "MemoryMetricMB", MemoryMetricMB -> PolledMemoryMB ? https://codereview.chromium.org/2582453002/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.cc:211: FROM_HERE, out of curiosity why reposting instead of using Time(is_repeating=true)? (no need to change it here, can do in a follow up CL, if this make sense) https://codereview.chromium.org/2582453002/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.cc:223: ++num_polls_from_last_dump_; can you make all this in early-out style, without using should_dump? might be easier to follow. e.g., ++num_polls_from_last_dump_; if (num_polls_from_last_dump_ < min_polls_between_dumps_) return false; ... if (last_dump_memory_total && increase_from_last_dump < memory_increase_threshold_) return false; TRACE_EVENT... return true https://codereview.chromium.org/2582453002/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.cc:226: should_dump |= true; well, really no need for the |= here :) just = true should be enough https://codereview.chromium.org/2582453002/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.cc:238: "Peak memory dump Triggered", I'd move this above in Poll...OnThread, so this is emitted in the same function of the other one. https://codereview.chromium.org/2582453002/diff/240001/base/trace_event/memor... File base/trace_event/memory_dump_scheduler.h (right): https://codereview.chromium.org/2582453002/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.h:73: bool is_setup() const { return is_setup_; } s/is_setup/has_triggers/ ? or (is_configured, to keep it consistent with the other one) https://codereview.chromium.org/2582453002/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.h:87: uint32_t light_dump_period_ms = 0; why these 2 are initialized here, but not the others (dump_count and friends) ? can you just initialize everything in the ctor? https://codereview.chromium.org/2582453002/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.h:93: class PeakDumpScheduler { honestly I am not fully sold you need two classes here. it seems you are forwarding most of the methods, which makes the code look more complex than what it is. How about just having two structs, with the timing parametrs for polling and triggers? something like class MemoryDumpScheduler { private: struct PeakPollConfig { int num_polls_.... .. }; struct PeriodicCOnfig { int .... } }
Patchset #7 (id:260001) has been deleted
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #7 (id:280001) has been deleted
Patchset #8 (id:320001) has been deleted
Patchset #7 (id:300001) has been deleted
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/v2/patch-status/codereview.chromium.or...
Made changes, ptal thanks! https://codereview.chromium.org/2582453002/diff/240001/base/trace_event/memor... File base/trace_event/memory_dump_scheduler.cc (right): https://codereview.chromium.org/2582453002/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.cc:103: DCHECK_EQ(0u, light_dump_period_ms % min_timer_period_ms_); On 2017/02/03 20:08:19, Primiano Tucci wrote: > I'd move these checks to AddTrigger, so the stacktrace will shop up at the time > we read them form the config, and not only later when we enable this. > here you can leave just a check min_timer_period > 0, or similar Um, we will be doing the check multiple times when we can do it at the end. But its okay. https://codereview.chromium.org/2582453002/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.cc:157: DCHECK(!polling_task_runner_) << "Polling was not disabled safely"; On 2017/02/03 20:08:19, Primiano Tucci wrote: > remove the string comment, the code in the dcheck seems clear enough Done. https://codereview.chromium.org/2582453002/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.cc:160: void MemoryDumpScheduler::PeakDumpScheduler::AddTrigger( On 2017/02/03 20:08:19, Primiano Tucci wrote: > shouldn't this method just be called Configure() or Initialize(). > You can't support more than 1 trigger here really Removed this method since this is a struct. https://codereview.chromium.org/2582453002/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.cc:166: dump_mode_ = level_of_detail; On 2017/02/03 20:08:19, Primiano Tucci wrote: > just call the variable level_of_detail_ ? never been a huge fan of variables > that change name along the road Done. https://codereview.chromium.org/2582453002/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.cc:169: is_setup_ = true; On 2017/02/03 20:08:19, Primiano Tucci wrote: > s/is_setup_/is_initialized_/ (or is_configured_) Done. https://codereview.chromium.org/2582453002/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.cc:175: num_polls_from_last_dump_ = 0; On 2017/02/03 20:08:20, Primiano Tucci wrote: > can you add some logic (either a check or a return) to prevent that this > doesn't et accidentally called twice, firing 2x timers Done. https://codereview.chromium.org/2582453002/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.cc:202: TRACE_COUNTER1(MemoryDumpManager::kTraceCategory, "MemoryMetricMB", On 2017/02/03 20:08:19, Primiano Tucci wrote: > MemoryMetricMB -> PolledMemoryMB ? Done. https://codereview.chromium.org/2582453002/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.cc:211: FROM_HERE, On 2017/02/03 20:08:19, Primiano Tucci wrote: > out of curiosity why reposting instead of using Time(is_repeating=true)? (no > need to change it here, can do in a follow up CL, if this make sense) I did not use timer here because in future we plan to do smarter polling based on background processes. https://codereview.chromium.org/2582453002/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.cc:223: ++num_polls_from_last_dump_; On 2017/02/03 20:08:19, Primiano Tucci wrote: > can you make all this in early-out style, without using should_dump? might be > easier to follow. e.g., > > ++num_polls_from_last_dump_; > if (num_polls_from_last_dump_ < min_polls_between_dumps_) > return false; > ... > if (last_dump_memory_total && increase_from_last_dump < > memory_increase_threshold_) > return false; > > TRACE_EVENT... > return true The code would look like: if (polling_state_.last_dump_memory_total == 0) { polling_state_.last_dump_memory_total = current_memory_total; return true; } ++polling_state_.num_polls_from_last_dump; if (polling_state_.min_polls_between_dumps > polling_state_.num_polls_from_last_dump) { return false; } int64_t increase_from_last_dump = current_memory_total - polling_state_.last_dump_memory_total; if (increase_from_last_dump < polling_state_.memory_increase_threshold) return false; polling_state_.last_dump_memory_total = current_memory_total; polling_state_.num_polls_from_last_dump = 0; return true; I don't like the ides of returning true from 2 places and the fact that we have to reset few values when returning true makes it weird. So keeping the current pattern. I can't do early out because of the fact that I want to return true for the first poll and also set the last_total's value. https://codereview.chromium.org/2582453002/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.cc:226: should_dump |= true; On 2017/02/03 20:08:19, Primiano Tucci wrote: > well, really no need for the |= here :) just = true should be enough Done. https://codereview.chromium.org/2582453002/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.cc:238: "Peak memory dump Triggered", On 2017/02/03 20:08:19, Primiano Tucci wrote: > I'd move this above in Poll...OnThread, so this is emitted in the same function > of the other one. Done. https://codereview.chromium.org/2582453002/diff/240001/base/trace_event/memor... File base/trace_event/memory_dump_scheduler.h (right): https://codereview.chromium.org/2582453002/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.h:73: bool is_setup() const { return is_setup_; } On 2017/02/03 20:08:20, Primiano Tucci wrote: > s/is_setup/has_triggers/ ? or (is_configured, to keep it consistent with the > other one) Done. https://codereview.chromium.org/2582453002/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.h:87: uint32_t light_dump_period_ms = 0; On 2017/02/03 20:08:20, Primiano Tucci wrote: > why these 2 are initialized here, but not the others (dump_count and friends) ? > can you just initialize everything in the ctor? Sorry this was mistake. https://codereview.chromium.org/2582453002/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.h:93: class PeakDumpScheduler { On 2017/02/03 20:08:20, Primiano Tucci wrote: > honestly I am not fully sold you need two classes here. it seems you are > forwarding most of the methods, which makes the code look more complex than what > it is. > How about just having two structs, with the timing parametrs for polling and > triggers? > something like > > class MemoryDumpScheduler { > > private: > struct PeakPollConfig { > int num_polls_.... > .. > }; > struct PeriodicCOnfig { > int .... > } > } yes I agree this should be the case. I did aim for it initially then changed my mind because there were too many references to the fields here. I had to use "peak_poll_state_.light_dump_period_ms" at each access to this data. This just felt too long and harder to read the implementation than going to one extra function call because of the class. Made it struct, slightly unhappy about too many reference to the members, but doesn't look bad.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
LGTM thanks https://codereview.chromium.org/2582453002/diff/340001/base/trace_event/memor... File base/trace_event/memory_dump_scheduler.cc (right): https://codereview.chromium.org/2582453002/diff/340001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.cc:16: // Threshold on increase in memory from last dump beyond which a new dump must maybe you can add a goo.gl link to your doc or the bug where the doc is linked
Thanks. https://codereview.chromium.org/2582453002/diff/340001/base/trace_event/memor... File base/trace_event/memory_dump_scheduler.cc (right): https://codereview.chromium.org/2582453002/diff/340001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.cc:16: // Threshold on increase in memory from last dump beyond which a new dump must On 2017/02/15 18:13:44, Primiano Tucci wrote: > maybe you can add a goo.gl link to your doc or the bug where the doc is linked I added it in the ShouldTriggerDump function.
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/v2/patch-status/codereview.chromium.or...
ssid@chromium.org changed reviewers: + thakis@chromium.org
+Nico for the BUILD file edit. Thanks
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.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/2582453002/#ps360001 (title: "doc link and fix test.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 360001, "attempt_start_ts": 1487361815301540, "parent_rev": "4a1e5ced806c4452252e6531e50b3549b31f5f96", "commit_rev": "5047017bdca8641764919bcbb25b60b8d89c5684"}
Message was sent while issue was closed.
Description was changed from ========== [tracing] Implement polling in MemoryDumpManager Implement polling for memory totals when peak triggers are given in trace config. It also triggers dumps if there is a big increase in memory total. BUG=607533 ========== to ========== [tracing] Implement polling in MemoryDumpManager Implement polling for memory totals when peak triggers are given in trace config. It also triggers dumps if there is a big increase in memory total. BUG=607533 Review-Url: https://codereview.chromium.org/2582453002 Cr-Commit-Position: refs/heads/master@{#451386} Committed: https://chromium.googlesource.com/chromium/src/+/5047017bdca8641764919bcbb25b... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:360001) as https://chromium.googlesource.com/chromium/src/+/5047017bdca8641764919bcbb25b... |