Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2793023002:
memory-infra: port peak detection logic to MemoryPeakDetector (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, vmpstr+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionmemory-infra: port peak detection logic to MemoryPeakDetector
This CL ports the memory peak detection of MemoryDumpScheduler
into the new, self-contained, MemoryPeakDetector.
This CL has no functional effects, since the new peak detector is not
used yet.
BUG=607533
Review-Url: https://codereview.chromium.org/2793023002
Cr-Commit-Position: refs/heads/master@{#463642}
Committed: https://chromium.googlesource.com/chromium/src/+/3821fa74fa559fbaabda7c9ca82ffa244c2e45a7
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : fix lambdas for MSVC #Patch Set 5 : fix lambdas for MSVC #
Total comments: 29
Patch Set 6 : NaCL, as usual #
Total comments: 4
Patch Set 7 : ssid review #Patch Set 8 : rebase, no changes #
Total comments: 4
Patch Set 9 : ssid comments, clear -> throttle #Patch Set 10 : fix uint64_t cast #
Dependent Patchsets: Messages
Total messages: 65 (50 generated)
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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by primiano@chromium.org to run a CQ dry run
Description was changed from ========== memory-infra: port peak detection logic WIP BUG=607533 ========== to ========== memory-infra: port peak detection logic to MemoryPeakDetector WIP BUG=607533 ==========
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: port peak detection logic to MemoryPeakDetector WIP BUG=607533 ========== to ========== memory-infra: port peak detection logic to MemoryPeakDetector This CL ports the memory peak detection of MemoryDumpScheduler into the new, self-contained, MemoryPeakDetector. This CL has no functional effects, since the new peak detector is not used yet. BUG=607533 ==========
primiano@chromium.org changed reviewers: + hjd@chromium.org, ssid@chromium.org
Not fully finished (see TODOs left around) but could you folks have an initial pass?
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_...)
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...
Patchset #5 (id:80001) has been deleted
Patchset #5 (id:100001) has been deleted
Patchset #4 (id:60001) 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...
Ready for review. if lgty will ditch the peak code from memorydumpscheduler and update MDM in the next cl
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_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 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: This issue passed the CQ dry run.
https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... File base/trace_event/memory_peak_detector.cc (right): https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... base/trace_event/memory_peak_detector.cc:100: state_ = ENABLED; DCHECK_GT(polling_interval_ms, 0) DCHECK_GT(min_time_between_peaks_ms, 0) https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... base/trace_event/memory_peak_detector.cc:172: polled_mem_bytes / 1024); MB is easier to read in TV. https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... base/trace_event/memory_peak_detector.cc:185: is_peak = static_threshold_bytes_ && why this condition after dcheck above? should we not trust SysInfo::AmountOfPhysicalMemory()? If so, we could have a default value when this is assigned. https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... base/trace_event/memory_peak_detector.cc:194: "Peak memory dump detected", TRACE_EVENT_SCOPE_PROCESS, "Peak memory detected"? Also can we keep this in MB? It is just hard to read the values in the trace in Kb. 2.4GB will show up as 2516582. https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... base/trace_event/memory_peak_detector.cc:220: uint64_t variance = 0; Either make this double or make the sample kb. A sample of 1GB change will cause 2^30 * 2^30 variance. If we have 4 such ups and downs over the last 50 samples, then we can easily hit 2^64. Back to the previous discussion, maybe double is just better to avoid such confusion. https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... base/trace_event/memory_peak_detector.cc:239: last_dump_memory_total_ = 0; hm shouldn't this be set to the current value instead? If browser triggers a dump and renderer is polling, it should be notified that the dump was triggered at current value instead of 0. Also, if we sometime get to a point where we use peak and periodic dumps together, then it is better to track when the last dump was triggered. So, the existing code sets this to previous sample if it exists. https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... base/trace_event/memory_peak_detector.cc:242: if (config_.polling_interval_ms > 0) { Maybe we should check for state_ == running or enabled. Since the comment on config_ says valid only if state_ is this. Also if enabled/running, last_dump_memory_total = samples_[sample_index]; https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... File base/trace_event/memory_peak_detector.h (right): https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... base/trace_event/memory_peak_detector.h:53: bool enable_verbose_poll_tracing; Nice to have some comment on the config members. https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... base/trace_event/memory_peak_detector.h:96: void Clear(); MemoryPeakDetector::Clear() sounds like this would clear the detector after stopping. If this is the only use case why not NotifyDumpTriggered? Also it is useful to set the last total at the notify instead of just resetting results. Also, you'd not have to write such elaborate comment about it if it's just notify they are impl details. https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... base/trace_event/memory_peak_detector.h:163: uint64_t samples_bytes_[kSlidingWindowNumSamples]; Samples were stored in kb because of the overflows in stddev calculation.
https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... File base/trace_event/memory_peak_detector.cc (right): https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... base/trace_event/memory_peak_detector.cc:102: ResetPollHistory(); Should we test if Start() Stop() Start() happened, then the history was cleared between Stop and Start?
https://codereview.chromium.org/2793023002/diff/160001/base/trace_event/memor... File base/trace_event/memory_peak_detector.cc (right): https://codereview.chromium.org/2793023002/diff/160001/base/trace_event/memor... base/trace_event/memory_peak_detector.cc:53: #if !defined(OS_NACL) The previous check also excluded iOS I don't know if that is important: https://cs.chromium.org/chromium/src/base/trace_event/memory_dump_scheduler.c... https://codereview.chromium.org/2793023002/diff/160001/base/trace_event/memor... base/trace_event/memory_peak_detector.cc:218: if (samples_bytes_[i] == 0) We won't be able to detect a spike from 0 with this code which I guess is probably fine.
The CQ bit was checked by primiano@chromium.org to run a CQ dry run
https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... File base/trace_event/memory_peak_detector.cc (right): https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... base/trace_event/memory_peak_detector.cc:100: state_ = ENABLED; On 2017/04/05 21:23:46, ssid wrote: > DCHECK_GT(polling_interval_ms, 0) This one is already done in ::Start() I added an extra one for safety in PollMemoryAndDetectPeak, just before the PostDelayedTask > DCHECK_GT(min_time_between_peaks_ms, 0) Nope, this one can be zero (I use it in tests). There is no reason why one shouldn't be able to say "no throttling" https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... base/trace_event/memory_peak_detector.cc:102: ResetPollHistory(); On 2017/04/05 21:26:46, ssid wrote: > Should we test if Start() Stop() Start() happened, then the history was cleared > between Stop and Start? Done. See RestartClearsState test https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... base/trace_event/memory_peak_detector.cc:172: polled_mem_bytes / 1024); On 2017/04/05 21:23:46, ssid wrote: > MB is easier to read in TV. Ahh I see, good point. done https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... base/trace_event/memory_peak_detector.cc:185: is_peak = static_threshold_bytes_ && On 2017/04/05 21:23:46, ssid wrote: > why this condition after dcheck above? should we not trust > SysInfo::AmountOfPhysicalMemory()? If so, we could have a default value when > this is assigned. I expect AmountOfPhysicalMemory can be 0 in tests (asan bots are known to cheat on this) but not in actual production code. If that happens I think the safest thing is just ignorning the static threshold. https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... base/trace_event/memory_peak_detector.cc:194: "Peak memory dump detected", TRACE_EVENT_SCOPE_PROCESS, On 2017/04/05 21:23:46, ssid wrote: > "Peak memory detected"? > Also can we keep this in MB? It is just hard to read the values in the trace in > Kb. 2.4GB will show up as 2516582. Done. https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... base/trace_event/memory_peak_detector.cc:220: uint64_t variance = 0; On 2017/04/05 21:23:46, ssid wrote: > Either make this double or make the sample kb. > A sample of 1GB change will cause > 2^30 * 2^30 variance. > > If we have 4 such ups and downs over the last 50 samples, then we can easily hit > 2^64. Oh excellent point. > Back to the previous discussion, maybe double is just better to avoid such > confusion. Yeah agreed. My obsession for keeping integer arith was becoming too painful here. switched to floats. https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... base/trace_event/memory_peak_detector.cc:239: last_dump_memory_total_ = 0; On 2017/04/05 21:23:46, ssid wrote: > hm shouldn't this be set to the current value instead? > If browser triggers a dump and renderer is polling, it should be notified that > the dump was triggered at current value instead of 0. Does it make a difference in practice? If I set this to 0, the next poll will take the polled value as initial. So we really lose the increase beween the reset and the next poll, which is like 1 ms. Or is there asomething more I am not thinking about? > Also, if we sometime get to a point where we use peak and periodic dumps > together, then it is better to track when the last dump was triggered. > So, the existing code sets this to previous sample if it exists. Not sure I understand this. Are you suggesting that Clear() sholuld take an argument? THat feels a bit of a weird api. Also, as above, this makes a difference only for one sample ? Or am I missing something? https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... base/trace_event/memory_peak_detector.cc:242: if (config_.polling_interval_ms > 0) { On 2017/04/05 21:23:46, ssid wrote: > Maybe we should check for state_ == running or enabled. Since the comment on > config_ says valid only if state_ is this. No I use this also to clearup during Setup(). > Also if enabled/running, > last_dump_memory_total = samples_[sample_index]; Does it make a difference in practice? other than getting an extra sample for the next turn? If I do this, then it becomes awkward because I have to clearup separately samples_ at Setup (while here I am using this also for clear at Setup) https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... File base/trace_event/memory_peak_detector.h (right): https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... base/trace_event/memory_peak_detector.h:53: bool enable_verbose_poll_tracing; On 2017/04/05 21:23:47, ssid wrote: > Nice to have some comment on the config members. done https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... base/trace_event/memory_peak_detector.h:96: void Clear(); On 2017/04/05 21:23:47, ssid wrote: > MemoryPeakDetector::Clear() sounds like this would clear the detector after > stopping. But the comment says "If start()-ed, otherwise it's a noop". Isn't that clear enough? > If this is the only use case why not NotifyDumpTriggered? > Also it is useful to set the last total at the notify instead of just resetting > results. Hm not sure I get this part. Can you expand? I guess I am missing some part of the logic? > Also, you'd not have to write such elaborate comment about it if it's just > notify they are impl details. NotifyDumpTriggered is kind of weird to see here. First of all this class doesn't know anything anymore about dumps. This just gives a callback when a peak is detected. If I say "hey peak detector, I notify you I just created a memory dump" my reaction is "so what?". https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... base/trace_event/memory_peak_detector.h:163: uint64_t samples_bytes_[kSlidingWindowNumSamples]; On 2017/04/05 21:23:47, ssid wrote: > Samples were stored in kb because of the overflows in stddev calculation. I moved the stddev to floats, should be fine now. https://codereview.chromium.org/2793023002/diff/160001/base/trace_event/memor... File base/trace_event/memory_peak_detector.cc (right): https://codereview.chromium.org/2793023002/diff/160001/base/trace_event/memor... base/trace_event/memory_peak_detector.cc:53: #if !defined(OS_NACL) On 2017/04/06 11:33:48, hjd wrote: > The previous check also excluded iOS I don't know if that is important: > https://cs.chromium.org/chromium/src/base/trace_event/memory_dump_scheduler.c... This one seems defined for ios (base/sys_info_ios.mm) https://codereview.chromium.org/2793023002/diff/160001/base/trace_event/memor... base/trace_event/memory_peak_detector.cc:218: if (samples_bytes_[i] == 0) On 2017/04/06 11:33:48, hjd wrote: > We won't be able to detect a spike from 0 with this code which I guess is > probably fine. Yeah. 0 memory usage is quite rare :)
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 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: This issue passed the CQ dry run.
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...
Ping: folks can you get a final pass on this? Unless I missed to address some comments, this one should be ready to go. The rest will happen in https://codereview.chromium.org/2799023002/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/10 12:29:10, Primiano Tucci wrote: > Ping: folks can you get a final pass on this? Unless I missed to address some > comments, this one should be ready to go. > The rest will happen in https://codereview.chromium.org/2799023002/ lgtm
lgtm % the Clear() function logic. Please look at it before landing. Thanks! https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... File base/trace_event/memory_peak_detector.cc (right): https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... base/trace_event/memory_peak_detector.cc:100: state_ = ENABLED; On 2017/04/06 20:07:10, Primiano Tucci wrote: > On 2017/04/05 21:23:46, ssid wrote: > > DCHECK_GT(polling_interval_ms, 0) > This one is already done in ::Start() > I added an extra one for safety in PollMemoryAndDetectPeak, just before the > PostDelayedTask > > > DCHECK_GT(min_time_between_peaks_ms, 0) > Nope, this one can be zero (I use it in tests). There is no reason why one > shouldn't be able to say "no throttling" > Acknowledged. https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... base/trace_event/memory_peak_detector.cc:239: last_dump_memory_total_ = 0; On 2017/04/06 20:07:10, Primiano Tucci wrote: > On 2017/04/05 21:23:46, ssid wrote: > > hm shouldn't this be set to the current value instead? > > If browser triggers a dump and renderer is polling, it should be notified that > > the dump was triggered at current value instead of 0. > > Does it make a difference in practice? If I set this to 0, the next poll will > take the polled value as initial. > So we really lose the increase beween the reset and the next poll, which is like > 1 ms. > Or is there asomething more I am not thinking about? > > > Also, if we sometime get to a point where we use peak and periodic dumps > > together, then it is better to track when the last dump was triggered. > > So, the existing code sets this to previous sample if it exists. > Not sure I understand this. Are you suggesting that Clear() sholuld take an > argument? THat feels a bit of a weird api. > Also, as above, this makes a difference only for one sample ? Or am I missing > something? The issue is: If we detect peak in browser and renderer (as happens in Android), let's say browser detected a peak at t1. The renderer process peak detector gets notified that dump was triggered at t1. Renderer last_memory_total = 0;. If we specified the min time between dumps as 2 seconds in trace config, then after 2 seconds renderer peak detector will set value of last total = current value, since we set it after skip_polls are done. What we should do here is set the value to the current total when notify is called. I suggested setting the value here since we should always remember to do this reset in the next poll. It is just clean to set it to previous value in the notify call. https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... base/trace_event/memory_peak_detector.cc:242: if (config_.polling_interval_ms > 0) { On 2017/04/06 20:07:10, Primiano Tucci wrote: > On 2017/04/05 21:23:46, ssid wrote: > > Maybe we should check for state_ == running or enabled. Since the comment on > > config_ says valid only if state_ is this. > > No I use this also to clearup during Setup(). > > > Also if enabled/running, > > last_dump_memory_total = samples_[sample_index]; > > Does it make a difference in practice? other than getting an extra sample for > the next turn? > If I do this, then it becomes awkward because I have to clearup separately > samples_ at Setup (while here I am using this also for clear at Setup) hm that is true. Maybe we could just do it in next poll. https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... File base/trace_event/memory_peak_detector.h (right): https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... base/trace_event/memory_peak_detector.h:96: void Clear(); On 2017/04/06 20:07:10, Primiano Tucci wrote: > On 2017/04/05 21:23:47, ssid wrote: > > MemoryPeakDetector::Clear() sounds like this would clear the detector after > > stopping. > But the comment says "If start()-ed, otherwise it's a noop". Isn't that clear > enough? > > > If this is the only use case why not NotifyDumpTriggered? > > Also it is useful to set the last total at the notify instead of just > resetting > > results. > Hm not sure I get this part. Can you expand? I guess I am missing some part of > the logic? > I tried to explain this in the comment below. > > Also, you'd not have to write such elaborate comment about it if it's just > > notify they are impl details. > NotifyDumpTriggered is kind of weird to see here. First of all this class > doesn't know anything anymore about dumps. This just gives a callback when a > peak is detected. If I say "hey peak detector, I notify you I just created a > memory dump" my reaction is "so what?". Okay in that case the main use case of the Clear() is to reset the skip_polls_. That is the state that prevents dumps from being created immediately. Clearing of the sliding window is required because we skip the polls and we will have an inconsistent window. Resetting the static threshold is required because a dump was triggered at the current total. Maybe this function should be named NotifyPeakDetected()? But if we did that then in future if we wanted to delay the dump triggering because of a periodic dump then it'd not make sense. So, maybe ResetTimer ? I can't think of a better name. In any case looking at the comment, my question is why should peak detector have an api to clear the sliding window(implementation detail). If this function is to stop dump from being created then that should only be in the comment. https://codereview.chromium.org/2793023002/diff/200001/base/trace_event/memor... File base/trace_event/memory_peak_detector.cc (right): https://codereview.chromium.org/2793023002/diff/200001/base/trace_event/memor... base/trace_event/memory_peak_detector.cc:58: static_threshold_bytes_ = 0; Not a big deal, but why not have a default value here instead of 0? We used to have it in the old version. Useful in case some platform does not return correct value for PhysicalMemory. https://codereview.chromium.org/2793023002/diff/200001/base/trace_event/memor... File base/trace_event/memory_peak_detector.h (right): https://codereview.chromium.org/2793023002/diff/200001/base/trace_event/memor... base/trace_event/memory_peak_detector.h:55: // Guarantees that a peak detection callback will never before than the "will never before than"?
Patchset #9 (id:220001) has been deleted
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/2793023002/#ps240001 (title: "ssid comments, clear -> throttle")
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 primiano@chromium.org
The CQ bit was checked by primiano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... File base/trace_event/memory_peak_detector.cc (right): https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... base/trace_event/memory_peak_detector.cc:239: last_dump_memory_total_ = 0; On 2017/04/10 20:55:06, ssid wrote: > On 2017/04/06 20:07:10, Primiano Tucci wrote: > > On 2017/04/05 21:23:46, ssid wrote: > > > hm shouldn't this be set to the current value instead? > > > If browser triggers a dump and renderer is polling, it should be notified > that > > > the dump was triggered at current value instead of 0. > > > > Does it make a difference in practice? If I set this to 0, the next poll will > > take the polled value as initial. > > So we really lose the increase beween the reset and the next poll, which is > like > > 1 ms. > > Or is there asomething more I am not thinking about? > > > > > Also, if we sometime get to a point where we use peak and periodic dumps > > > together, then it is better to track when the last dump was triggered. > > > So, the existing code sets this to previous sample if it exists. > > Not sure I understand this. Are you suggesting that Clear() sholuld take an > > argument? THat feels a bit of a weird api. > > Also, as above, this makes a difference only for one sample ? Or am I missing > > something? > > > The issue is: If we detect peak in browser and renderer (as happens in Android), > let's say browser detected a peak at t1. The renderer process peak detector gets > notified that dump was triggered at t1. Renderer last_memory_total = 0;. If we > specified the min time between dumps as 2 seconds in trace config, then after 2 > seconds renderer peak detector will set value of last total = current value, > since we set it after skip_polls are done. What we should do here is set the > value to the current total when notify is called. > > I suggested setting the value here since we should always remember to do this > reset in the next poll. It is just clean to set it to previous value in the > notify call. > Okay I changes this, but still not sure this makes a huge difference. The only difference would be if in the last 2 seconds the renderer had a jump > the static threshold. The stddev is unaffected in practice, because we are just talking about keeping an old sample (Which will last 1ms) in the window or not. W.r.t. the case you mentioned above, I wonder if clearing the history is really the best thing to do, or whether we should just fill the skip_polls_ and keep monitoring memory. Or maybe we should probably have one instance of peak detector per process (all of them in the browser process). Let's rediscuss this in future, there is no rush now. For the moment will do what you say as doesn't make a huge difference anyways. Anyhow, don't want to get this blocked on this decision that we can improve later. Let's get this landed and we can improve subsequently, but I can unblock the rest of the work in the meantime. https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... base/trace_event/memory_peak_detector.cc:242: if (config_.polling_interval_ms > 0) { On 2017/04/10 20:55:06, ssid wrote: > On 2017/04/06 20:07:10, Primiano Tucci wrote: > > On 2017/04/05 21:23:46, ssid wrote: > > > Maybe we should check for state_ == running or enabled. Since the comment on > > > config_ says valid only if state_ is this. > > > > No I use this also to clearup during Setup(). > > > > > Also if enabled/running, > > > last_dump_memory_total = samples_[sample_index]; > > > > Does it make a difference in practice? other than getting an extra sample for > > the next turn? > > If I do this, then it becomes awkward because I have to clearup separately > > samples_ at Setup (while here I am using this also for clear at Setup) > > hm that is true. Maybe we could just do it in next poll. I just added a bool bool keep_last_sample here, so we can either clear all or keep the last value. https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... File base/trace_event/memory_peak_detector.h (right): https://codereview.chromium.org/2793023002/diff/140001/base/trace_event/memor... base/trace_event/memory_peak_detector.h:96: void Clear(); On 2017/04/10 20:55:06, ssid wrote: > On 2017/04/06 20:07:10, Primiano Tucci wrote: > > On 2017/04/05 21:23:47, ssid wrote: > > > MemoryPeakDetector::Clear() sounds like this would clear the detector after > > > stopping. > > But the comment says "If start()-ed, otherwise it's a noop". Isn't that clear > > enough? > > > > > If this is the only use case why not NotifyDumpTriggered? > > > Also it is useful to set the last total at the notify instead of just > > resetting > > > results. > > Hm not sure I get this part. Can you expand? I guess I am missing some part of > > the logic? > > > I tried to explain this in the comment below. > > > > Also, you'd not have to write such elaborate comment about it if it's just > > > notify they are impl details. > > NotifyDumpTriggered is kind of weird to see here. First of all this class > > doesn't know anything anymore about dumps. This just gives a callback when a > > peak is detected. If I say "hey peak detector, I notify you I just created a > > memory dump" my reaction is "so what?". > > Okay in that case the main use case of the Clear() is to reset the skip_polls_. > That is the state that prevents dumps from being created immediately. Clearing > of the sliding window is required because we skip the polls and we will have an > inconsistent window. See my other comment. That suggests to me that we should keep accumulating the window in the Poll function regardless of the |skip|, and then just not do dumps. Let's discuss this in a future cl. > Resetting the static threshold is required because a dump > was triggered at the current total. Maybe this function should be named > NotifyPeakDetected()? But if we did that then in future if we wanted to delay > the dump triggering because of a periodic dump then it'd not make sense. So, > maybe ResetTimer ? I can't think of a better name. > In any case looking at the comment, my question is why should peak detector have > an api to clear the sliding window(implementation detail). If this function is > to stop dump from being created then that should only be in the comment. Okay good point. Reanemed to Throttle() and made the comment less implementation-dependent. https://codereview.chromium.org/2793023002/diff/200001/base/trace_event/memor... File base/trace_event/memory_peak_detector.cc (right): https://codereview.chromium.org/2793023002/diff/200001/base/trace_event/memor... base/trace_event/memory_peak_detector.cc:58: static_threshold_bytes_ = 0; On 2017/04/10 20:55:06, ssid wrote: > Not a big deal, but why not have a default value here instead of 0? We used to > have it in the old version. Useful in case some platform does not return correct > value for PhysicalMemory. Done https://codereview.chromium.org/2793023002/diff/200001/base/trace_event/memor... File base/trace_event/memory_peak_detector.h (right): https://codereview.chromium.org/2793023002/diff/200001/base/trace_event/memor... base/trace_event/memory_peak_detector.h:55: // Guarantees that a peak detection callback will never before than the On 2017/04/10 20:55:06, ssid wrote: > "will never before than"? reworded
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_tsan_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 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/2793023002/#ps260001 (title: "fix uint64_t cast")
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": 260001, "attempt_start_ts": 1491921990613510,
"parent_rev": "ffc3b8b65470e1a2fe1e27e7f3e75227a46ddddf", "commit_rev":
"3821fa74fa559fbaabda7c9ca82ffa244c2e45a7"}
Message was sent while issue was closed.
Description was changed from ========== memory-infra: port peak detection logic to MemoryPeakDetector This CL ports the memory peak detection of MemoryDumpScheduler into the new, self-contained, MemoryPeakDetector. This CL has no functional effects, since the new peak detector is not used yet. BUG=607533 ========== to ========== memory-infra: port peak detection logic to MemoryPeakDetector This CL ports the memory peak detection of MemoryDumpScheduler into the new, self-contained, MemoryPeakDetector. This CL has no functional effects, since the new peak detector is not used yet. BUG=607533 Review-Url: https://codereview.chromium.org/2793023002 Cr-Commit-Position: refs/heads/master@{#463642} Committed: https://chromium.googlesource.com/chromium/src/+/3821fa74fa559fbaabda7c9ca82f... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:260001) as https://chromium.googlesource.com/chromium/src/+/3821fa74fa559fbaabda7c9ca82f... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
