Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2850203003:
Do not kill recently killed ARC processes again (Closed)
|
Created:
3 years, 7 months ago by Yusuke Sato Modified:
3 years, 7 months ago CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDo not kill recently killed ARC processes again
ARC processes sometimes respawn right after being killed. In that case,
killing them every time is just a waste of resources.
BUG=706048
TEST=try
TEST=Follow crbug.com/706048#c76, confirm Chrome no longer kills the
processes too often.
Review-Url: https://codereview.chromium.org/2850203003
Cr-Commit-Position: refs/heads/master@{#468835}
Committed: https://chromium.googlesource.com/chromium/src/+/b784d0769dce13bd2efc4ae9bf13b0ffe8523a03
Patch Set 1 #
Total comments: 4
Patch Set 2 : address comments from cylee #
Total comments: 4
Patch Set 3 : address comments from Luis #
Total comments: 4
Patch Set 4 : fix comment typo #Patch Set 5 : fix another typo #Patch Set 6 : rebase #
Total comments: 1
Messages
Total messages: 35 (22 generated)
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
PTAL too. This is less important than https://codereview.chromium.org/2852093003/ but I think this is still sometimes helpful since we don't really know which processes are sticky.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
cylee@chromium.org changed reviewers: + georgesak@chromium.org
https://codereview.chromium.org/2850203003/diff/1/chrome/browser/memory/tab_m... File chrome/browser/memory/tab_manager_delegate_chromeos_unittest.cc (right): https://codereview.chromium.org/2850203003/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager_delegate_chromeos_unittest.cc:281: now - base::TimeDelta::FromSeconds(60); nit: do you mind making kArcSkipKillingTime accessible from unit test so to avoid hard-code 60 or 61 seconds in unit test ? Thus if we change the constant in the future we don't bother fix the test here. https://codereview.chromium.org/2850203003/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager_delegate_chromeos_unittest.cc:299: TEST_F(TabManagerDelegateTest, KillRecentlyKilledArcProcesses) { maybe DoNotKillRecentlyKilledArcProcess ?
The CQ bit was checked by yusukes@chromium.org to run a CQ dry run
Please take another look. https://codereview.chromium.org/2850203003/diff/1/chrome/browser/memory/tab_m... File chrome/browser/memory/tab_manager_delegate_chromeos_unittest.cc (right): https://codereview.chromium.org/2850203003/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager_delegate_chromeos_unittest.cc:281: now - base::TimeDelta::FromSeconds(60); On 2017/05/02 19:51:41, cylee1 wrote: > nit: do you mind making kArcSkipKillingTime accessible from unit test so to > avoid hard-code 60 or 61 seconds in unit test ? Thus if we change the constant > in the future we don't bother fix the test here. Done. https://codereview.chromium.org/2850203003/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager_delegate_chromeos_unittest.cc:299: TEST_F(TabManagerDelegateTest, KillRecentlyKilledArcProcesses) { On 2017/05/02 19:51:41, cylee1 wrote: > maybe DoNotKillRecentlyKilledArcProcess ? Done.
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/2850203003/diff/20001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/2850203003/diff/20001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:564: auto it = recently_killed_arc_processes_.find(process_name); nit: maybe const auto. https://codereview.chromium.org/2850203003/diff/20001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.h (right): https://codereview.chromium.org/2850203003/diff/20001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:167: static const int64_t kArcSkipKillingTimeInSeconds; nit: maybe call this kArcRespawnKillDelay or kArcRecentlyKilledDelay. Also, can this be a base::TimeDelta? It is just a very thin wrapper around an int64_t, so type-wise it should be fine.
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 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...
PTAL https://codereview.chromium.org/2850203003/diff/20001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/2850203003/diff/20001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:564: auto it = recently_killed_arc_processes_.find(process_name); On 2017/05/02 20:16:27, Luis Héctor Chávez wrote: > nit: maybe const auto. Done. https://codereview.chromium.org/2850203003/diff/20001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.h (right): https://codereview.chromium.org/2850203003/diff/20001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:167: static const int64_t kArcSkipKillingTimeInSeconds; On 2017/05/02 20:16:27, Luis Héctor Chávez wrote: > nit: maybe call this kArcRespawnKillDelay or kArcRecentlyKilledDelay. > > Also, can this be a base::TimeDelta? It is just a very thin wrapper around an > int64_t, so type-wise it should be fine. Done. Following the offline discussion, added a static function that returns a constexpr TimeDelta object.
lgtm https://codereview.chromium.org/2850203003/diff/40001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos_unittest.cc (right): https://codereview.chromium.org/2850203003/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos_unittest.cc:288: // (kArcSkipKillingTimeInSeconds + 1) seconds ago. In this case, nit: kArcSkipKillingTimeInSeconds no longer exists
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % nit. https://codereview.chromium.org/2850203003/diff/40001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos_unittest.cc (right): https://codereview.chromium.org/2850203003/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos_unittest.cc:259: // When the process name is not in the map, sRecentlyKilledArcProcess should nit: s/sRecentlyKilledArcProcess/IsRecentlyKilledArcProcess
Thanks! https://codereview.chromium.org/2850203003/diff/40001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos_unittest.cc (right): https://codereview.chromium.org/2850203003/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos_unittest.cc:259: // When the process name is not in the map, sRecentlyKilledArcProcess should On 2017/05/02 21:36:30, Georges Khalil wrote: > nit: s/sRecentlyKilledArcProcess/IsRecentlyKilledArcProcess Done. https://codereview.chromium.org/2850203003/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos_unittest.cc:288: // (kArcSkipKillingTimeInSeconds + 1) seconds ago. In this case, On 2017/05/02 20:59:53, cylee1 wrote: > nit: kArcSkipKillingTimeInSeconds no longer exists Done.
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...
https://codereview.chromium.org/2850203003/diff/100001/chrome/browser/memory/... File chrome/browser/memory/tab_manager_delegate_chromeos_unittest.cc (right): https://codereview.chromium.org/2850203003/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager_delegate_chromeos_unittest.cc:407: EXPECT_EQ(1U, processes_map.count("not-visible")); Rebased on top of the other CL. Also slightly changed L405 and 407 because of the rebase. Running git cl try again.
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 yusukes@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cylee@chromium.org, lhchavez@chromium.org, georgesak@chromium.org Link to the patchset: https://codereview.chromium.org/2850203003/#ps100001 (title: "rebase")
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": 1493770954312800,
"parent_rev": "9c5225ad517ba1f07552314930e53d9859e60f37", "commit_rev":
"b784d0769dce13bd2efc4ae9bf13b0ffe8523a03"}
Message was sent while issue was closed.
Description was changed from ========== Do not kill recently killed ARC processes again ARC processes sometimes respawn right after being killed. In that case, killing them every time is just a waste of resources. BUG=706048 TEST=try TEST=Follow crbug.com/706048#c76, confirm Chrome no longer kills the processes too often. ========== to ========== Do not kill recently killed ARC processes again ARC processes sometimes respawn right after being killed. In that case, killing them every time is just a waste of resources. BUG=706048 TEST=try TEST=Follow crbug.com/706048#c76, confirm Chrome no longer kills the processes too often. Review-Url: https://codereview.chromium.org/2850203003 Cr-Commit-Position: refs/heads/master@{#468835} Committed: https://chromium.googlesource.com/chromium/src/+/b784d0769dce13bd2efc4ae9bf13... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/b784d0769dce13bd2efc4ae9bf13...
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/2855003003/ by findit-for-me@appspot.gserviceaccount.com. The reason for reverting is: Findit (https://goo.gl/kROfz5) identified CL at revision 468835 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb.... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
