Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7930022:
chromeos: Set a least score to the currently focused tab so that it is not selected (Closed)
|
Created:
9 years, 3 months ago by amruthraj Modified:
9 years, 3 months ago CC:
chromium-reviews, roseN Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
Descriptionchromeos: Set a least score to the currently focused tab so that it is not selected
by the OOM killer to get killed.
BUG=chromium-os:18421
TEST=Verify that "He's dead JIM tab doesn't come on the currently focused tab in OOM
cases. It can still happen only if there is one tab present and it eats up all the
available system memory."
Patch by amruthraj@motorola.com, rosen.dash@motorola.com
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102134
Patch Set 1 #
Total comments: 16
Patch Set 2 : '' #
Total comments: 5
Patch Set 3 : '' #
Messages
Total messages: 13 (0 generated)
Hi, I tried to fix this bug by doing the following: - Once CompareRendererStats builds an order, scores for processes will be set in the order 1000, 999, 998 .. The least score that is set is stored. - The tab for which a NOTIFICATION_RENDER_WIDGET_VISIBILITY_CHANGED is emitted gets the least score that is stored in the previous step and the least score is updated. This continues even if the user does a Ctrl-Tab-Hold. The next tab coming to focus gets the new least score. - Once the 10 sec timer gets fired, it resets scores of all tabs as per the rules. There is one bug however that in older Linux systems with oom_adj(-15, 15), this approach would set all tabs to the same score. If you are fine with this approach, I can try to investigate on how to fix this problem. For CrOS, however, this is not a problem.
Amruth, I'm not sure this does what you want.... Questions below. http://codereview.chromium.org/7930022/diff/1/chrome/browser/oom_priority_man... File chrome/browser/oom_priority_manager.cc (right): http://codereview.chromium.org/7930022/diff/1/chrome/browser/oom_priority_man... chrome/browser/oom_priority_manager.cc:134: // Use a score 1 less than the last used score. A lesser value ensures I don't think this does what you want. oom_score_adj is an adjustment to the kernel's internal OOM score for a process - it gets added in. The kernel's portion is usually 100-200 points, depending on the process size, runtime, etc. So just adjusting by a single point won't necessarily change the order. Would it work to just set the freshly-visible tab to kLowestRendererOomScore? http://codereview.chromium.org/7930022/diff/1/chrome/browser/oom_priority_man... chrome/browser/oom_priority_manager.cc:153: case content::NOTIFICATION_RENDER_WIDGET_VISIBILITY_CHANGED: { Does this fire just when the widget becomes visible, or also when it goes invisible? http://codereview.chromium.org/7930022/diff/1/chrome/browser/oom_priority_man... chrome/browser/oom_priority_manager.cc:161: 200); 200 should be a named constant http://codereview.chromium.org/7930022/diff/1/chrome/browser/oom_priority_man... chrome/browser/oom_priority_manager.cc:261: float priority_increment = -1; I think the increment here needs to be much larger, like it was before. http://codereview.chromium.org/7930022/diff/1/chrome/browser/oom_priority_man... File chrome/browser/oom_priority_manager.h (right): http://codereview.chromium.org/7930022/diff/1/chrome/browser/oom_priority_man... chrome/browser/oom_priority_manager.h:85: // This lock is for pid_to_oom_score_ and last_set_score_. Thanks for a separate comment!
http://codereview.chromium.org/7930022/diff/1/chrome/browser/oom_priority_man... File chrome/browser/oom_priority_manager.cc (right): http://codereview.chromium.org/7930022/diff/1/chrome/browser/oom_priority_man... chrome/browser/oom_priority_manager.cc:134: // Use a score 1 less than the last used score. A lesser value ensures On 2011/09/19 17:08:55, James Cook (Chromium) wrote: > I don't think this does what you want. oom_score_adj is an adjustment to the > kernel's internal OOM score for a process - it gets added in. The kernel's > portion is usually 100-200 points, depending on the process size, runtime, etc. > So just adjusting by a single point won't necessarily change the order. > > Would it work to just set the freshly-visible tab to kLowestRendererOomScore? The problem with this approach is that when a user does a Ctrl-Tab-and-Hold, thereby switching all tabs in very short time, all of them get set to 300. That is the reason I chose the decrement approach. Is it fine to have another segment in the scores, something like 300-400 for tabs switching case and 400-1000 in DoAdjustOomPriorities case? http://codereview.chromium.org/7930022/diff/1/chrome/browser/oom_priority_man... chrome/browser/oom_priority_manager.cc:153: case content::NOTIFICATION_RENDER_WIDGET_VISIBILITY_CHANGED: { On 2011/09/19 17:08:55, James Cook (Chromium) wrote: > Does this fire just when the widget becomes visible, or also when it goes > invisible? It gets fired in invisible case too. But the if condition next line checks for only visibility case.
> The problem with this approach is that when a user does a Ctrl-Tab-and-Hold, > thereby switching all tabs in very short time, all of them get set to 300. That > is the reason I chose the decrement approach. > Is it fine to have another segment in the scores, something like 300-400 for > tabs switching case and 400-1000 in DoAdjustOomPriorities case? Greg, thoughts? > It gets fired in invisible case too. But the if condition next line checks for > only visibility case. Ah, that was not obvious to me from the code. Maybe you could pull the bool out into a named "visible" variable?
On 2011/09/19 17:36:34, James Cook (Chromium) wrote: > > The problem with this approach is that when a user does a Ctrl-Tab-and-Hold, > > thereby switching all tabs in very short time, all of them get set to 300. > That > > is the reason I chose the decrement approach. > > Is it fine to have another segment in the scores, something like 300-400 for > > tabs switching case and 400-1000 in DoAdjustOomPriorities case? > > Greg, thoughts? > > > It gets fired in invisible case too. But the if condition next line checks for > > only visibility case. > > Ah, that was not obvious to me from the code. Maybe you could pull the bool out > into a named "visible" variable? Sure. Will add a new variable to it.
Thanks for taking a look at this! http://codereview.chromium.org/7930022/diff/1/chrome/browser/oom_priority_man... File chrome/browser/oom_priority_manager.cc (right): http://codereview.chromium.org/7930022/diff/1/chrome/browser/oom_priority_man... chrome/browser/oom_priority_manager.cc:134: // Use a score 1 less than the last used score. A lesser value ensures On 2011/09/19 17:17:05, amruthraj wrote: > On 2011/09/19 17:08:55, James Cook (Chromium) wrote: > > I don't think this does what you want. oom_score_adj is an adjustment to the > > kernel's internal OOM score for a process - it gets added in. The kernel's > > portion is usually 100-200 points, depending on the process size, runtime, > etc. > > So just adjusting by a single point won't necessarily change the order. > > > > Would it work to just set the freshly-visible tab to kLowestRendererOomScore? > The problem with this approach is that when a user does a Ctrl-Tab-and-Hold, > thereby switching all tabs in very short time, all of them get set to 300. That > is the reason I chose the decrement approach. > Is it fine to have another segment in the scores, something like 300-400 for > tabs switching case and 400-1000 in DoAdjustOomPriorities case? I agree with James that this isn't really what you want. The reason we distribute the values over the range is to make sure that we provide the strongest signal possible to the kernel about the order, but it is still a "suggestion" as far as the kernel is concerned, and making them only one value apart is basically setting them equal (e.g. 1% difference in memory use can cause them to happen in a different order than desired). As for the Ctrl-TAB-and-hold issue, I don't think they all get set to 300. Basically, it drops out the "last clicked" time from the sort, but doesn't prevent the currently displayed tab or pinned tab or smaller tab from being elevated in priority: they'll still get spread out along the range based on the remaining criterion. This is also an edge case: not many people will hold down ctrl-tab. If we see this being a problem, then we can add a timer before setting the last selection time so that it won't get set unless you view the tab for longer than 500ms or something (in which case the user really is visiting the tab). http://codereview.chromium.org/7930022/diff/1/chrome/browser/oom_priority_man... chrome/browser/oom_priority_manager.cc:157: // Post a delayed task so that the tab switch time doesn't get effected. effected -> affected http://codereview.chromium.org/7930022/diff/1/chrome/browser/oom_priority_man... chrome/browser/oom_priority_manager.cc:158: BrowserThread::PostDelayedTask( This doesn't replace previous tasks in the queue, and for your ctrl-tab-and-hold case (admittedly a corner), will queue up a bunch of adjustments at once. I'd suggest using a timer to trigger the adjustment, and just resetting the timer here.
http://codereview.chromium.org/7930022/diff/1/chrome/browser/oom_priority_man... File chrome/browser/oom_priority_manager.cc (right): http://codereview.chromium.org/7930022/diff/1/chrome/browser/oom_priority_man... chrome/browser/oom_priority_manager.cc:134: // Use a score 1 less than the last used score. A lesser value ensures On 2011/09/19 19:45:51, Greg Spencer (Chromium) wrote: > On 2011/09/19 17:17:05, amruthraj wrote: > > On 2011/09/19 17:08:55, James Cook (Chromium) wrote: > > > I don't think this does what you want. oom_score_adj is an adjustment to > the > > > kernel's internal OOM score for a process - it gets added in. The kernel's > > > portion is usually 100-200 points, depending on the process size, runtime, > > etc. > > > So just adjusting by a single point won't necessarily change the order. > > > > > > Would it work to just set the freshly-visible tab to > kLowestRendererOomScore? > > The problem with this approach is that when a user does a Ctrl-Tab-and-Hold, > > thereby switching all tabs in very short time, all of them get set to 300. > That > > is the reason I chose the decrement approach. > > Is it fine to have another segment in the scores, something like 300-400 for > > tabs switching case and 400-1000 in DoAdjustOomPriorities case? > > I agree with James that this isn't really what you want. The reason we > distribute the values over the range is to make sure that we provide the > strongest signal possible to the kernel about the order, but it is still a > "suggestion" as far as the kernel is concerned, and making them only one value > apart is basically setting them equal (e.g. 1% difference in memory use can > cause them to happen in a different order than desired). > > As for the Ctrl-TAB-and-hold issue, I don't think they all get set to 300. > Basically, it drops out the "last clicked" time from the sort, but doesn't > prevent the currently displayed tab or pinned tab or smaller tab from being > elevated in priority: they'll still get spread out along the range based on the > remaining criterion. > > This is also an edge case: not many people will hold down ctrl-tab. If we see > this being a problem, then we can add a timer before setting the last selection > time so that it won't get set unless you view the tab for longer than 500ms or > something (in which case the user really is visiting the tab). Done. http://codereview.chromium.org/7930022/diff/1/chrome/browser/oom_priority_man... chrome/browser/oom_priority_manager.cc:153: case content::NOTIFICATION_RENDER_WIDGET_VISIBILITY_CHANGED: { On 2011/09/19 17:17:05, amruthraj wrote: > On 2011/09/19 17:08:55, James Cook (Chromium) wrote: > > Does this fire just when the widget becomes visible, or also when it goes > > invisible? > It gets fired in invisible case too. But the if condition next line checks for > only visibility case. Added a variable visible here. http://codereview.chromium.org/7930022/diff/1/chrome/browser/oom_priority_man... chrome/browser/oom_priority_manager.cc:157: // Post a delayed task so that the tab switch time doesn't get effected. On 2011/09/19 19:45:51, Greg Spencer (Chromium) wrote: > effected -> affected Done. http://codereview.chromium.org/7930022/diff/1/chrome/browser/oom_priority_man... chrome/browser/oom_priority_manager.cc:158: BrowserThread::PostDelayedTask( On 2011/09/19 19:45:51, Greg Spencer (Chromium) wrote: > This doesn't replace previous tasks in the queue, and for your ctrl-tab-and-hold > case (admittedly a corner), will queue up a bunch of adjustments at once. I'd > suggest using a timer to trigger the adjustment, and just resetting the timer > here. Done. http://codereview.chromium.org/7930022/diff/1/chrome/browser/oom_priority_man... chrome/browser/oom_priority_manager.cc:161: 200); On 2011/09/19 17:08:55, James Cook (Chromium) wrote: > 200 should be a named constant Done. http://codereview.chromium.org/7930022/diff/1/chrome/browser/oom_priority_man... chrome/browser/oom_priority_manager.cc:261: float priority_increment = -1; On 2011/09/19 17:08:55, James Cook (Chromium) wrote: > I think the increment here needs to be much larger, like it was before. I reverted all the changes here.
Just a couple of formatting things, then should be good to submit. http://codereview.chromium.org/7930022/diff/7001/chrome/browser/oom_priority_... File chrome/browser/oom_priority_manager.cc (right): http://codereview.chromium.org/7930022/diff/7001/chrome/browser/oom_priority_... chrome/browser/oom_priority_manager.cc:173: break; break needs to be indented, or reverse the if() conditional and wrap the rest of the block in braces. http://codereview.chromium.org/7930022/diff/7001/chrome/browser/oom_priority_... chrome/browser/oom_priority_manager.cc:179: focus_tab_score_adjust_timer_.Reset(); Good idea - it won't adjust scores while tab focus is rapidly changing. http://codereview.chromium.org/7930022/diff/7001/chrome/browser/oom_priority_... File chrome/browser/oom_priority_manager.h (right): http://codereview.chromium.org/7930022/diff/7001/chrome/browser/oom_priority_... chrome/browser/oom_priority_manager.h:69: void FocusTabScoreAdjustmentTimeout(); nit: Maybe call this PostAdjustFocusedTabScore() or OnFocusTabScoreAdjustmentTimeout()?
LGTM (after James' comments are addressed)
http://codereview.chromium.org/7930022/diff/7001/chrome/browser/oom_priority_... File chrome/browser/oom_priority_manager.cc (right): http://codereview.chromium.org/7930022/diff/7001/chrome/browser/oom_priority_... chrome/browser/oom_priority_manager.cc:173: break; On 2011/09/20 17:08:03, James Cook (Chromium) wrote: > break needs to be indented, or reverse the if() conditional and wrap the rest of > the block in braces. Done. http://codereview.chromium.org/7930022/diff/7001/chrome/browser/oom_priority_... File chrome/browser/oom_priority_manager.h (right): http://codereview.chromium.org/7930022/diff/7001/chrome/browser/oom_priority_... chrome/browser/oom_priority_manager.h:69: void FocusTabScoreAdjustmentTimeout(); On 2011/09/20 17:08:03, James Cook (Chromium) wrote: > nit: Maybe call this PostAdjustFocusedTabScore() or > OnFocusTabScoreAdjustmentTimeout()? Done.
lgtm
Change committed as 102134 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
