Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2737153002:
[memory-infra] Implement peak detection logic (Closed)
|
Created:
3 years, 9 months ago by ssid Modified:
3 years, 9 months ago Reviewers:
Primiano Tucci (use gerrit) CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org, vmpstr+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[memory-infra] Implement peak detection logic
The CL implements peak detection in memory dump scheduler.
More discussion at https://goo.gl/0kOU4A.
BUG=607533
Review-Url: https://codereview.chromium.org/2737153002
Cr-Commit-Position: refs/heads/master@{#456163}
Committed: https://chromium.googlesource.com/chromium/src/+/203654f1cb443b997c58dfda0dbb8e11d7222f97
Patch Set 1 : fixes. #
Total comments: 17
Patch Set 2 : nit. #Patch Set 3 : Fixes. #Patch Set 4 : nit. #
Total comments: 8
Patch Set 5 : Fixes. #
Dependent Patchsets: Messages
Total messages: 25 (16 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
ssid@chromium.org changed reviewers: + primiano@chromium.org
ptal thanks
https://codereview.chromium.org/2737153002/diff/60001/base/trace_event/memory... File base/trace_event/memory_dump_scheduler.cc (right): https://codereview.chromium.org/2737153002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_scheduler.cc:32: #if defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_LINUX) || \ isn't easier to say which one you don't want? Is this just a way to say !NACL, in which case just #if !defined(OS_NACL) :) https://codereview.chromium.org/2737153002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_scheduler.cc:219: double mean = 0; never like too much to use floating point arith if we really don't need to. Can you just use uint64_t for this? https://codereview.chromium.org/2737153002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_scheduler.cc:222: mean = mean / kNumTotalsTrackedForPeakDetection; hmm this is not true until you get the initial kNum samples. I think you want to keep a count in the loop for the non-0 alues and divide by that (see my other comment about getting rid of th deque) https://codereview.chromium.org/2737153002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_scheduler.cc:226: stddev = sqrt(stddev / kNumTotalsTrackedForPeakDetection); same here, would be great if instead of the actual stddev you kept the ^2 uint64_t value and adapt the logic to reason on the squared value (unless makes anything too crazy). On android I am not even sure we target the hardfp ABI, which means that we might end up using some software fallback of this. https://codereview.chromium.org/2737153002/diff/60001/base/trace_event/memory... File base/trace_event/memory_dump_scheduler.h (right): https://codereview.chromium.org/2737153002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_scheduler.h:91: std::deque<uint64_t> last_n_memory_totals; I'd drop the "_n" in the name https://codereview.chromium.org/2737153002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_scheduler.h:91: std::deque<uint64_t> last_n_memory_totals; also deque is super silly memory-wise, as dskiba found out, in some platforms allocated 4k pages, which is a waste here. I think a better approach here would be: - Have a flat array uint32_t last_memory_samples[kNumTotalsTrackedForPeakDetection]; - Have a uint32_t last_memory_samples_index; When you write them in the buffer just do: last_memory_samples[index] = GetMemory() index = (index + 1) % kNumTotals When you compute the average/stddev just exclude the values with 0, because you know that 0 is never a valid value. https://codereview.chromium.org/2737153002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_scheduler.h:123: size_t total_physical_memory_; I find this a bit of a misleading name, because I can't tell whether this is the "total amount of system memory" or "amount of system memory used" or "total memory used by chrome". I think the 1st, but would be great to clarify.
Patchset #3 (id:100001) has been deleted
thanks, made changes, ptal https://codereview.chromium.org/2737153002/diff/60001/base/trace_event/memory... File base/trace_event/memory_dump_scheduler.cc (right): https://codereview.chromium.org/2737153002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_scheduler.cc:32: #if defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_LINUX) || \ On 2017/03/09 18:31:27, Primiano Tucci (slow - perf) wrote: > isn't easier to say which one you don't want? > Is this just a way to say !NACL, in which case just #if !defined(OS_NACL) :) GetSystemMemoryInfo is defined under this same macro. So, using the same. I might break tracing for BSD or some os. https://codereview.chromium.org/2737153002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_scheduler.cc:219: double mean = 0; On 2017/03/09 18:31:27, Primiano Tucci (slow - perf) wrote: > never like too much to use floating point arith if we really don't need to. > Can you just use uint64_t for this? Done. I liked double because i didn't have to worry about overflow. https://codereview.chromium.org/2737153002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_scheduler.cc:222: mean = mean / kNumTotalsTrackedForPeakDetection; On 2017/03/09 18:31:27, Primiano Tucci (slow - perf) wrote: > hmm this is not true until you get the initial kNum samples. > I think you want to keep a count in the loop for the non-0 alues and divide by > that (see my other comment about getting rid of th deque) If I don't have N samples then the func returns false in the start. So, only detect peak when we have enough samples. added a comment now. https://codereview.chromium.org/2737153002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_scheduler.cc:226: stddev = sqrt(stddev / kNumTotalsTrackedForPeakDetection); On 2017/03/09 18:31:27, Primiano Tucci (slow - perf) wrote: > same here, would be great if instead of the actual stddev you kept the ^2 > uint64_t value and adapt the logic to reason on the squared value (unless makes > anything too crazy). > On android I am not even sure we target the hardfp ABI, which means that we > might end up using some software fallback of this. Done. https://codereview.chromium.org/2737153002/diff/60001/base/trace_event/memory... File base/trace_event/memory_dump_scheduler.h (right): https://codereview.chromium.org/2737153002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_scheduler.h:91: std::deque<uint64_t> last_n_memory_totals; On 2017/03/09 18:31:27, Primiano Tucci (slow - perf) wrote: > I'd drop the "_n" in the name Done. https://codereview.chromium.org/2737153002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_scheduler.h:91: std::deque<uint64_t> last_n_memory_totals; On 2017/03/09 18:31:27, Primiano Tucci (slow - perf) wrote: > also deque is super silly memory-wise, as dskiba found out, in some platforms > allocated 4k pages, which is a waste here. > I think a better approach here would be: > - Have a flat array uint32_t > last_memory_samples[kNumTotalsTrackedForPeakDetection]; > - Have a uint32_t last_memory_samples_index; > > When you write them in the buffer just do: > last_memory_samples[index] = GetMemory() > index = (index + 1) % kNumTotals > > When you compute the average/stddev just exclude the values with 0, because you > know that 0 is never a valid value. I didn't want to re-implement deque here and code looks complicated. It will save few hundred bytes i guess. https://codereview.chromium.org/2737153002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_scheduler.h:123: size_t total_physical_memory_; On 2017/03/09 18:31:27, Primiano Tucci (slow - perf) wrote: > I find this a bit of a misleading name, because I can't tell whether this is the > "total amount of system memory" or "amount of system memory used" or "total > memory used by chrome". I think the 1st, but would be great to clarify. sorry, mistake. Removed this field.
https://codereview.chromium.org/2737153002/diff/60001/base/trace_event/memory... File base/trace_event/memory_dump_scheduler.cc (right): https://codereview.chromium.org/2737153002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_scheduler.cc:219: double mean = 0; On 2017/03/09 19:32:44, ssid wrote: > On 2017/03/09 18:31:27, Primiano Tucci (slow - perf) wrote: > > never like too much to use floating point arith if we really don't need to. > > Can you just use uint64_t for this? > > Done. I liked double because i didn't have to worry about overflow. ah are definitely right. I didn't realize that you have anyways the 3.69 down there. sorry for wasting your time :/ Anything you want to do works, if you want to go back to the fp math, you are completely right, there are less things to worry about. I guess I just didn't look well enough to the code before adding that comment. https://codereview.chromium.org/2737153002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_scheduler.cc:222: mean = mean / kNumTotalsTrackedForPeakDetection; On 2017/03/09 19:32:44, ssid wrote: > On 2017/03/09 18:31:27, Primiano Tucci (slow - perf) wrote: > > hmm this is not true until you get the initial kNum samples. > > I think you want to keep a count in the loop for the non-0 alues and divide by > > that (see my other comment about getting rid of th deque) > > If I don't have N samples then the func returns false in the start. So, only > detect peak when we have enough samples. added a comment now. ah righ. ack https://codereview.chromium.org/2737153002/diff/180001/base/trace_event/memor... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/2737153002/diff/180001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:773: const int kPollsToQuit = 60; out of curiosity, why? isn't this going to make the test a bit too long? https://codereview.chromium.org/2737153002/diff/180001/base/trace_event/memor... File base/trace_event/memory_dump_scheduler.cc (right): https://codereview.chromium.org/2737153002/diff/180001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.cc:28: #if defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_LINUX) || \ if it's not a problem can you move this into NotifyPollingSupported() behind a if(!memory_increase_threshold) ? GetSystemMemoryinfo might involve some syscall and IIRC the MDS can be constructed during early startup. Since we don't really need (I think) the threshold until later, would be nice to avoid to add a syscall to the browser startup if we don't really need it. https://codereview.chromium.org/2737153002/diff/180001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.cc:205: i < PollingTriggerState::kNumTotalsTrackedForPeakDetection; ++i) { maybe if you call this kNumPeakSamples those if-s will become shorter. (I guess this is my naming paranoia that is slowly inducing you into pick longer name, sorry, I always complain :P) https://codereview.chromium.org/2737153002/diff/180001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.cc:226: current_memory_total_kb; you are storing this anyways, in this if and below. Shouldn't you just move this up before the for loop?
and I meant... lgtm
Patchset #5 (id:160001) has been deleted
Patchset #4 (id:140001) has been deleted
Patchset #5 (id:200001) has been deleted
Thanks, made changes. https://codereview.chromium.org/2737153002/diff/180001/base/trace_event/memor... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/2737153002/diff/180001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:773: const int kPollsToQuit = 60; On 2017/03/10 10:19:32, Primiano Tucci (slow - perf) wrote: > out of curiosity, why? isn't this going to make the test a bit too long? Thought more about it and it does not help the testing here. I am writing some more unittests to cover cases better. https://codereview.chromium.org/2737153002/diff/180001/base/trace_event/memor... File base/trace_event/memory_dump_scheduler.cc (right): https://codereview.chromium.org/2737153002/diff/180001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.cc:28: #if defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_LINUX) || \ On 2017/03/10 10:19:33, Primiano Tucci (slow - perf) wrote: > if it's not a problem can you move this into NotifyPollingSupported() behind a > if(!memory_increase_threshold) ? > GetSystemMemoryinfo might involve some syscall and IIRC the MDS can be > constructed during early startup. > Since we don't really need (I think) the threshold until later, would be nice to > avoid to add a syscall to the browser startup if we don't really need it. Agreed. https://codereview.chromium.org/2737153002/diff/180001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.cc:205: i < PollingTriggerState::kNumTotalsTrackedForPeakDetection; ++i) { On 2017/03/10 10:19:34, Primiano Tucci (slow - perf) wrote: > maybe if you call this kNumPeakSamples those if-s will become shorter. > (I guess this is my naming paranoia that is slowly inducing you into pick longer > name, sorry, I always complain :P) Done. https://codereview.chromium.org/2737153002/diff/180001/base/trace_event/memor... base/trace_event/memory_dump_scheduler.cc:226: current_memory_total_kb; On 2017/03/10 10:19:33, Primiano Tucci (slow - perf) wrote: > you are storing this anyways, in this if and below. > Shouldn't you just move this up before the for loop? I am trying to compute the mean and stddev without the current sample. So, doing it before inserting new element. Do not want to complicate code with N+1 array and not include "index" element. Also, early return looked better here.
https://codereview.chromium.org/2737153002/diff/60001/base/trace_event/memory... File base/trace_event/memory_dump_scheduler.cc (right): https://codereview.chromium.org/2737153002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_scheduler.cc:219: double mean = 0; On 2017/03/10 10:19:32, Primiano Tucci (slow - perf) wrote: > On 2017/03/09 19:32:44, ssid wrote: > > On 2017/03/09 18:31:27, Primiano Tucci (slow - perf) wrote: > > > never like too much to use floating point arith if we really don't need to. > > > Can you just use uint64_t for this? > > > > Done. I liked double because i didn't have to worry about overflow. > > ah are definitely right. I didn't realize that you have anyways the 3.69 down > there. sorry for wasting your time :/ > Anything you want to do works, if you want to go back to the fp math, you are > completely right, there are less things to worry about. I guess I just didn't > look well enough to the code before adding that comment. Weel now that i accounted for the overflow, it should be better than sqrt calls. I guess i will just use the variance calculations.
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: 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 Link to the patchset: https://codereview.chromium.org/2737153002/#ps220001 (title: "Fixes.")
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": 220001, "attempt_start_ts": 1489180102555310,
"parent_rev": "d9ab107ef6724e5fe194e6b7402086d41c187063", "commit_rev":
"203654f1cb443b997c58dfda0dbb8e11d7222f97"}
Message was sent while issue was closed.
Description was changed from ========== [memory-infra] Implement peak detection logic The CL implements peak detection in memory dump scheduler. More discussion at https://goo.gl/0kOU4A. BUG=607533 ========== to ========== [memory-infra] Implement peak detection logic The CL implements peak detection in memory dump scheduler. More discussion at https://goo.gl/0kOU4A. BUG=607533 Review-Url: https://codereview.chromium.org/2737153002 Cr-Commit-Position: refs/heads/master@{#456163} Committed: https://chromium.googlesource.com/chromium/src/+/203654f1cb443b997c58dfda0dbb... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:220001) as https://chromium.googlesource.com/chromium/src/+/203654f1cb443b997c58dfda0dbb... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
