Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2843633002:
TabManager: Do not overkill when target memory to free is negative. (Closed)
DescriptionTabManager: Do not overkill when target memory to free is negative.
BUG=chrome:713944
TEST=samus
Review-Url: https://codereview.chromium.org/2843633002
Cr-Commit-Position: refs/heads/master@{#467044}
Committed: https://chromium.googlesource.com/chromium/src/+/418926bbe929301fa7d3aff43d375963ae0ce205
Patch Set 1 #Patch Set 2 : format #
Total comments: 1
Patch Set 3 : fix test #
Total comments: 3
Messages
Total messages: 23 (13 generated)
Description was changed from ========== TabManager: Do not overkill when target memory to free is negative. BUG=chrome:713944 TEST=samus ========== to ========== TabManager: Do not overkill when target memory to free is negative. BUG=chrome:713944 TEST=samus ==========
cylee@chromium.org changed reviewers: + georgesak@chromium.org, sonnyrao@chromium.org, teravest@chromium.org
Hi teravest, I'd like to ask for your advise about the log level of MEMORY_LOG(). Making it "ERROR" saves us from adding vmodule option. But to understand better about the killing status I added more MEMORY_LOG(ERROR) in the file. Do you think it's fine this way, or should I use MEMORY_LOG(USER) or something? Thanks,
lgtm I think ERROR is reasonable for these messages-- we can remove some of these if it gets too spammy. Hitting LowMemoryKillImpl should be rare, and these log messages seem valuable for debugging. https://codereview.chromium.org/2843633002/diff/20001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/2843633002/diff/20001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:640: << "Unable to kill enough candidate to meet target_memory_to_free_kb "; nit: s/candidate/candidates/
sonnyrao@google.com changed reviewers: + sonnyrao@google.com
lgtm lgtm I've been running with something like this already and it seems like it works well and is more informative when things fail to be killed.
The CQ bit was checked by cylee@chromium.org
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by cylee@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/2843633002/diff/40001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_browsertest.cc (right): https://codereview.chromium.org/2843633002/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_browsertest.cc:191: #if defined(OS_WIN) || defined(OS_MACOSX) Why are we disabling the test for CrOS here?
https://codereview.chromium.org/2843633002/diff/40001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_browsertest.cc (right): https://codereview.chromium.org/2843633002/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_browsertest.cc:191: #if defined(OS_WIN) || defined(OS_MACOSX) On 2017/04/25 15:21:15, Georges Khalil (slow) wrote: > Why are we disabling the test for CrOS here? It's what this CL fixes: on CrOS, it reads memory state (free memory, swap, etc) to figure out how much memory to free. If it sees memory has been recovered to normal level since the low memory signal had been raised, no tab is discarded. That's why this test would fail - it no longer kills "at least" one tab. TabManagerDelegate has its own test, so I just disabled the test here.
lgtm https://codereview.chromium.org/2843633002/diff/40001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_browsertest.cc (right): https://codereview.chromium.org/2843633002/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_browsertest.cc:191: #if defined(OS_WIN) || defined(OS_MACOSX) On 2017/04/25 15:46:20, cylee1 wrote: > On 2017/04/25 15:21:15, Georges Khalil (slow) wrote: > > Why are we disabling the test for CrOS here? > > It's what this CL fixes: > on CrOS, it reads memory state (free memory, swap, etc) to figure out how much > memory to free. If it sees memory has been recovered to normal level since the > low memory signal had been raised, no tab is discarded. That's why this test > would fail - it no longer kills "at least" one tab. > > TabManagerDelegate has its own test, so I just disabled the test here. Got it, thanks.
The CQ bit was checked by cylee@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from teravest@chromium.org, sonnyrao@google.com Link to the patchset: https://codereview.chromium.org/2843633002/#ps40001 (title: "fix test")
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": 1493142882828570,
"parent_rev": "0718c3ee0d51683879cf7817f3615e8a0ed69a80", "commit_rev":
"418926bbe929301fa7d3aff43d375963ae0ce205"}
Message was sent while issue was closed.
Description was changed from ========== TabManager: Do not overkill when target memory to free is negative. BUG=chrome:713944 TEST=samus ========== to ========== TabManager: Do not overkill when target memory to free is negative. BUG=chrome:713944 TEST=samus Review-Url: https://codereview.chromium.org/2843633002 Cr-Commit-Position: refs/heads/master@{#467044} Committed: https://chromium.googlesource.com/chromium/src/+/418926bbe929301fa7d3aff43d37... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/418926bbe929301fa7d3aff43d37... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
