Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2731273003:
Update MemoryMonitorChromeOS (Closed)
DescriptionUpdate MemoryMonitorChromeOS
Current guidelines for defining critical memory state is swapping will
occur in that situation. Update MemoryMonitorChromeOS to align the
guidelines.
- Use |available| field of base::SystemMemoryInfoKB if the OS supports it
- Don't count SwapFree as free available memory
BUG=696844
Review-Url: https://codereview.chromium.org/2731273003
Cr-Commit-Position: refs/heads/master@{#455970}
Committed: https://chromium.googlesource.com/chromium/src/+/a2181336154c12cb49601f515cb5dace1b06b1f2
Patch Set 1 #Patch Set 2 : Fix test #
Total comments: 6
Patch Set 3 : comment #
Messages
Total messages: 22 (16 generated)
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 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: This issue passed the CQ dry run.
bashi@chromium.org changed reviewers: + chrisha@chromium.org, haraken@chromium.org
PTAL I think this makes MemoryMonitorChromeOS be consistent with other platforms. Counting unused swap as free available memory wouldn't make sense as we've been discussing.
haraken@chromium.org changed reviewers: + wez@chromium.org
+wez LGTM on my side.
lgtm https://codereview.chromium.org/2731273003/diff/20001/content/browser/memory/... File content/browser/memory/memory_monitor_chromeos.cc (right): https://codereview.chromium.org/2731273003/diff/20001/content/browser/memory/... content/browser/memory/memory_monitor_chromeos.cc:38: file_memory -= mem_info.dirty + kMinFileMemory; Is there a risk of |file_memory| ending up -ve as a result of this calculation? https://codereview.chromium.org/2731273003/diff/20001/content/browser/memory/... File content/browser/memory/memory_monitor_chromeos_unittest.cc (right): https://codereview.chromium.org/2731273003/diff/20001/content/browser/memory/... content/browser/memory/memory_monitor_chromeos_unittest.cc:63: 32 * kKBperMB, 16 * kKBperMB, 512 * kKBperMB); Consider passing in more obviously-wrong values for the other fields, so that it's clear that this test won't pass if the 512MiB isn't used by the function. https://codereview.chromium.org/2731273003/diff/20001/content/browser/memory/... content/browser/memory/memory_monitor_chromeos_unittest.cc:67: // |available| is not supported. It seems it would be useful to verify here that the result is the same, for a given level of memory use, for a kernel that supports |available| vs one that doesn't?
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...
Thank you for review! https://codereview.chromium.org/2731273003/diff/20001/content/browser/memory/... File content/browser/memory/memory_monitor_chromeos.cc (right): https://codereview.chromium.org/2731273003/diff/20001/content/browser/memory/... content/browser/memory/memory_monitor_chromeos.cc:38: file_memory -= mem_info.dirty + kMinFileMemory; On 2017/03/08 01:59:05, Wez wrote: > Is there a risk of |file_memory| ending up -ve as a result of this calculation? Actually I'm not sure. Looks like this is a simple version of MemoryPressureMonitor::GetUsedMemoryInPercent(). Let me leave this as-is for now. https://codereview.chromium.org/2731273003/diff/20001/content/browser/memory/... File content/browser/memory/memory_monitor_chromeos_unittest.cc (right): https://codereview.chromium.org/2731273003/diff/20001/content/browser/memory/... content/browser/memory/memory_monitor_chromeos_unittest.cc:63: 32 * kKBperMB, 16 * kKBperMB, 512 * kKBperMB); On 2017/03/08 01:59:06, Wez wrote: > Consider passing in more obviously-wrong values for the other fields, so that > it's clear that this test won't pass if the 512MiB isn't used by the function. Done. https://codereview.chromium.org/2731273003/diff/20001/content/browser/memory/... content/browser/memory/memory_monitor_chromeos_unittest.cc:67: // |available| is not supported. On 2017/03/08 01:59:05, Wez wrote: > It seems it would be useful to verify here that the result is the same, for a > given level of memory use, for a kernel that supports |available| vs one that > doesn't? Done.
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 bashi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/2731273003/#ps40001 (title: "comment")
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": 40001, "attempt_start_ts": 1489114397456880,
"parent_rev": "9c002f2da505ce41c638412a8ede39e24249e7dd", "commit_rev":
"a2181336154c12cb49601f515cb5dace1b06b1f2"}
Message was sent while issue was closed.
Description was changed from ========== Update MemoryMonitorChromeOS Current guidelines for defining critical memory state is swapping will occur in that situation. Update MemoryMonitorChromeOS to align the guidelines. - Use |available| field of base::SystemMemoryInfoKB if the OS supports it - Don't count SwapFree as free available memory BUG=696844 ========== to ========== Update MemoryMonitorChromeOS Current guidelines for defining critical memory state is swapping will occur in that situation. Update MemoryMonitorChromeOS to align the guidelines. - Use |available| field of base::SystemMemoryInfoKB if the OS supports it - Don't count SwapFree as free available memory BUG=696844 Review-Url: https://codereview.chromium.org/2731273003 Cr-Commit-Position: refs/heads/master@{#455970} Committed: https://chromium.googlesource.com/chromium/src/+/a2181336154c12cb49601f515cb5... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/a2181336154c12cb49601f515cb5... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
