|
|
Descriptionmemory coordinator: Purge memory under memory pressure
- On WARNING condition, request a backgrounded child process to purge memory
- On CRITICAL condition, try to discard a tab first. If it failed, try to
purge memory from all child processes. It it failed, try to purge memory
from the browser process
- Use somewhat conservative heuristics for purging
- Browser: suppress purging for 2 mins after a purge
- Renderer: after purging, suppress the next purging until the renderer
goes foreground -> background then remains backgrounded for 30 secs
BUG=696844
Review-Url: https://codereview.chromium.org/2763933002
Cr-Commit-Position: refs/heads/master@{#460622}
Committed: https://chromium.googlesource.com/chromium/src/+/b9058f285cf483b7c49fe46e554f55f54ffeb956
Patch Set 1 #Patch Set 2 : update #Patch Set 3 : tweak #Patch Set 4 : Tweak #Patch Set 5 : Use int64_t #
Total comments: 26
Patch Set 6 : comments #Patch Set 7 : discard tab #
Total comments: 5
Patch Set 8 : comments #
Messages
Total messages: 44 (28 generated)
Description was changed from ========== WIP: memory coordinator: Purge memory under memory pressure - On WARNING condition, request a backgrounded child process to purge memory - On CRITICAL condition, try to discard a tab first. If it failed, try to purge memory from all child processes. It it failed, try to purge memory from the browser process BUG=696844 ========== to ========== WIP: memory coordinator: Purge memory under memory pressure - On WARNING condition, request a backgrounded child process to purge memory - On CRITICAL condition, try to discard a tab first. If it failed, try to purge memory from all child processes. It it failed, try to purge memory from the browser process - Use somewhat conservative heuristics for purging - Browser: suppress purging for 2 mins after a purge - Renderer: after purging, suppress the next purging until the renderer goes foreround -> background then remains backgrounded for 30 secs BUG=696844 ==========
Description was changed from ========== WIP: memory coordinator: Purge memory under memory pressure - On WARNING condition, request a backgrounded child process to purge memory - On CRITICAL condition, try to discard a tab first. If it failed, try to purge memory from all child processes. It it failed, try to purge memory from the browser process - Use somewhat conservative heuristics for purging - Browser: suppress purging for 2 mins after a purge - Renderer: after purging, suppress the next purging until the renderer goes foreround -> background then remains backgrounded for 30 secs BUG=696844 ========== to ========== WIP: memory coordinator: Purge memory under memory pressure - On WARNING condition, request a backgrounded child process to purge memory - On CRITICAL condition, try to discard a tab first. If it failed, try to purge memory from all child processes. It it failed, try to purge memory from the browser process - Use somewhat conservative heuristics for purging - Browser: suppress purging for 2 mins after a purge - Renderer: after purging, suppress the next purging until the renderer goes foreground -> background then remains backgrounded for 30 secs BUG=696844 ==========
bashi@chromium.org changed reviewers: + chrisha@chromium.org, haraken@chromium.org
I've been doing local experiments on my ChromeOS and Android devices to figure out a simple but workable strategy to schedule purging memory and I came around to implementing this CL. What do you think about this approach? If this looks okay I'll add tests.
On 2017/03/23 02:58:06, bashi (slow til Mar 23) wrote: > I've been doing local experiments on my ChromeOS and Android devices to figure > out a simple but workable strategy to schedule purging memory and I came around > to implementing this CL. What do you think about this approach? If this looks > okay I'll add tests. The approach makes sense and +1 to starting with the heuristics.
Description was changed from ========== WIP: memory coordinator: Purge memory under memory pressure - On WARNING condition, request a backgrounded child process to purge memory - On CRITICAL condition, try to discard a tab first. If it failed, try to purge memory from all child processes. It it failed, try to purge memory from the browser process - Use somewhat conservative heuristics for purging - Browser: suppress purging for 2 mins after a purge - Renderer: after purging, suppress the next purging until the renderer goes foreground -> background then remains backgrounded for 30 secs BUG=696844 ========== to ========== memory coordinator: Purge memory under memory pressure - On WARNING condition, request a backgrounded child process to purge memory - On CRITICAL condition, try to discard a tab first. If it failed, try to purge memory from all child processes. It it failed, try to purge memory from the browser process - Use somewhat conservative heuristics for purging - Browser: suppress purging for 2 mins after a purge - Renderer: after purging, suppress the next purging until the renderer goes foreground -> background then remains backgrounded for 30 secs BUG=696844 ==========
The CQ bit was checked by bashi@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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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 bashi@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...
Added tests. Could you take another look?
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_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Mostly looks good. https://codereview.chromium.org/2763933002/diff/80001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/2763933002/diff/80001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.cc:369: return false; Does this mean that the tab discarding not working on Chrome OS? https://codereview.chromium.org/2763933002/diff/80001/content/browser/memory/... File content/browser/memory/memory_coordinator_impl.cc (right): https://codereview.chromium.org/2763933002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:85: if (bytes < 0) Maybe we want to record this case too. See my comment in render_thread_impl. https://codereview.chromium.org/2763933002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:304: if (next_condition == MemoryCondition::CRITICAL) else if https://codereview.chromium.org/2763933002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:415: tick_clock_->NowTicks() + background_child_purge_candidate_period_; Maybe you can inline base::TimeDelta::FromSeconds(kDefaultBackgroundChildPurgeCandidatePeriodSeconds) and remove background_child_purge_candidate_period_. https://codereview.chromium.org/2763933002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:499: if (TryPurgeMemoryFromChildren(PurgeTarget::ALL)) Add: // Prefer purging memory from child processes than the browser process because ... https://codereview.chromium.org/2763933002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:511: if (!iter.second.can_purge_after.is_null() && I'd prefer initializing can_purge_after to base::TimeTicks::Max() and dropping the null check. https://codereview.chromium.org/2763933002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:527: if (!can_purge_after_.is_null() && can_purge_after_ > now) Remove the null check. https://codereview.chromium.org/2763933002/diff/80001/content/browser/memory/... File content/browser/memory/memory_coordinator_impl.h (right): https://codereview.chromium.org/2763933002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.h:208: // Called regularly while the memory condition is WARNING. periodically https://codereview.chromium.org/2763933002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.h:211: // Called regularly while the memory condition is CRITICAL. periodically https://codereview.chromium.org/2763933002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.h:221: bool TryPurgeMemoryFromChildren(PurgeTarget target); TryTo https://codereview.chromium.org/2763933002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.h:224: bool TryPurgeMemoryFromBrowser(); TryTo https://codereview.chromium.org/2763933002/diff/80001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2763933002/diff/80001/content/renderer/render... content/renderer/render_thread_impl.cc:2238: if (mbytes < 0) Maybe we want to count this case in some way too? e.g., treating as mbytes == 0. https://codereview.chromium.org/2763933002/diff/80001/content/renderer/render... content/renderer/render_thread_impl.cc:2251: base::TimeDelta::FromSeconds(2)); I guess tasak@ was using 15 seconds or something. I'm not sure what the right number would be :)
Regarding the question about tab discarding, I'll address in the next patch set. Maybe I need to take different approach in OnCriticalCondition() https://codereview.chromium.org/2763933002/diff/80001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/2763933002/diff/80001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.cc:369: return false; On 2017/03/24 11:55:38, haraken wrote: > > Does this mean that the tab discarding not working on Chrome OS? Sorry I didn't check the implementation of LowMemoryKill(). It seems that it may synchronously/asynchronously discard a tab or kill a process. At this point we can't check if it does something (tab discarding / killing process). Probably we should take another approach and I'll try to figure it out. https://codereview.chromium.org/2763933002/diff/80001/content/browser/memory/... File content/browser/memory/memory_coordinator_impl.cc (right): https://codereview.chromium.org/2763933002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:85: if (bytes < 0) On 2017/03/24 11:55:39, haraken wrote: > > Maybe we want to record this case too. See my comment in render_thread_impl. Acknowledged. https://codereview.chromium.org/2763933002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:304: if (next_condition == MemoryCondition::CRITICAL) On 2017/03/24 11:55:39, haraken wrote: > > else if Done. https://codereview.chromium.org/2763933002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:415: tick_clock_->NowTicks() + background_child_purge_candidate_period_; On 2017/03/24 11:55:39, haraken wrote: > > Maybe you can inline > base::TimeDelta::FromSeconds(kDefaultBackgroundChildPurgeCandidatePeriodSeconds) > and remove background_child_purge_candidate_period_. Done. https://codereview.chromium.org/2763933002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:499: if (TryPurgeMemoryFromChildren(PurgeTarget::ALL)) On 2017/03/24 11:55:39, haraken wrote: > > Add: > > // Prefer purging memory from child processes than the browser process because > ... Done. https://codereview.chromium.org/2763933002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:511: if (!iter.second.can_purge_after.is_null() && On 2017/03/24 11:55:38, haraken wrote: > > I'd prefer initializing can_purge_after to base::TimeTicks::Max() and dropping > the null check. Using Max() as the initial value is slightly different from using null value as the initial value. If we use Max(), a tab won't become a candidate until it goes background. I'd prefer to make foreground tabs be candidates of purging memory in CRITICAL condition. https://codereview.chromium.org/2763933002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:527: if (!can_purge_after_.is_null() && can_purge_after_ > now) On 2017/03/24 11:55:38, haraken wrote: > > Remove the null check. Done. https://codereview.chromium.org/2763933002/diff/80001/content/browser/memory/... File content/browser/memory/memory_coordinator_impl.h (right): https://codereview.chromium.org/2763933002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.h:208: // Called regularly while the memory condition is WARNING. On 2017/03/24 11:55:39, haraken wrote: > > periodically Done. https://codereview.chromium.org/2763933002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.h:211: // Called regularly while the memory condition is CRITICAL. On 2017/03/24 11:55:39, haraken wrote: > > periodically Done. https://codereview.chromium.org/2763933002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.h:221: bool TryPurgeMemoryFromChildren(PurgeTarget target); On 2017/03/24 11:55:39, haraken wrote: > > TryTo Done. https://codereview.chromium.org/2763933002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.h:224: bool TryPurgeMemoryFromBrowser(); On 2017/03/24 11:55:39, haraken wrote: > > TryTo Done. https://codereview.chromium.org/2763933002/diff/80001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2763933002/diff/80001/content/renderer/render... content/renderer/render_thread_impl.cc:2238: if (mbytes < 0) On 2017/03/24 11:55:39, haraken wrote: > > Maybe we want to count this case in some way too? e.g., treating as mbytes == 0. Done. https://codereview.chromium.org/2763933002/diff/80001/content/renderer/render... content/renderer/render_thread_impl.cc:2251: base::TimeDelta::FromSeconds(2)); On 2017/03/24 11:55:39, haraken wrote: > > I guess tasak@ was using 15 seconds or something. I'm not sure what the right > number would be :) I think 2 seconds is enough and 15 sec seems too long for my case as the process may be foregrounded.
The CQ bit was checked by bashi@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...
PTAL? As chatted offline I changed the CL to call DiscardTab() every time OnCriticalCondition() is called. This may be a little aggressive but given the feedback I received, we should be a little aggressive here. Also I changed the monitoring interval from 5 secs to 1 sec to align the current interval of base::chromeos::MemoryPressureMonitor. https://codereview.chromium.org/2763933002/diff/120001/content/browser/memory... File content/browser/memory/memory_condition_observer.cc (right): https://codereview.chromium.org/2763933002/diff/120001/content/browser/memory... content/browser/memory/memory_condition_observer.cc:31: const int kDefaultMonitoringIntervalSeconds = 1; It seems that base::chromeos::MemoryPressureMonitor uses 1 sec for the interval. I think we should use the same interval.
LGTM
bashi@chromium.org changed reviewers: + kinuko@chromium.org
kinuko@: Could you review content/ ?
bashi@chromium.org changed reviewers: + isherman@chromium.org
isherman@: Could you review histograms.xml?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2763933002/diff/120001/content/renderer/rende... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2763933002/diff/120001/content/renderer/rende... content/renderer/render_thread_impl.cc:2237: void RenderThreadImpl::RecordPurgeMemory(RendererMemoryMetrics before) { nit: please move this after OnPurgeMemory() (to match the method order in .cc and in .h) https://codereview.chromium.org/2763933002/diff/120001/content/renderer/rende... content/renderer/render_thread_impl.cc:2255: base::TimeDelta::FromSeconds(2)); nit: Would be nice to have a short note about why 2 sec.
The CQ bit was checked by bashi@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/2763933002/diff/120001/content/renderer/rende... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2763933002/diff/120001/content/renderer/rende... content/renderer/render_thread_impl.cc:2237: void RenderThreadImpl::RecordPurgeMemory(RendererMemoryMetrics before) { On 2017/03/28 08:07:00, kinuko wrote: > nit: please move this after OnPurgeMemory() (to match the method order in .cc > and in .h) Done. https://codereview.chromium.org/2763933002/diff/120001/content/renderer/rende... content/renderer/render_thread_impl.cc:2255: base::TimeDelta::FromSeconds(2)); On 2017/03/28 08:07:00, kinuko wrote: > nit: Would be nice to have a short note about why 2 sec. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Metrics LGTM, thanks.
The CQ bit was checked by bashi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, kinuko@chromium.org Link to the patchset: https://codereview.chromium.org/2763933002/#ps140001 (title: "comments")
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_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
The CQ bit was checked by bashi@chromium.org
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": 140001, "attempt_start_ts": 1490837030887320, "parent_rev": "7ea80c3afd06f970d40cff5d673f06f959b6dd6c", "commit_rev": "b9058f285cf483b7c49fe46e554f55f54ffeb956"}
Message was sent while issue was closed.
Description was changed from ========== memory coordinator: Purge memory under memory pressure - On WARNING condition, request a backgrounded child process to purge memory - On CRITICAL condition, try to discard a tab first. If it failed, try to purge memory from all child processes. It it failed, try to purge memory from the browser process - Use somewhat conservative heuristics for purging - Browser: suppress purging for 2 mins after a purge - Renderer: after purging, suppress the next purging until the renderer goes foreground -> background then remains backgrounded for 30 secs BUG=696844 ========== to ========== memory coordinator: Purge memory under memory pressure - On WARNING condition, request a backgrounded child process to purge memory - On CRITICAL condition, try to discard a tab first. If it failed, try to purge memory from all child processes. It it failed, try to purge memory from the browser process - Use somewhat conservative heuristics for purging - Browser: suppress purging for 2 mins after a purge - Renderer: after purging, suppress the next purging until the renderer goes foreground -> background then remains backgrounded for 30 secs BUG=696844 Review-Url: https://codereview.chromium.org/2763933002 Cr-Commit-Position: refs/heads/master@{#460622} Committed: https://chromium.googlesource.com/chromium/src/+/b9058f285cf483b7c49fe46e554f... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/b9058f285cf483b7c49fe46e554f... |