Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2799023002:
memory-infra: Switch to MemoryPeakDetector and simplify MemoryDumpScheduler (Closed)
|
Created:
3 years, 8 months ago by Primiano Tucci (use gerrit) Modified:
3 years, 8 months ago CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org, danakj+watch_chromium.org, vmpstr+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionmemory-infra: Switch to MemoryPeakDetector and simplify MemoryDumpScheduler
This CL concludes the refactoring of the old monolithic MemoryDumpScheduler in
the new MemoryPeakDetector (for peaks) + MemoryDumpScheduler (for periodic
dumps only). This refactoring was mostly done to clean up the entanglement of
threading dependencies between MemoryDumpManager and the scheduler.
Note for perf sheriffs:
----------------------
If memory metrics are skewed, or system_health memory benchmarks start
failing this is very likely the culprit. Proceed with revert in this case.
BUG=607533
Review-Url: https://codereview.chromium.org/2799023002
Cr-Commit-Position: refs/heads/master@{#463991}
Committed: https://chromium.googlesource.com/chromium/src/+/57cee813cd7d1c400275a5cb8e95612a4fae0fd4
Patch Set 1 #Patch Set 2 : . #
Total comments: 17
Patch Set 3 : ssid comments + add more tests #
Total comments: 11
Patch Set 4 : address reviews comments #Patch Set 5 : Add extra BASE_EXPORT to make win happy, mah #Patch Set 6 : rebase bind -> bindonce #
Messages
Total messages: 36 (22 generated)
primiano@chromium.org changed reviewers: + hjd@chromium.org, ssid@chromium.org
Still WIP here, but you can get a feeling of the final picture. ARchitectural comments welcome. Will sort out the rest of the code in the next days.
https://codereview.chromium.org/2799023002/diff/20001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2799023002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:869: Bind(&OnPeakDetected, trigger.level_of_detail)); Actually there is no reason to have a Setup() and Start() function in PeakDetector anymore. We needed 2 earlier because we added triggers and then enabled only when we have dump providers. If we are going to call it from the same place here then why have 2 functions? I tried to point this in the first CL, but I thought you had some other issue here. https://codereview.chromium.org/2799023002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:876: Can we not add support for periodic+peak triggers in single session now? I can't think of anything that would break. But, it sounds not very useful anyway. https://codereview.chromium.org/2799023002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:878: MemoryPeakDetector::GetInstance()->Start(peak_config); can move this inot the if above. https://codereview.chromium.org/2799023002/diff/20001/base/trace_event/memory... File base/trace_event/memory_dump_scheduler.cc (right): https://codereview.chromium.org/2799023002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_scheduler.cc:102: SequencedTaskRunnerHandle::Get()->PostDelayedTask( Can we do this posttask at the start of this function? the callback could possibly take locks and if it's slow then we will mess up the timer. https://codereview.chromium.org/2799023002/diff/20001/base/trace_event/memory... File base/trace_event/memory_dump_scheduler.h (right): https://codereview.chromium.org/2799023002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_scheduler.h:27: using PeriodicCallback = RepeatingCallback<void(MemoryDumpLevelOfDetail)>; This feels unnecessary. But, ok. https://codereview.chromium.org/2799023002/diff/20001/base/trace_event/memory... File base/trace_event/memory_dump_scheduler_unittest.cc (right): https://codereview.chromium.org/2799023002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_scheduler_unittest.cc:56: TEST_F(MemoryDumpSchedulerTest, SingleTrigger) { We should remove some tests from MDM maybe, which has too many tests related to the timer.
https://codereview.chromium.org/2799023002/diff/20001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2799023002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:463: for (const scoped_refptr<MemoryDumpProviderInfo>& mdp : dump_providers_) { We could DCHECK providers is empty or use a set so we can't end up appending the same mdps to the list over and over.
The CQ bit was checked by primiano@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...
Description was changed from ========== memory-infra: Switch to MemoryPeakDetector and simplify MemoryDumpScheduler WIP BUG= ========== to ========== memory-infra: Switch to MemoryPeakDetector and simplify MemoryDumpScheduler This CL concludes the refactoring of the old monolithic MemoryDumpScheduler in the new MemoryPeakDetector (for peaks) + MemoryDumpScheduler (for periodic dumps only). This refactoring was mostly done to clean up the entanglement of threading dependencies between MemoryDumpManager and the scheduler. Note for perf sheriffs: ---------------------- If memory metrics are skewed, or system_health memory benchmarks start failing this is very likely the culprit. Proceed with revert in this case. BUG=607533 ==========
CL Updated, PTAL https://codereview.chromium.org/2799023002/diff/20001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2799023002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:463: for (const scoped_refptr<MemoryDumpProviderInfo>& mdp : dump_providers_) { On 2017/04/06 12:14:17, hjd wrote: > We could DCHECK providers is empty or use a set so we can't end up appending the > same mdps to the list over and over. Good point. done. https://codereview.chromium.org/2799023002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:869: Bind(&OnPeakDetected, trigger.level_of_detail)); On 2017/04/05 22:04:22, ssid wrote: > Actually there is no reason to have a Setup() and Start() function in > PeakDetector anymore. We needed 2 earlier because we added triggers and then > enabled only when we have dump providers. If we are going to call it from the > same place here then why have 2 functions? > I tried to point this in the first CL, but I thought you had some other issue > here. I want to keep them separate because they will be useful for uma when the peak detector will be used by the service ;-) https://codereview.chromium.org/2799023002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:876: On 2017/04/05 22:04:22, ssid wrote: > Can we not add support for periodic+peak triggers in single session now? > I can't think of anything that would break. But, it sounds not very useful > anyway. Is there a specific reason? Nothing should break right? I think we might want to use it at some point soon-ish in telemetry. https://codereview.chromium.org/2799023002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:878: MemoryPeakDetector::GetInstance()->Start(peak_config); On 2017/04/05 22:04:22, ssid wrote: > can move this inot the if above. ? https://codereview.chromium.org/2799023002/diff/20001/base/trace_event/memory... File base/trace_event/memory_dump_scheduler.cc (right): https://codereview.chromium.org/2799023002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_scheduler.cc:102: SequencedTaskRunnerHandle::Get()->PostDelayedTask( On 2017/04/05 22:04:22, ssid wrote: > Can we do this posttask at the start of this function? > the callback could possibly take locks and if it's slow then we will mess up the > timer. Excellent point. done. https://codereview.chromium.org/2799023002/diff/20001/base/trace_event/memory... File base/trace_event/memory_dump_scheduler_unittest.cc (right): https://codereview.chromium.org/2799023002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_scheduler_unittest.cc:56: TEST_F(MemoryDumpSchedulerTest, SingleTrigger) { On 2017/04/05 22:04:22, ssid wrote: > We should remove some tests from MDM maybe, which has too many tests related to > the timer. I simplified a bit the TestPollingOnDumpThread limint that to check that th traceconfig works properly end to end. DIdn't find anything else too relevant (but very lkely I missed something, open to suggestions)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
lgtm Thanks! https://codereview.chromium.org/2799023002/diff/20001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2799023002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:876: On 2017/04/10 19:16:41, Primiano Tucci wrote: > On 2017/04/05 22:04:22, ssid wrote: > > Can we not add support for periodic+peak triggers in single session now? > > I can't think of anything that would break. But, it sounds not very useful > > anyway. > > Is there a specific reason? Nothing should break right? I think we might want to > use it at some point soon-ish in telemetry. yeah should work fine. I will give it a try once the refactoring is done. https://codereview.chromium.org/2799023002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:878: MemoryPeakDetector::GetInstance()->Start(peak_config); On 2017/04/10 19:16:41, Primiano Tucci wrote: > On 2017/04/05 22:04:22, ssid wrote: > > can move this inot the if above. > > ? I meant this can be done in the if (trigger.trigger_type == MemoryDumpType::PEAK_MEMORY_USAGE) condition above instead of checking for time being > 0 here. Easier to read. https://codereview.chromium.org/2799023002/diff/20001/base/trace_event/memory... File base/trace_event/memory_dump_scheduler_unittest.cc (right): https://codereview.chromium.org/2799023002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_scheduler_unittest.cc:56: TEST_F(MemoryDumpSchedulerTest, SingleTrigger) { On 2017/04/10 19:16:41, Primiano Tucci wrote: > On 2017/04/05 22:04:22, ssid wrote: > > We should remove some tests from MDM maybe, which has too many tests related > to > > the timer. > > I simplified a bit the TestPollingOnDumpThread limint that to check that th > traceconfig works properly end to end. DIdn't find anything else too relevant > (but very lkely I missed something, open to suggestions) You are right. They test trace config parsing. https://codereview.chromium.org/2799023002/diff/40001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2799023002/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:886: MemoryPeakDetector::GetInstance()->Setup( Maybe a dcheck here to avoid calling setup multiple times? ie we shouldn't have a config with multiple peak triggers. https://codereview.chromium.org/2799023002/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:899: MemoryPeakDetector::GetInstance()->Start(peak_config); Can we find some way to trigger a memory dump at the start of tracing in case peak detection is enabled? It'd be good to have a reference point to compare peak to. Makes finding issues easier. Also this was the behavior before the switch. https://codereview.chromium.org/2799023002/diff/40001/base/trace_event/memory... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/2799023002/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:789: EXPECT_CALL(*mdp1, SuspendFastMemoryPolling()).Times(1); Why not EXPECT_CALL(*mdp2, SuspendFastMemoryPolling()).Times(1); Actually just realized ~ProcessMetricsMemoryDumpProvider() does not close the fast_polling_statm_fd_
https://codereview.chromium.org/2799023002/diff/40001/base/trace_event/memory... File base/trace_event/memory_dump_scheduler.cc (right): https://codereview.chromium.org/2799023002/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_scheduler.cc:63: min_period_ms = std::min(min_period_ms, trigger.period_ms); #include <algorithm>?
https://codereview.chromium.org/2799023002/diff/40001/base/trace_event/memory... File base/trace_event/memory_dump_scheduler.h (right): https://codereview.chromium.org/2799023002/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_scheduler.h:48: bool is_enabled_for_testing() const { return task_runner_ ? true : false; } task_runner_ ? true : false; is this the idiom? Naively bool(task_runner_) seems cleaner if it does the same thing.
On 2017/04/11 10:02:30, hjd wrote: > https://codereview.chromium.org/2799023002/diff/40001/base/trace_event/memory... > File base/trace_event/memory_dump_scheduler.h (right): > > https://codereview.chromium.org/2799023002/diff/40001/base/trace_event/memory... > base/trace_event/memory_dump_scheduler.h:48: bool is_enabled_for_testing() const > { return task_runner_ ? true : false; } > task_runner_ ? true : false; > is this the idiom? Naively bool(task_runner_) seems cleaner if it does the same > thing. lgtm other than comments above
The CQ bit was checked by primiano@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...
https://codereview.chromium.org/2799023002/diff/20001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2799023002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:878: MemoryPeakDetector::GetInstance()->Start(peak_config); On 2017/04/10 21:35:51, ssid wrote: > On 2017/04/10 19:16:41, Primiano Tucci wrote: > > On 2017/04/05 22:04:22, ssid wrote: > > > can move this inot the if above. > > > > ? > > I meant this can be done in the > if (trigger.trigger_type == MemoryDumpType::PEAK_MEMORY_USAGE) > condition above instead of checking for time being > 0 here. Easier to read. Ahh. Done https://codereview.chromium.org/2799023002/diff/40001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2799023002/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:886: MemoryPeakDetector::GetInstance()->Setup( On 2017/04/10 21:35:51, ssid wrote: > Maybe a dcheck here to avoid calling setup multiple times? ie we shouldn't have > a config with multiple peak triggers. Done. https://codereview.chromium.org/2799023002/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:899: MemoryPeakDetector::GetInstance()->Start(peak_config); On 2017/04/10 21:35:51, ssid wrote: > Can we find some way to trigger a memory dump at the start of tracing in case > peak detection is enabled? It'd be good to have a reference point to compare > peak to. Makes finding issues easier. Also this was the behavior before the > switch. Done. https://codereview.chromium.org/2799023002/diff/40001/base/trace_event/memory... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/2799023002/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:789: EXPECT_CALL(*mdp1, SuspendFastMemoryPolling()).Times(1); On 2017/04/10 21:35:51, ssid wrote: > Why not > > EXPECT_CALL(*mdp2, SuspendFastMemoryPolling()).Times(1); Because I realized that there is no point calling SuspendFastMemoryPolling() at unregistration, as the MDP will just be destroyed, and the FD will be closed in its dtor. So I kept it only for the stop case. I made the comment and the code more explicit though. > Actually just realized ~ProcessMetricsMemoryDumpProvider() does not close the > fast_polling_statm_fd_ Unless i a missing something major, It does. primiano-from-the-past was quite parnoid and asked you to use a base::ScopedFD fast_polling_statm_fd_ :) it should be auto closed. https://codereview.chromium.org/2799023002/diff/40001/base/trace_event/memory... File base/trace_event/memory_dump_scheduler.cc (right): https://codereview.chromium.org/2799023002/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_scheduler.cc:63: min_period_ms = std::min(min_period_ms, trigger.period_ms); On 2017/04/11 09:42:55, hjd wrote: > #include <algorithm>? Good point done. (I always find funny that in order to get a "min" function I have to include "algorithm", as if I was doing some crazy A* on a graph) https://codereview.chromium.org/2799023002/diff/40001/base/trace_event/memory... File base/trace_event/memory_dump_scheduler.h (right): https://codereview.chromium.org/2799023002/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_scheduler.h:48: bool is_enabled_for_testing() const { return task_runner_ ? true : false; } On 2017/04/11 10:02:30, hjd wrote: > task_runner_ ? true : false; > is this the idiom? Naively bool(task_runner_) seems cleaner if it does the same > thing. Ah I thought it wouldn't have triggered the bool cast operator instead it does. C++.
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...)
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by primiano@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...)
https://codereview.chromium.org/2799023002/diff/40001/base/trace_event/memory... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/2799023002/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:789: EXPECT_CALL(*mdp1, SuspendFastMemoryPolling()).Times(1); On 2017/04/11 11:43:08, Primiano Tucci wrote: > On 2017/04/10 21:35:51, ssid wrote: > > Why not > > > > EXPECT_CALL(*mdp2, SuspendFastMemoryPolling()).Times(1); > Because I realized that there is no point calling SuspendFastMemoryPolling() at > unregistration, as the MDP will just be destroyed, and the FD will be closed in > its dtor. So I kept it only for the stop case. > > I made the comment and the code more explicit though. > > > Actually just realized ~ProcessMetricsMemoryDumpProvider() does not close the > > fast_polling_statm_fd_ > Unless i a missing something major, It does. > primiano-from-the-past was quite parnoid and asked you to use a base::ScopedFD > fast_polling_statm_fd_ :) > it should be auto closed. Haha true!
The CQ bit was checked by primiano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ssid@chromium.org, hjd@chromium.org Link to the patchset: https://codereview.chromium.org/2799023002/#ps100001 (title: "Add extra BASE_EXPORT to make win happy, mah")
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
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...)
The CQ bit was checked by primiano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ssid@chromium.org, hjd@chromium.org Link to the patchset: https://codereview.chromium.org/2799023002/#ps120001 (title: "rebase bind -> bindonce")
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": 120001, "attempt_start_ts": 1491992345139480,
"parent_rev": "fd0e8caa5cfe6316a3de3d181d7feb448359f125", "commit_rev":
"57cee813cd7d1c400275a5cb8e95612a4fae0fd4"}
Message was sent while issue was closed.
Description was changed from ========== memory-infra: Switch to MemoryPeakDetector and simplify MemoryDumpScheduler This CL concludes the refactoring of the old monolithic MemoryDumpScheduler in the new MemoryPeakDetector (for peaks) + MemoryDumpScheduler (for periodic dumps only). This refactoring was mostly done to clean up the entanglement of threading dependencies between MemoryDumpManager and the scheduler. Note for perf sheriffs: ---------------------- If memory metrics are skewed, or system_health memory benchmarks start failing this is very likely the culprit. Proceed with revert in this case. BUG=607533 ========== to ========== memory-infra: Switch to MemoryPeakDetector and simplify MemoryDumpScheduler This CL concludes the refactoring of the old monolithic MemoryDumpScheduler in the new MemoryPeakDetector (for peaks) + MemoryDumpScheduler (for periodic dumps only). This refactoring was mostly done to clean up the entanglement of threading dependencies between MemoryDumpManager and the scheduler. Note for perf sheriffs: ---------------------- If memory metrics are skewed, or system_health memory benchmarks start failing this is very likely the culprit. Proceed with revert in this case. BUG=607533 Review-Url: https://codereview.chromium.org/2799023002 Cr-Commit-Position: refs/heads/master@{#463991} Committed: https://chromium.googlesource.com/chromium/src/+/57cee813cd7d1c400275a5cb8e95... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/chromium/src/+/57cee813cd7d1c400275a5cb8e95... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
