Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2874543002:
Refactor ARC OOM handler code (Closed)
|
Created:
3 years, 7 months ago by Yusuke Sato Modified:
3 years, 7 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, oshima+watch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor ARC OOM handler code
This CL moves around some code for Chrome OS OOM handling to make
the code a bit easier to follow:
* Previously, code for selecting "protected" processes was scattered
around the file. One was in TabManagerDelegate::GetSortedCandidates
for protecting PERSISTENT processes from *both* Chrome and kernel
OOM handler, and the other was in LowMemoryKillImpl for protecting
FOCUSED and VISIBLE processes from *only* Chrome. This CL moves the
former to AdjustOomPrioritiesImpl to simplify the code.
* Previously, tab_manager_delegate_chromeos.cc also directly used
arc::mojom::ProcessState enums. This CL moves the code to
arc::ArcProcess with unit tests to hide ARC details from the
TabManagerDelegate class.
* Previously, AdjustOomPrioritiesImpl had an assumption that the
processes the function protected from the kernel always had -1000
OOM adjustment scores. The assumption is fine, but will make it
difficult for us to e.g. protect arc::mojom::ProcessState::TOP
processes from the kernel because unlike PERSISTENT ones, TOP
processes change time to time and may have positive OOM adjustment
scores. With this CL, AdjustOomPrioritiesImpl explicitly sets
-1000 for processes the function protects to remove the assumption.
In the future, we can modify ArcProcess::IsKillableForKernel as we
like to experiment various OOM scoring policy without any
artificial restrictions.
* Add more unit tests for better code coverage.
This CL does NOT change the current Chrome/kernel OOM handling
behavior at all. This is pure code refactoring. Also, this CL
does not touch the code for handling Chrome tabs. This is only
for ARC.
BUG=719537
TEST=try
TEST=just in case, manually confirmed PERSISTENT processes still
have -1000 adjustment.
Review-Url: https://codereview.chromium.org/2874543002
Cr-Commit-Position: refs/heads/master@{#471099}
Committed: https://chromium.googlesource.com/chromium/src/+/ee315bafcc115470e4b0b994b48602650b417db2
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 7
Patch Set 3 : address comments from Luis #
Total comments: 19
Patch Set 4 : address comments #Patch Set 5 : comment fix only #Patch Set 6 : address comments from Cheng-Yu #
Total comments: 6
Messages
Total messages: 38 (24 generated)
The CQ bit was checked by yusukes@chromium.org to run a CQ dry run
Description was changed from ========== Refactor Chrome OS OOM handler code This CL moves around some code for Chrome OS OOM handling to make the code a bit easier to follow: * Previously, code for selecting "protected" processes was scattered around the file. One was in TabManagerDelegate::GetSortedCandidates for protecting PERSISTENT processes from *both* Chrome and kernel OOM handler, and the other was in LowMemoryKillImpl for protecting FOCUSED and VISIBLE processes from *only* Chrome. This CL moves the former to AdjustOomPrioritiesImpl to simplify the code. * Previously, tab_manager_delegate_chromeos.cc also directly used arc::mojom::ProcessState enums. This CL moves the code to arc::ArcProcess with unit tests to hide ARC details from the TabManagerDelegate class. * Previously, AdjustOomPrioritiesImpl had an assumption that the processes the function protected from the kernel always had -1000 OOM adjustment scores. The assumption is fine, but will make it difficult for us to e.g. protect arc::mojom::ProcessState::TOP processes from the kernel because unlike PERSISTENT ones, TOP processes change time to time and may have positive OOM adjustment scores. With this CL, AdjustOomPrioritiesImpl explicitly sets -1000 for processes the function protects to remove the assumption. In the future, we can modify ArcProcess::IsKillableForKernel as we like to experiment various OOM scoring policy without any artificial restrictions. * Add more unit tests for better code coverage. This CL does NOT change the current Chrome/kernel OOM handling behavior at all. This is pure code refactoring. Also, this CL does not touch the code for handling Chrome tabs. This is only for ARC. BUG=719537 TEST=try ========== to ========== Refactor ARC OOM handler code This CL moves around some code for Chrome OS OOM handling to make the code a bit easier to follow: * Previously, code for selecting "protected" processes was scattered around the file. One was in TabManagerDelegate::GetSortedCandidates for protecting PERSISTENT processes from *both* Chrome and kernel OOM handler, and the other was in LowMemoryKillImpl for protecting FOCUSED and VISIBLE processes from *only* Chrome. This CL moves the former to AdjustOomPrioritiesImpl to simplify the code. * Previously, tab_manager_delegate_chromeos.cc also directly used arc::mojom::ProcessState enums. This CL moves the code to arc::ArcProcess with unit tests to hide ARC details from the TabManagerDelegate class. * Previously, AdjustOomPrioritiesImpl had an assumption that the processes the function protected from the kernel always had -1000 OOM adjustment scores. The assumption is fine, but will make it difficult for us to e.g. protect arc::mojom::ProcessState::TOP processes from the kernel because unlike PERSISTENT ones, TOP processes change time to time and may have positive OOM adjustment scores. With this CL, AdjustOomPrioritiesImpl explicitly sets -1000 for processes the function protects to remove the assumption. In the future, we can modify ArcProcess::IsKillableForKernel as we like to experiment various OOM scoring policy without any artificial restrictions. * Add more unit tests for better code coverage. This CL does NOT change the current Chrome/kernel OOM handling behavior at all. This is pure code refactoring. Also, this CL does not touch the code for handling Chrome tabs. This is only for ARC. BUG=719537 TEST=try ==========
Description was changed from ========== Refactor ARC OOM handler code This CL moves around some code for Chrome OS OOM handling to make the code a bit easier to follow: * Previously, code for selecting "protected" processes was scattered around the file. One was in TabManagerDelegate::GetSortedCandidates for protecting PERSISTENT processes from *both* Chrome and kernel OOM handler, and the other was in LowMemoryKillImpl for protecting FOCUSED and VISIBLE processes from *only* Chrome. This CL moves the former to AdjustOomPrioritiesImpl to simplify the code. * Previously, tab_manager_delegate_chromeos.cc also directly used arc::mojom::ProcessState enums. This CL moves the code to arc::ArcProcess with unit tests to hide ARC details from the TabManagerDelegate class. * Previously, AdjustOomPrioritiesImpl had an assumption that the processes the function protected from the kernel always had -1000 OOM adjustment scores. The assumption is fine, but will make it difficult for us to e.g. protect arc::mojom::ProcessState::TOP processes from the kernel because unlike PERSISTENT ones, TOP processes change time to time and may have positive OOM adjustment scores. With this CL, AdjustOomPrioritiesImpl explicitly sets -1000 for processes the function protects to remove the assumption. In the future, we can modify ArcProcess::IsKillableForKernel as we like to experiment various OOM scoring policy without any artificial restrictions. * Add more unit tests for better code coverage. This CL does NOT change the current Chrome/kernel OOM handling behavior at all. This is pure code refactoring. Also, this CL does not touch the code for handling Chrome tabs. This is only for ARC. BUG=719537 TEST=try ========== to ========== Refactor ARC OOM handler code This CL moves around some code for Chrome OS OOM handling to make the code a bit easier to follow: * Previously, code for selecting "protected" processes was scattered around the file. One was in TabManagerDelegate::GetSortedCandidates for protecting PERSISTENT processes from *both* Chrome and kernel OOM handler, and the other was in LowMemoryKillImpl for protecting FOCUSED and VISIBLE processes from *only* Chrome. This CL moves the former to AdjustOomPrioritiesImpl to simplify the code. * Previously, tab_manager_delegate_chromeos.cc also directly used arc::mojom::ProcessState enums. This CL moves the code to arc::ArcProcess with unit tests to hide ARC details from the TabManagerDelegate class. * Previously, AdjustOomPrioritiesImpl had an assumption that the processes the function protected from the kernel always had -1000 OOM adjustment scores. The assumption is fine, but will make it difficult for us to e.g. protect arc::mojom::ProcessState::TOP processes from the kernel because unlike PERSISTENT ones, TOP processes change time to time and may have positive OOM adjustment scores. With this CL, AdjustOomPrioritiesImpl explicitly sets -1000 for processes the function protects to remove the assumption. In the future, we can modify ArcProcess::IsKillableForKernel as we like to experiment various OOM scoring policy without any artificial restrictions. * Add more unit tests for better code coverage. This CL does NOT change the current Chrome/kernel OOM handling behavior at all. This is pure code refactoring. Also, this CL does not touch the code for handling Chrome tabs. This is only for ARC. BUG=719537 TEST=try TEST=just in case, manually confirmed PERSISTENT processes still have -1000 adjustment. ==========
The CQ bit was checked by yusukes@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...
yusukes@chromium.org changed reviewers: + cylee@chromium.org, lhchavez@chromium.org
owners, ptal. https://codereview.chromium.org/2874543002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/process/arc_process.cc (right): https://codereview.chromium.org/2874543002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/process/arc_process.cc:13: // A special process on Android side which serves as a dummy "focused" app moved https://codereview.chromium.org/2874543002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/process/arc_process_unittest.cc (right): https://codereview.chromium.org/2874543002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/process/arc_process_unittest.cc:23: false /* is_focused */, kNow + 1); fixed a typo, sorry. https://codereview.chromium.org/2874543002/diff/20001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/2874543002/diff/20001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:89: LOG(ERROR) << "Set OOM score error: " << output; Since this is a serious failure, I'd use ERROR.
https://codereview.chromium.org/2874543002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/process/arc_process.cc (right): https://codereview.chromium.org/2874543002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/process/arc_process.cc:16: static constexpr char kArcHomeProcess[] = "org.chromium.arc.home"; nit: static is superfluous in the anonymous namespace https://codereview.chromium.org/2874543002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/process/arc_process.h (right): https://codereview.chromium.org/2874543002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/process/arc_process.h:50: bool IsKillableForKernel() const; nit: IsKillableByKernel() or IsKernelKillable()
The CQ bit was checked by yusukes@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2874543002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/process/arc_process.cc (right): https://codereview.chromium.org/2874543002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/process/arc_process.cc:16: static constexpr char kArcHomeProcess[] = "org.chromium.arc.home"; On 2017/05/09 23:00:10, Luis Héctor Chávez wrote: > nit: static is superfluous in the anonymous namespace Done. https://codereview.chromium.org/2874543002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/process/arc_process.h (right): https://codereview.chromium.org/2874543002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/process/arc_process.h:50: bool IsKillableForKernel() const; On 2017/05/09 23:00:10, Luis Héctor Chávez wrote: > nit: IsKillableByKernel() or IsKernelKillable() Done.
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
lgtm with one nit i missed earlier https://codereview.chromium.org/2874543002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/process/arc_process_unittest.cc (right): https://codereview.chromium.org/2874543002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/process/arc_process_unittest.cc:61: constexpr bool kIsFocused = false; nit: maybe kIsNotFocused?
https://codereview.chromium.org/2874543002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/process/arc_process.cc (right): https://codereview.chromium.org/2874543002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/process/arc_process.cc:16: constexpr char kArcHomeProcess[] = "org.chromium.arc.home"; A no-so-relevant comment : I'm not sure if what described in the comment still hold now. Something may had changed and I'm not sure if org.chromium.arc.home is still used that way. Could you do a quick check to see if the process is still used that way? (marked as TOP when a Chrome side window is focused, and not TOP is an Android app window is focused). https://codereview.chromium.org/2874543002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/process/arc_process.cc:46: bool ArcProcess::IsUserVisible() const { The name is not so right because processes like persistent app would be included here. And it's not always visible ? Also kArcHomeProcess (if still needed) is not "user visible" in the strict sense. https://codereview.chromium.org/2874543002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/process/arc_process.cc:52: // Protect PERSISTENT, PERSISTENT_UI, and our HOME processes since they should Thinking it twice, I feel there's no need to separate IsUserVisible() (or what its name) and IsKernelKillable(). I remember that in some discussion thread, as Luigi suggested, we should try to make chrome and kernel do the same thing. So maybe we just leave one function as IsKillable() or IsImportant(), which uses return process_state() <= mojom::ProcessState::IMPORTANT_FOREGROUND || process_name() == kArcHomeProcess; as criteria ? https://codereview.chromium.org/2874543002/diff/40001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/2874543002/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:163: return ProcessType::VISIBLE_APP; Let's rename it, it's no longer "visible app" but something like "important app" or "non-killable app". https://codereview.chromium.org/2874543002/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:657: for (auto& candidate : all_candidates) { const auto ? https://codereview.chromium.org/2874543002/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:714: DistributeOomScoreInRange(candidates.begin(), candidates.end(), score - 1, I'm confused: why is it score -1 ?
The CQ bit was checked by yusukes@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 yusukes@chromium.org to run a CQ dry run
Thanks! Please take another look. https://codereview.chromium.org/2874543002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/process/arc_process.cc (right): https://codereview.chromium.org/2874543002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/process/arc_process.cc:16: constexpr char kArcHomeProcess[] = "org.chromium.arc.home"; On 2017/05/10 16:31:54, cylee1 wrote: > A no-so-relevant comment : I'm not sure if what described in the comment still > hold now. Something may had changed and I'm not sure if org.chromium.arc.home is > still used that way. Could you do a quick check to see if the process is still > used that way? (marked as TOP when a Chrome side window is focused, and not TOP > is an Android app window is focused). The process still works like that. https://codereview.chromium.org/2874543002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/process/arc_process.cc:46: bool ArcProcess::IsUserVisible() const { On 2017/05/10 16:31:54, cylee1 wrote: > The name is not so right because processes like persistent app would be included > here. And it's not always visible ? Also kArcHomeProcess (if still needed) is > not "user visible" in the strict sense. > Done. Changed to IsImportant. https://codereview.chromium.org/2874543002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/process/arc_process.cc:52: // Protect PERSISTENT, PERSISTENT_UI, and our HOME processes since they should On 2017/05/10 16:31:55, cylee1 wrote: > Thinking it twice, I feel there's no need to separate IsUserVisible() (or what > its name) and IsKernelKillable(). > I remember that in some discussion thread, as Luigi suggested, we should try to > make chrome and kernel do the same thing. So maybe we just leave one function as > IsKillable() or IsImportant(), which uses > > return process_state() <= mojom::ProcessState::IMPORTANT_FOREGROUND || > process_name() == kArcHomeProcess; > > as criteria ? With this CL, because of the explicit SetOomScore(-1000) call, merging these two functions is possible. It may be a good idea. However, because merging the two _drastically_ changes the kernel OOM behavior, I'd avoid doing so in this pure refactoring CL. We can investigate the current OOM behavior more, and change this file later. Also, please note that Chrome and kernel do a different thing for tabs. Chrome never kills FOCUSED_TAB but the kernel still can kill it. Are you saying this should be changed too? Anyway, added a TODO to the header side. https://codereview.chromium.org/2874543002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/process/arc_process_unittest.cc (right): https://codereview.chromium.org/2874543002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/process/arc_process_unittest.cc:61: constexpr bool kIsFocused = false; On 2017/05/10 14:21:46, Luis Héctor Chávez wrote: > nit: maybe kIsNotFocused? Done. https://codereview.chromium.org/2874543002/diff/40001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/2874543002/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:163: return ProcessType::VISIBLE_APP; On 2017/05/10 16:31:55, cylee1 wrote: > Let's rename it, it's no longer "visible app" but something like "important app" > or "non-killable app". Done. https://codereview.chromium.org/2874543002/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:657: for (auto& candidate : all_candidates) { On 2017/05/10 16:31:55, cylee1 wrote: > const auto ? This is non-const for the std::move calls. https://codereview.chromium.org/2874543002/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:714: DistributeOomScoreInRange(candidates.begin(), candidates.end(), score - 1, On 2017/05/10 16:31:55, cylee1 wrote: > I'm confused: why is it score -1 ? This is because of the rounding code at L737. Added a comment.
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/2874543002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/process/arc_process.cc (right): https://codereview.chromium.org/2874543002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/process/arc_process.cc:52: // Protect PERSISTENT, PERSISTENT_UI, and our HOME processes since they should On 2017/05/10 18:29:43, Yusuke Sato wrote: > On 2017/05/10 16:31:55, cylee1 wrote: > > Thinking it twice, I feel there's no need to separate IsUserVisible() (or what > > its name) and IsKernelKillable(). > > I remember that in some discussion thread, as Luigi suggested, we should try > to > > make chrome and kernel do the same thing. So maybe we just leave one function > as > > IsKillable() or IsImportant(), which uses > > > > return process_state() <= mojom::ProcessState::IMPORTANT_FOREGROUND || > > process_name() == kArcHomeProcess; > > > > as criteria ? > > With this CL, because of the explicit SetOomScore(-1000) call, merging these two > functions is possible. It may be a good idea. > > However, because merging the two _drastically_ changes the kernel OOM behavior, > I'd avoid doing so in this pure refactoring CL. We can investigate the current > OOM behavior more, and change this file later. > > Also, please note that Chrome and kernel do a different thing for tabs. Chrome > never kills FOCUSED_TAB but the kernel still can kill it. Are you saying this > should be changed too? > > Anyway, added a TODO to the header side. Yes. I remember that's what Luigi and me discussed then (whether to kill focused tab). The bottom line is that there's no reason to make one killable by us but not by kernel or vice versa. If TabDiscarder shouldn't kill it, it means we can't handle the UI nicely. It's impossible for kernel to do it better. So let's sync them in the next CL. Thanks ! We may loop other folks to discuss this. I'll find the discussion later... https://codereview.chromium.org/2874543002/diff/40001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/2874543002/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:657: for (auto& candidate : all_candidates) { On 2017/05/10 18:29:43, Yusuke Sato wrote: > On 2017/05/10 16:31:55, cylee1 wrote: > > const auto ? > > This is non-const for the std::move calls. Acknowledged. https://codereview.chromium.org/2874543002/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:714: DistributeOomScoreInRange(candidates.begin(), candidates.end(), score - 1, On 2017/05/10 18:29:43, Yusuke Sato wrote: > On 2017/05/10 16:31:55, cylee1 wrote: > > I'm confused: why is it score -1 ? > > This is because of the rounding code at L737. Added a comment. Ah it's because of the round down of "negative" numbers. I didn't handle it nicely then. How about using math functions like "round()" at L737 to handle it better for both positive and negative values ? Even better, we can avoid floating point arithmetic by using something like range / num + (index < (range % num)) trick ? Just some random thought.
The CQ bit was checked by yusukes@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...
Please take another look. https://codereview.chromium.org/2874543002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/process/arc_process.cc (right): https://codereview.chromium.org/2874543002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/process/arc_process.cc:52: // Protect PERSISTENT, PERSISTENT_UI, and our HOME processes since they should On 2017/05/11 08:59:53, cylee1 wrote: > On 2017/05/10 18:29:43, Yusuke Sato wrote: > > On 2017/05/10 16:31:55, cylee1 wrote: > > > Thinking it twice, I feel there's no need to separate IsUserVisible() (or > what > > > its name) and IsKernelKillable(). > > > I remember that in some discussion thread, as Luigi suggested, we should try > > to > > > make chrome and kernel do the same thing. So maybe we just leave one > function > > as > > > IsKillable() or IsImportant(), which uses > > > > > > return process_state() <= mojom::ProcessState::IMPORTANT_FOREGROUND || > > > process_name() == kArcHomeProcess; > > > > > > as criteria ? > > > > With this CL, because of the explicit SetOomScore(-1000) call, merging these > two > > functions is possible. It may be a good idea. > > > > However, because merging the two _drastically_ changes the kernel OOM > behavior, > > I'd avoid doing so in this pure refactoring CL. We can investigate the current > > OOM behavior more, and change this file later. > > > > Also, please note that Chrome and kernel do a different thing for tabs. Chrome > > never kills FOCUSED_TAB but the kernel still can kill it. Are you saying this > > should be changed too? > > > > Anyway, added a TODO to the header side. > > Yes. I remember that's what Luigi and me discussed then (whether to kill focused > tab). The bottom line is that there's no reason to make one killable by us but > not by kernel or vice versa. If TabDiscarder shouldn't kill it, it means we > can't handle the UI nicely. It's impossible for kernel to do it better. So let's > sync them in the next CL. Thanks ! > We may loop other folks to discuss this. I'll find the discussion later... Acknowledged. https://codereview.chromium.org/2874543002/diff/40001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/2874543002/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:714: DistributeOomScoreInRange(candidates.begin(), candidates.end(), score - 1, On 2017/05/11 08:59:53, cylee1 wrote: > On 2017/05/10 18:29:43, Yusuke Sato wrote: > > On 2017/05/10 16:31:55, cylee1 wrote: > > > I'm confused: why is it score -1 ? > > > > This is because of the rounding code at L737. Added a comment. > > Ah it's because of the round down of "negative" numbers. I didn't handle it > nicely then. > How about using math functions like "round()" at L737 to handle it better for > both positive and negative values ? > > Even better, we can avoid floating point arithmetic by using something like > range / num + (index < (range % num)) > trick ? Just some random thought. > Done. Let me use round as this seems easier to read for everyone. https://codereview.chromium.org/2874543002/diff/100001/chrome/browser/memory/... File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/2874543002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager_delegate_chromeos.cc:661: // TODO(cylee): Also consider protecting FOCUSED_TAB from the kernel OOM Added this to track. https://codereview.chromium.org/2874543002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager_delegate_chromeos.cc:775: if (oom_scores_to_change.size()) { I believe multi-line if-body requires {}.
Thanks for the refactoring ! One more question regarding kArcHomeProcess https://codereview.chromium.org/2874543002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/process/arc_process.cc (right): https://codereview.chromium.org/2874543002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/process/arc_process.cc:48: process_name() == kArcHomeProcess; BTW, Is it possible to make the process persistent in AndroidManifest.xml ? I don't know the xml thing when writing this. https://codereview.chromium.org/2874543002/diff/100001/chrome/browser/memory/... File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/2874543002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager_delegate_chromeos.cc:661: // TODO(cylee): Also consider protecting FOCUSED_TAB from the kernel OOM On 2017/05/11 18:50:16, Yusuke Sato wrote: > Added this to track. Done. https://codereview.chromium.org/2874543002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager_delegate_chromeos.cc:775: if (oom_scores_to_change.size()) { On 2017/05/11 18:50:16, Yusuke Sato wrote: > I believe multi-line if-body requires {}. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2874543002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/process/arc_process.cc (right): https://codereview.chromium.org/2874543002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/process/arc_process.cc:48: process_name() == kArcHomeProcess; On 2017/05/11 19:29:14, cylee1 wrote: > BTW, Is it possible to make the process persistent in AndroidManifest.xml ? I > don't know the xml thing when writing this. Yes, if we want to. I'll work on it in a separate CL.
On 2017/05/11 20:01:31, Yusuke Sato wrote: > https://codereview.chromium.org/2874543002/diff/100001/chrome/browser/chromeo... > File chrome/browser/chromeos/arc/process/arc_process.cc (right): > > https://codereview.chromium.org/2874543002/diff/100001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/process/arc_process.cc:48: process_name() == > kArcHomeProcess; > On 2017/05/11 19:29:14, cylee1 wrote: > > BTW, Is it possible to make the process persistent in AndroidManifest.xml ? I > > don't know the xml thing when writing this. > > Yes, if we want to. I'll work on it in a separate CL. Great, looking forward to remove the hard code. Appreciate the effort !
lgtm lgtm
The CQ bit was checked by yusukes@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2874543002/#ps100001 (title: "address comments from Cheng-Yu")
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": 100001, "attempt_start_ts": 1494541708400510,
"parent_rev": "7c998bd51c87eca0427c67a449a5686b583c9a8e", "commit_rev":
"ee315bafcc115470e4b0b994b48602650b417db2"}
Message was sent while issue was closed.
Description was changed from ========== Refactor ARC OOM handler code This CL moves around some code for Chrome OS OOM handling to make the code a bit easier to follow: * Previously, code for selecting "protected" processes was scattered around the file. One was in TabManagerDelegate::GetSortedCandidates for protecting PERSISTENT processes from *both* Chrome and kernel OOM handler, and the other was in LowMemoryKillImpl for protecting FOCUSED and VISIBLE processes from *only* Chrome. This CL moves the former to AdjustOomPrioritiesImpl to simplify the code. * Previously, tab_manager_delegate_chromeos.cc also directly used arc::mojom::ProcessState enums. This CL moves the code to arc::ArcProcess with unit tests to hide ARC details from the TabManagerDelegate class. * Previously, AdjustOomPrioritiesImpl had an assumption that the processes the function protected from the kernel always had -1000 OOM adjustment scores. The assumption is fine, but will make it difficult for us to e.g. protect arc::mojom::ProcessState::TOP processes from the kernel because unlike PERSISTENT ones, TOP processes change time to time and may have positive OOM adjustment scores. With this CL, AdjustOomPrioritiesImpl explicitly sets -1000 for processes the function protects to remove the assumption. In the future, we can modify ArcProcess::IsKillableForKernel as we like to experiment various OOM scoring policy without any artificial restrictions. * Add more unit tests for better code coverage. This CL does NOT change the current Chrome/kernel OOM handling behavior at all. This is pure code refactoring. Also, this CL does not touch the code for handling Chrome tabs. This is only for ARC. BUG=719537 TEST=try TEST=just in case, manually confirmed PERSISTENT processes still have -1000 adjustment. ========== to ========== Refactor ARC OOM handler code This CL moves around some code for Chrome OS OOM handling to make the code a bit easier to follow: * Previously, code for selecting "protected" processes was scattered around the file. One was in TabManagerDelegate::GetSortedCandidates for protecting PERSISTENT processes from *both* Chrome and kernel OOM handler, and the other was in LowMemoryKillImpl for protecting FOCUSED and VISIBLE processes from *only* Chrome. This CL moves the former to AdjustOomPrioritiesImpl to simplify the code. * Previously, tab_manager_delegate_chromeos.cc also directly used arc::mojom::ProcessState enums. This CL moves the code to arc::ArcProcess with unit tests to hide ARC details from the TabManagerDelegate class. * Previously, AdjustOomPrioritiesImpl had an assumption that the processes the function protected from the kernel always had -1000 OOM adjustment scores. The assumption is fine, but will make it difficult for us to e.g. protect arc::mojom::ProcessState::TOP processes from the kernel because unlike PERSISTENT ones, TOP processes change time to time and may have positive OOM adjustment scores. With this CL, AdjustOomPrioritiesImpl explicitly sets -1000 for processes the function protects to remove the assumption. In the future, we can modify ArcProcess::IsKillableForKernel as we like to experiment various OOM scoring policy without any artificial restrictions. * Add more unit tests for better code coverage. This CL does NOT change the current Chrome/kernel OOM handling behavior at all. This is pure code refactoring. Also, this CL does not touch the code for handling Chrome tabs. This is only for ARC. BUG=719537 TEST=try TEST=just in case, manually confirmed PERSISTENT processes still have -1000 adjustment. Review-Url: https://codereview.chromium.org/2874543002 Cr-Commit-Position: refs/heads/master@{#471099} Committed: https://chromium.googlesource.com/chromium/src/+/ee315bafcc115470e4b0b994b486... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/ee315bafcc115470e4b0b994b486... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
