Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2893943003:
TabManager: Add more logs to understand how it works when memory is low. (Closed)
DescriptionTabManager: Add more logs to understand how it works when memory is low.
BUG=721629
TEST=tried on cave
Review-Url: https://codereview.chromium.org/2893943003
Cr-Commit-Position: refs/heads/master@{#472970}
Committed: https://chromium.googlesource.com/chromium/src/+/576b449ac1a7a0fc6a3d55fec99bf5e6e91d4de2
Patch Set 1 #
Total comments: 19
Patch Set 2 : review comments #Patch Set 3 : review comments #
Total comments: 12
Patch Set 4 : more review comments #
Messages
Total messages: 25 (13 generated)
cylee@chromium.org changed reviewers: + sonnyrao@chromium.org, teravest@chromium.org, yusukes@chromium.org
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...
lgtm This should be helpful, thanks! https://codereview.chromium.org/2893943003/diff/1/chrome/browser/memory/tab_m... File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/2893943003/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager_delegate_chromeos.cc:124: const auto& app = candidate.app(); nit: Mind using the type explicitly here? It's not obvious that the type of app is arc::ArcProcess https://codereview.chromium.org/2893943003/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager_delegate_chromeos.cc:130: const auto& tab = candidate.tab(); nit: Mind using the type explicitly here? It's not obvious that the type of tab is TabStats*
https://codereview.chromium.org/2893943003/diff/1/chrome/browser/memory/tab_m... File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/2893943003/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager_delegate_chromeos.cc:125: out << "app " << app->process_name() << " (" << app->pid() << ")" How about moving this to c/b/c/arc/process/arc_process.h and .cc? Either ToString() or operator<< seems fine to me. https://codereview.chromium.org/2893943003/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager_delegate_chromeos.cc:126: << ", process_state: " << app->process_state() Can you stringify the state number? https://codereview.chromium.org/2893943003/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager_delegate_chromeos.cc:128: << ", last_activity_time: " << app->last_activity_time(); Can you also dump packages_ vector? https://codereview.chromium.org/2893943003/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager_delegate_chromeos.cc:131: out << "tab " << tab->title << " (" << tab->renderer_handle << ")" How did you select the variables to dump? https://chromium.googlesource.com/chromium/src/+/3f7781da5e43f9c32b5e4654e08b... checks more. What are we trying to understand from the new log? https://codereview.chromium.org/2893943003/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager_delegate_chromeos.cc:599: MEMORY_LOG(ERROR) << "List of all low memory kill candidates:"; Mention that the list is sorted? https://codereview.chromium.org/2893943003/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager_delegate_chromeos.cc:617: MEMORY_LOG(ERROR) << "Skipped killing " << *it; Using *it inside the second loop might be too spammy. It'll end up dumping many of the candidates twice. Can you change L617, L622, etc., to << it->ToShortString(); or something like that to only dump a process name and its PID?
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_...)
https://codereview.chromium.org/2893943003/diff/1/chrome/browser/memory/tab_m... File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/2893943003/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager_delegate_chromeos.cc:124: const auto& app = candidate.app(); On 2017/05/18 20:11:55, teravest wrote: > nit: Mind using the type explicitly here? It's not obvious that the type of app > is arc::ArcProcess Also, const auto& failed git-cl try. FYI.
https://codereview.chromium.org/2893943003/diff/1/chrome/browser/memory/tab_m... File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/2893943003/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager_delegate_chromeos.cc:126: << ", process_state: " << app->process_state() On 2017/05/18 20:24:51, Yusuke Sato wrote: > Can you stringify the state number? nvm, it's already done in the mojo-generated operator<<.
https://codereview.chromium.org/2893943003/diff/1/chrome/browser/memory/tab_m... File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/2893943003/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager_delegate_chromeos.cc:124: const auto& app = candidate.app(); On 2017/05/18 20:27:36, Yusuke Sato wrote: > On 2017/05/18 20:11:55, teravest wrote: > > nit: Mind using the type explicitly here? It's not obvious that the type of > app > > is arc::ArcProcess > > Also, const auto& failed git-cl try. FYI. Done. https://codereview.chromium.org/2893943003/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager_delegate_chromeos.cc:125: out << "app " << app->process_name() << " (" << app->pid() << ")" On 2017/05/18 20:24:51, Yusuke Sato wrote: > How about moving this to c/b/c/arc/process/arc_process.h and .cc? Either > ToString() or operator<< seems fine to me. Done. https://codereview.chromium.org/2893943003/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager_delegate_chromeos.cc:126: << ", process_state: " << app->process_state() On 2017/05/18 20:24:51, Yusuke Sato wrote: > Can you stringify the state number? It's already printed like ProcessState::PERSISTENT https://codereview.chromium.org/2893943003/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager_delegate_chromeos.cc:128: << ", last_activity_time: " << app->last_activity_time(); On 2017/05/18 20:24:51, Yusuke Sato wrote: > Can you also dump packages_ vector? Done. https://codereview.chromium.org/2893943003/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager_delegate_chromeos.cc:130: const auto& tab = candidate.tab(); On 2017/05/18 20:11:55, teravest wrote: > nit: Mind using the type explicitly here? It's not obvious that the type of tab > is TabStats* Done. https://codereview.chromium.org/2893943003/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager_delegate_chromeos.cc:131: out << "tab " << tab->title << " (" << tab->renderer_handle << ")" On 2017/05/18 20:24:52, Yusuke Sato wrote: > How did you select the variables to dump? > https://chromium.googlesource.com/chromium/src/+/3f7781da5e43f9c32b5e4654e08b... > checks more. What are we trying to understand from the new log? I tried to select most relevant fields from my experience of reading system logs from feedback report. I understand your point here, but I'm little afraid to over spam the log if I add more fields. I'd like to keep it simple for now. If to add more fields, https://chromium.googlesource.com/chromium/src/+/3f7781da5e43f9c32b5e4654e08b... would be my top consideration. https://codereview.chromium.org/2893943003/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager_delegate_chromeos.cc:599: MEMORY_LOG(ERROR) << "List of all low memory kill candidates:"; On 2017/05/18 20:24:51, Yusuke Sato wrote: > Mention that the list is sorted? Done. https://codereview.chromium.org/2893943003/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager_delegate_chromeos.cc:617: MEMORY_LOG(ERROR) << "Skipped killing " << *it; On 2017/05/18 20:24:52, Yusuke Sato wrote: > Using *it inside the second loop might be too spammy. It'll end up dumping many > of the candidates twice. Can you change L617, L622, etc., to > > << it->ToShortString(); > > or something like that to only dump a process name and its PID? Done.
Thanks, style nits below: https://codereview.chromium.org/2893943003/diff/1/chrome/browser/memory/tab_m... File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/2893943003/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager_delegate_chromeos.cc:131: out << "tab " << tab->title << " (" << tab->renderer_handle << ")" On 2017/05/18 21:10:30, cylee1 wrote: > On 2017/05/18 20:24:52, Yusuke Sato wrote: > > How did you select the variables to dump? > > > https://chromium.googlesource.com/chromium/src/+/3f7781da5e43f9c32b5e4654e08b... > > checks more. What are we trying to understand from the new log? > > I tried to select most relevant fields from my experience of reading system logs > from feedback report. > I understand your point here, but I'm little afraid to over spam the log if I > add more fields. I'd like to keep it simple for now. If to add more fields, > https://chromium.googlesource.com/chromium/src/+/3f7781da5e43f9c32b5e4654e08b... > would be my top consideration. Acknowledged. https://codereview.chromium.org/2893943003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/process/arc_process.cc (right): https://codereview.chromium.org/2893943003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/process/arc_process.cc:49: out << "Arc process " << arc_process.process_name() << " (" nit: it might be unclear that the number inside the paren is a PID. What about "just" dump everything? out << "process_name: " << arc_process.process_name() << ", pid: " << arc_process.pid() https://codereview.chromium.org/2893943003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/process/arc_process.cc:56: out << pkg << ","; The last "," seems redundant. #include "base/strings/string_util.h" then << ", packages: " << base::JoinString(arc_process.packages(), ","); (or using " " as a separator) would be better. https://codereview.chromium.org/2893943003/diff/40001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/2893943003/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:124: out << *candidate.app(); nit: ...and add back "app " here? https://codereview.chromium.org/2893943003/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:127: out << "tab " << tab->title << " (" << tab->renderer_handle << ")" nit: same. maybe tab->title << ", renderer_handle: " << tab->renderer_handle ? https://codereview.chromium.org/2893943003/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:595: MEMORY_LOG(ERROR) << "List of sorted low memory kill candidates:"; lowest-priority first, right? can you mention that?
sorry, one more. https://codereview.chromium.org/2893943003/diff/40001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/2893943003/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:628: MEMORY_LOG(ERROR) << "Killed " << it->app()->process_name() nit: pid is missing which is inconsistent with L645.
https://codereview.chromium.org/2893943003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/process/arc_process.cc (right): https://codereview.chromium.org/2893943003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/process/arc_process.cc:49: out << "Arc process " << arc_process.process_name() << " (" On 2017/05/18 21:42:49, Yusuke Sato wrote: > nit: it might be unclear that the number inside the paren is a PID. What about > "just" dump everything? > > out << "process_name: " << arc_process.process_name() << ", pid: " << > arc_process.pid() Done. https://codereview.chromium.org/2893943003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/process/arc_process.cc:56: out << pkg << ","; On 2017/05/18 21:42:49, Yusuke Sato wrote: > The last "," seems redundant. > > #include "base/strings/string_util.h" > > then > > << ", packages: " << base::JoinString(arc_process.packages(), ","); > > (or using " " as a separator) would be better. thanks ! haven't work on the code base for a while so forget the util library .. https://codereview.chromium.org/2893943003/diff/40001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/2893943003/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:124: out << *candidate.app(); On 2017/05/18 21:42:49, Yusuke Sato wrote: > nit: ...and add back "app " here? Done. https://codereview.chromium.org/2893943003/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:127: out << "tab " << tab->title << " (" << tab->renderer_handle << ")" On 2017/05/18 21:42:49, Yusuke Sato wrote: > nit: same. maybe > > tab->title << ", renderer_handle: " << tab->renderer_handle > > ? Done. https://codereview.chromium.org/2893943003/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:595: MEMORY_LOG(ERROR) << "List of sorted low memory kill candidates:"; On 2017/05/18 21:42:49, Yusuke Sato wrote: > lowest-priority first, right? can you mention that? Done. https://codereview.chromium.org/2893943003/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:628: MEMORY_LOG(ERROR) << "Killed " << it->app()->process_name() On 2017/05/18 21:45:49, Yusuke Sato wrote: > nit: pid is missing which is inconsistent with L645. That's intentional. Normally there's only one app with a given process name (?) So I think it can be uniquely identified. Tab title are different though. There can be multiple tabs on the same webpage. So I think it needs to be uniquely identified by pid. That said, adding it back is not harmful :)
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...
lgtm
The CQ bit was unchecked by cylee@chromium.org
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 Link to the patchset: https://codereview.chromium.org/2893943003/#ps60001 (title: "more review comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== TabManager: Add more logs to understand how it works when memory is low. BUG=chrmoium:721629 TEST=tried on cave ========== to ========== TabManager: Add more logs to understand how it works when memory is low. BUG=721629 TEST=tried on cave ==========
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495146155779030,
"parent_rev": "4ef9da30e7037ec462c6238237f7e09c18d085d7", "commit_rev":
"576b449ac1a7a0fc6a3d55fec99bf5e6e91d4de2"}
Message was sent while issue was closed.
Description was changed from ========== TabManager: Add more logs to understand how it works when memory is low. BUG=721629 TEST=tried on cave ========== to ========== TabManager: Add more logs to understand how it works when memory is low. BUG=721629 TEST=tried on cave Review-Url: https://codereview.chromium.org/2893943003 Cr-Commit-Position: refs/heads/master@{#472970} Committed: https://chromium.googlesource.com/chromium/src/+/576b449ac1a7a0fc6a3d55fec99b... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/576b449ac1a7a0fc6a3d55fec99b... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
