Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2374343002:
Add MemoryCoordinatorImpl (Closed)
DescriptionAdd MemoryCoordinatorImpl
This CL adds actual implementation of memory coordinator.
The idea is from chrisha@ and a basic strategy is:
* There is only one global state. All processes share the same state.
* The state level calculation is determined by the amount of free
memory and the median of renderer size (platform-dependent).
BUG=617492
Committed: https://crrev.com/529cb14c66359213f1d19b0450ff4d73b25c0e10
Cr-Commit-Position: refs/heads/master@{#425786}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Implement #msg4 #
Total comments: 8
Patch Set 3 : Addressed comments (still lacks tests & comments) #
Total comments: 2
Patch Set 4 : Add comments and tests #
Total comments: 1
Patch Set 5 : Attempt to fix mac failure #
Total comments: 15
Patch Set 6 : Addressed comments #Patch Set 7 : disable test on mac #Patch Set 8 : Update comments and rebase #
Total comments: 14
Patch Set 9 : comment #
Total comments: 9
Patch Set 10 : comments #Messages
Total messages: 45 (20 generated)
bashi@chromium.org changed reviewers: + chrisha@chromium.org, haraken@chromium.org
Hi Chris and Haraken-san, This lacks comments and tests but before moving forward I'd like to have your feedback on this. What do you think about this approach? I'm not 100% sure I got Chris's idea correctly. Let me know if I miss your idea.
https://codereview.chromium.org/2374343002/diff/1/content/browser/memory/memo... File content/browser/memory/memory_coordinator_impl.cc (right): https://codereview.chromium.org/2374343002/diff/1/content/browser/memory/memo... content/browser/memory/memory_coordinator_impl.cc:81: // of child processes. I'm curious what Chris has in mind, but I'd define the global state as follows: - NORMAL: MC is taking no action because the memory is under the threshold. - THROTTLING: MC is in a phase to throttle background renderers until the memory goes down to the threshold. - SUSPENDING: MC has throttled all background renderers. MC is in a phase to suspend background renderers until the memory goes down to the threshold. - DISCARDING: MC has suspended all background renderers. MC is in a phase to discard background renderers until the memory goes down to the threshold. My 2 cents.
My intended logic is as follows (for a cheap and dirty v0):
- Pass in a MemoryMonitor in the constructor.
- Poll on a 5 second timer, each time calculating memory free until critical.
- For each platform, have a constant which corresponds to the median? 75th
percentile? renderer size (taken from Memory.Renderer UMA)
- Use the median renderer size to convert the amount of free memory until
critical to a "number of renderers that can be created until critical". Call
this N.
- Use N to determine the memory state, using a simple mapping and some
hysteresis:
switch (current_state) {
case NORMAL:
if (N <= 1) return PURGED;
if (N <= 3) return THROTTLED;
return NORMAL;
case THROTTLED:
if (N <= 1) return PURGED;
if (N >= 4) return NORMAL;
return THROTTLED;
case PURGED:
if (N >= 4) return NORMAL;
if (N >= 2) return THROTTLED;
return PURGED;
}
The constants 1/2/3/4 above are chosen out of thin air, but this is a start so
we can start grabbing numbers and tuning.
Additional logic would ensure that a transition to a more restricted state
endures for at least ~30 seconds before being allowed to go a less restricted
state. No such limit need be observed for transitioning to a *more* restricted
state (that is, at one poll period we could transition to THROTTLED, and in the
immediate next polling period we could decide to go to PURGED).
If a new renderer is created it should be transitioned to the current memory
state at some point. Maybe we guarantee it some amount of time at NORMAL in
order to get spooled up? (Say, 30 seconds again?)
Is this more clear?
(Again, all of these constants are quite arbitrary. The main thing is make the
system work in order to ensure that there is sufficient memory to spool up
another renderer at any given point in time. If PURGED doesn't suffice, we'd
then move on to tab discarding, but we can worry about integrating that logic
afterwards.)
https://codereview.chromium.org/2374343002/diff/1/content/browser/memory/memo... File content/browser/memory/memory_coordinator.h (right): https://codereview.chromium.org/2374343002/diff/1/content/browser/memory/memo... content/browser/memory/memory_coordinator.h:78: virtual void OnChildRemoved() {} Why are these needed? The Impl should be able to infer that a child has been added or removed by inspecting the map, and need not react immediately. https://codereview.chromium.org/2374343002/diff/1/content/browser/memory/memo... content/browser/memory/memory_coordinator.h:96: class MemoryCoordinatorHandleImpl : public mojom::MemoryCoordinatorHandle { Why do we need to expose this impl detail?
On 2016/09/30 02:55:49, chrisha (slow) wrote:
> My intended logic is as follows (for a cheap and dirty v0):
>
> - Pass in a MemoryMonitor in the constructor.
> - Poll on a 5 second timer, each time calculating memory free until critical.
> - For each platform, have a constant which corresponds to the median? 75th
> percentile? renderer size (taken from Memory.Renderer UMA)
> - Use the median renderer size to convert the amount of free memory until
> critical to a "number of renderers that can be created until critical". Call
> this N.
> - Use N to determine the memory state, using a simple mapping and some
> hysteresis:
>
> switch (current_state) {
> case NORMAL:
> if (N <= 1) return PURGED;
> if (N <= 3) return THROTTLED;
> return NORMAL;
> case THROTTLED:
> if (N <= 1) return PURGED;
> if (N >= 4) return NORMAL;
> return THROTTLED;
> case PURGED:
> if (N >= 4) return NORMAL;
> if (N >= 2) return THROTTLED;
> return PURGED;
> }
>
> The constants 1/2/3/4 above are chosen out of thin air, but this is a start so
> we can start grabbing numbers and tuning.
>
> Additional logic would ensure that a transition to a more restricted state
> endures for at least ~30 seconds before being allowed to go a less restricted
> state. No such limit need be observed for transitioning to a *more* restricted
> state (that is, at one poll period we could transition to THROTTLED, and in
the
> immediate next polling period we could decide to go to PURGED).
>
> If a new renderer is created it should be transitioned to the current memory
> state at some point. Maybe we guarantee it some amount of time at NORMAL in
> order to get spooled up? (Say, 30 seconds again?)
>
> Is this more clear?
>
> (Again, all of these constants are quite arbitrary. The main thing is make the
> system work in order to ensure that there is sufficient memory to spool up
> another renderer at any given point in time. If PURGED doesn't suffice, we'd
> then move on to tab discarding, but we can worry about integrating that logic
> afterwards.)
Just to confirm: You're assuming that all MCCs' states are synchronized with
MC's state, right? In other words, when MC transitions from NORMAL to THROTTLED,
all MCCs transition to THROTTLED accordingly.
Your plan sounds reasonable. We can implement the logic and iterate. Honestly
speaking, I don't know how we should implement the heuristics without making it
work in the wild and collecting some numbers :)
Yup, for simplicity I'm suggesting that initially everything works in lockstep: MC, CMC, and all MCCs. On Fri, Sep 30, 2016, 01:40 <haraken@chromium.org> wrote: > On 2016/09/30 02:55:49, chrisha (slow) wrote: > > My intended logic is as follows (for a cheap and dirty v0): > > > > - Pass in a MemoryMonitor in the constructor. > > - Poll on a 5 second timer, each time calculating memory free until > critical. > > - For each platform, have a constant which corresponds to the median? > 75th > > percentile? renderer size (taken from Memory.Renderer UMA) > > - Use the median renderer size to convert the amount of free memory until > > critical to a "number of renderers that can be created until critical". > Call > > this N. > > - Use N to determine the memory state, using a simple mapping and some > > hysteresis: > > > > switch (current_state) { > > case NORMAL: > > if (N <= 1) return PURGED; > > if (N <= 3) return THROTTLED; > > return NORMAL; > > case THROTTLED: > > if (N <= 1) return PURGED; > > if (N >= 4) return NORMAL; > > return THROTTLED; > > case PURGED: > > if (N >= 4) return NORMAL; > > if (N >= 2) return THROTTLED; > > return PURGED; > > } > > > > The constants 1/2/3/4 above are chosen out of thin air, but this is a > start so > > we can start grabbing numbers and tuning. > > > > Additional logic would ensure that a transition to a more restricted > state > > endures for at least ~30 seconds before being allowed to go a less > restricted > > state. No such limit need be observed for transitioning to a *more* > restricted > > state (that is, at one poll period we could transition to THROTTLED, and > in > the > > immediate next polling period we could decide to go to PURGED). > > > > If a new renderer is created it should be transitioned to the current > memory > > state at some point. Maybe we guarantee it some amount of time at NORMAL > in > > order to get spooled up? (Say, 30 seconds again?) > > > > Is this more clear? > > > > (Again, all of these constants are quite arbitrary. The main thing is > make the > > system work in order to ensure that there is sufficient memory to spool > up > > another renderer at any given point in time. If PURGED doesn't suffice, > we'd > > then move on to tab discarding, but we can worry about integrating that > logic > > afterwards.) > > Just to confirm: You're assuming that all MCCs' states are synchronized > with > MC's state, right? In other words, when MC transitions from NORMAL to > THROTTLED, > all MCCs transition to THROTTLED accordingly. > > Your plan sounds reasonable. We can implement the logic and iterate. > Honestly > speaking, I don't know how we should implement the heuristics without > making it > work in the wild and collecting some numbers :) > > > > https://codereview.chromium.org/2374343002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thanks for detailed explanations. I have better understanding of your idea now :) Patch set #2 is still work-in-progress but I'd like you to take another look at this before polishing this for actual review.
Heading in the right direction... https://codereview.chromium.org/2374343002/diff/20001/content/browser/memory/... File content/browser/memory/memory_coordinator_impl.cc (right): https://codereview.chromium.org/2374343002/diff/20001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:77: timer_.Start(FROM_HERE, config_.monitoring_iterval, Don't use a repeating timer? Or a timer at all? If we're going to have a minimum state time period, you only need to schedule a further update once that much time has expired. So make each call to UpdateState responsible for scheduling the next call? https://codereview.chromium.org/2374343002/diff/20001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:96: if (num_children_until_critical < config_.num_children_until_suspended) I'd want some hysteresis here. For example, shift to "suspended" when num_children_until_critical <= 2, but don't shift back to throttled untol num_children_until_critical >= 3. (More specifically, I'm suggesting a deadband as a way to prevent thrashing.) But, we already have a minimum_transition_period, so maybe this is overkill. Without the deadband, this code can be vastly simplified and doesn't need to be written as a switch, as the current state doesn't affect the next state: if (n < s) return SUSPENDED; if (n < t) return THROTTLED; return NORMAL; https://codereview.chromium.org/2374343002/diff/20001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:140: // TODO(bashi): Maybe worth considering another state for the browser. We also need to consider tabs that can't be suspended. For example, tabs playing audio. Either we make the make the memory coordinator be aware of these things, or we make the renderer ignore the state change, or ... ? Thoughts? Ideas? https://codereview.chromium.org/2374343002/diff/20001/content/browser/memory/... File content/browser/memory/memory_coordinator_impl.h (right): https://codereview.chromium.org/2374343002/diff/20001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.h:40: }; Not sure we need a separate struct for this. I'd simply make these constants that are all defined in the .cc file, defined in the anonymous namespace and with detailed comments. If we decide to make something configurable, promote it to a member variable, and add a function for setting the value that can be overridden / uses Finch / etc. (This is more in line with the pattern I've seen used across Chrome for such things.)
Thanks for review (and sorry for delay...)! I'll start adding tests and comments when overall design looks good. https://codereview.chromium.org/2374343002/diff/20001/content/browser/memory/... File content/browser/memory/memory_coordinator_impl.cc (right): https://codereview.chromium.org/2374343002/diff/20001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:77: timer_.Start(FROM_HERE, config_.monitoring_iterval, On 2016/10/04 20:00:53, chrisha (slow) wrote: > Don't use a repeating timer? Or a timer at all? > > If we're going to have a minimum state time period, you only need to schedule a > further update once that much time has expired. So make each call to UpdateState > responsible for scheduling the next call? Removed timer. https://codereview.chromium.org/2374343002/diff/20001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:96: if (num_children_until_critical < config_.num_children_until_suspended) On 2016/10/04 20:00:53, chrisha (slow) wrote: > I'd want some hysteresis here. For example, shift to "suspended" when > num_children_until_critical <= 2, but don't shift back to throttled untol > num_children_until_critical >= 3. > > (More specifically, I'm suggesting a deadband as a way to prevent thrashing.) > > But, we already have a minimum_transition_period, so maybe this is overkill. > > Without the deadband, this code can be vastly simplified and doesn't need to be > written as a switch, as the current state doesn't affect the next state: > > if (n < s) > return SUSPENDED; > if (n < t) > return THROTTLED; > return NORMAL; I see. Added variables for deadbands. https://codereview.chromium.org/2374343002/diff/20001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:140: // TODO(bashi): Maybe worth considering another state for the browser. On 2016/10/04 20:00:53, chrisha (slow) wrote: > We also need to consider tabs that can't be suspended. For example, tabs playing > audio. Either we make the make the memory coordinator be aware of these things, > or we make the renderer ignore the state change, or ... ? > > Thoughts? Ideas? I missed that. Maybe we need to add logic both browser and renderer. I think there are some options. I'll write a doc to discuss them in details. Either way I think we can address it in a separate CL (Maybe we will solve that problem before submitting this CL) https://codereview.chromium.org/2374343002/diff/20001/content/browser/memory/... File content/browser/memory/memory_coordinator_impl.h (right): https://codereview.chromium.org/2374343002/diff/20001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.h:40: }; On 2016/10/04 20:00:53, chrisha (slow) wrote: > Not sure we need a separate struct for this. I'd simply make these constants > that are all defined in the .cc file, defined in the anonymous namespace and > with detailed comments. If we decide to make something configurable, promote it > to a member variable, and add a function for setting the value that can be > overridden / uses Finch / etc. > > (This is more in line with the pattern I've seen used across Chrome for such > things.) Changed to put these as member variables (though I think putting them together in a single struct would be easy to read). I'll add comments when preparing actual review. I didn't make these constants because we would want to configure them for finch or tests.
Overall logic looks good to me.
https://codereview.chromium.org/2374343002/diff/40001/content/browser/memory/... File content/browser/memory/memory_coordinator_impl.h (right): https://codereview.chromium.org/2374343002/diff/40001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.h:58: base::TimeDelta monitoring_iterval_; Unless these are needed for unittesting, or otherwise need to be exposed, I'd make these all static constants in an anonymous namespace in the .cc file. If and when they need to be exposed for testing or experimentation, then move them as need be. num_children_* is slightly confusing, as this has nothing to do with the current number of children, rather the expected number of new children/renderers that could be created. Maybe new_renderers_* ? I would also prefer to see detailed comments for each of these configuration items, explaining exactly how they work. // The median size of a renderer on the current platform. This is used to convert // the amount of free memory to an expected number of new renderers that could be // started before hitting critical memory pressure. int predicted_renderer_size_; // When in a NORMAL state and the potential number of new renderers drop below this // level, the coordinator will transition to a THROTTLED state. int new_renderers_until_throttled_; Somewhere a detailed description of the current logic would be useful (deadband, minimum transition time, etc)
The CQ bit was checked by bashi@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...
Description was changed from ========== WIP: Implement MemoryCoordinatorImpl v0 (Work in progress and isn't ready for review) An implementation idea for MemoryCoordinatorImpl V0. The idea is from chrisha@ and a basic strategy is: * There is only one global state. All processes share the same state. * The state level calculation is determined by the number of renderers. Note: There are a lot of things we should consider (e.g. process prioritization, when to update state). BUG=617492 ========== to ========== Add MemoryCoordinatorImpl This CL adds actual implementation of memory coordinator. The idea is from chrisha@ and a basic strategy is: * There is only one global state. All processes share the same state. * The state level calculation is determined by the amount of free memory and the median of renderer size (platform-dependent). BUG=617492 ==========
Added tests and comments. https://codereview.chromium.org/2374343002/diff/40001/content/browser/memory/... File content/browser/memory/memory_coordinator_impl.h (right): https://codereview.chromium.org/2374343002/diff/40001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.h:58: base::TimeDelta monitoring_iterval_; On 2016/10/06 13:35:54, chrisha (slow) wrote: > Unless these are needed for unittesting, or otherwise need to be exposed, I'd > make these all static constants in an anonymous namespace in the .cc file. If > and when they need to be exposed for testing or experimentation, then move them > as need be. > > num_children_* is slightly confusing, as this has nothing to do with the current > number of children, rather the expected number of new children/renderers that > could be created. Maybe new_renderers_* ? > > I would also prefer to see detailed comments for each of these configuration > items, explaining exactly how they work. > > // The median size of a renderer on the current platform. This is used to > convert > // the amount of free memory to an expected number of new renderers that could > be > // started before hitting critical memory pressure. > int predicted_renderer_size_; > // When in a NORMAL state and the potential number of new renderers drop below > this > // level, the coordinator will transition to a THROTTLED state. > int new_renderers_until_throttled_; > > Somewhere a detailed description of the current logic would be useful (deadband, > minimum transition time, etc) Added comments. I still put these parameters in this class because I think we would want to tweak these parameters on Finch or local experiments. https://codereview.chromium.org/2374343002/diff/60001/content/browser/memory/... File content/browser/memory/memory_coordinator_impl_unittest.cc (right): https://codereview.chromium.org/2374343002/diff/60001/content/browser/memory/... content/browser/memory/memory_coordinator_impl_unittest.cc:67: coordinator_->predicted_renderer_size_ = 10; Set parameters explicitly to make tests clear
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Looking good! https://codereview.chromium.org/2374343002/diff/80001/content/browser/memory/... File content/browser/memory/memory_coordinator_impl.cc (right): https://codereview.chromium.org/2374343002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:16: static const int kDefaultPredictedRendererSizeMB = 40; Can you comment where these come from (ie, the XXth percentile of the appropriate UMA stat). Also, it would be good to define appropriate values for Linux, ChromeOS, Windows, MacOS and Android while you're at it. https://codereview.chromium.org/2374343002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:72: new_renderers_back_to_throttled_ = 3; I'd prefer to have constants defined in an anonymous namespace above, and then referenced here: const int kDefaultNewRenderersUntilThrottled = 4; etc... https://codereview.chromium.org/2374343002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:74: monitoring_interval_ = base::TimeDelta::FromSeconds(5); Ditto. https://codereview.chromium.org/2374343002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:88: SetMemoryState(render_process_id, ToMojomMemoryState(current_state_)); Either SetMemoryState needs to be defensive and only allow valid transitions, or we need to check that the state is valid for the given process. For example, if this is a new foreground tab, we shouldn't ever be trying to suspend or throttle it. https://codereview.chromium.org/2374343002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:98: int new_renderers_until_critical = available / predicted_renderer_size_; expected_renderer_size_ is a better name maybe? (We're not doing active work to try to make a good prediction... but that would be a potential refinement.) https://codereview.chromium.org/2374343002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:158: // be suspended. These TODOs need to happen before this can land, IMO. Have a CanThrottle and a CanSuspend in the delegate? Or a more general CanBeInState? Then iterate over the renderers and only transition those that can actually be in the new state? (In other words, this CL should land after the delegate CL.) https://codereview.chromium.org/2374343002/diff/80001/content/browser/memory/... File content/browser/memory/memory_coordinator_impl.h (right): https://codereview.chromium.org/2374343002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.h:105: // The memory coordinator stays a same state at least this duration even when stays in* the* same state for* at least
https://codereview.chromium.org/2374343002/diff/80001/content/browser/memory/... File content/browser/memory/memory_coordinator_impl.cc (right): https://codereview.chromium.org/2374343002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:16: static const int kDefaultPredictedRendererSizeMB = 40; On 2016/10/10 23:53:01, chrisha (slow) wrote: > Can you comment where these come from (ie, the XXth percentile of the > appropriate UMA stat). Also, it would be good to define appropriate values for > Linux, ChromeOS, Windows, MacOS and Android while you're at it. Done. I was thinking that that's kind of information is a bit sensitive, but it seems not. Memory.Renderer.Large2 tells me that MacOS, Linux and ChromeOS have a similar median. https://codereview.chromium.org/2374343002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:72: new_renderers_back_to_throttled_ = 3; On 2016/10/10 23:53:02, chrisha (slow) wrote: > I'd prefer to have constants defined in an anonymous namespace above, and then > referenced here: > > const int kDefaultNewRenderersUntilThrottled = 4; > etc... Done. https://codereview.chromium.org/2374343002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:74: monitoring_interval_ = base::TimeDelta::FromSeconds(5); On 2016/10/10 23:53:02, chrisha (slow) wrote: > Ditto. Done. https://codereview.chromium.org/2374343002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:88: SetMemoryState(render_process_id, ToMojomMemoryState(current_state_)); On 2016/10/10 23:53:02, chrisha (slow) wrote: > Either SetMemoryState needs to be defensive and only allow valid transitions, or > we need to check that the state is valid for the given process. For example, if > this is a new foreground tab, we shouldn't ever be trying to suspend or throttle > it. I've added checks in SetMemoryState() to prevent invalid transitions in https://codereview.chromium.org/2399293002/ Should we avoid throttling a new foreground tab? I think throttling a foreground tab from the beginning makes sense. https://codereview.chromium.org/2374343002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:98: int new_renderers_until_critical = available / predicted_renderer_size_; On 2016/10/10 23:53:02, chrisha (slow) wrote: > expected_renderer_size_ is a better name maybe? > > (We're not doing active work to try to make a good prediction... but that would > be a potential refinement.) Yeah, changed to expected_renderer_count. https://codereview.chromium.org/2374343002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:158: // be suspended. On 2016/10/10 23:53:02, chrisha (slow) wrote: > These TODOs need to happen before this can land, IMO. Yes. I'll submit this after the delegate is landed. > > Have a CanThrottle and a CanSuspend in the delegate? Or a more general > CanBeInState? > Then iterate over the renderers and only transition those that can actually be > in the new state? Hmm, I'd prefer to put logic in content/ as much as possible. IIUC, memory coordinator can determine if a renderer can be throttled without information in chrome/browser so I think CanSuspend() is enough at this point. How about adding CanBeInState() in MemoryCoordinatorImpl? > > (In other words, this CL should land after the delegate CL.)
haraken@chromium.org changed reviewers: + hajimehoshi@chromium.org
The CQ bit was checked by bashi@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 delegate is landed (https://codereview.chromium.org/2399293002/). PTAL?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM on my side. https://codereview.chromium.org/2374343002/diff/140001/content/browser/memory... File content/browser/memory/memory_coordinator_impl.cc (right): https://codereview.chromium.org/2374343002/diff/140001/content/browser/memory... content/browser/memory/memory_coordinator_impl.cc:22: const int kDefaultPredictedRendererSizeMB = 120; Not related to this CL, do you know why the median size is so different between win and others? https://codereview.chromium.org/2374343002/diff/140001/content/browser/memory... File content/browser/memory/memory_coordinator_impl.h (right): https://codereview.chromium.org/2374343002/diff/140001/content/browser/memory... content/browser/memory/memory_coordinator_impl.h:91: // When in a NORMAL state and the potential number of new renderers drop below drops https://codereview.chromium.org/2374343002/diff/140001/content/browser/memory... content/browser/memory/memory_coordinator_impl.h:95: // drop below this level, the coordinator will transition to a SUSPENDED drops https://codereview.chromium.org/2374343002/diff/140001/content/browser/memory... content/browser/memory/memory_coordinator_impl.h:99: // renderers rise above this level, the coordinator will transition to a rises https://codereview.chromium.org/2374343002/diff/140001/content/browser/memory... content/browser/memory/memory_coordinator_impl.h:102: // When in a SUSPENDED state and the potential number of new renderers rise rises https://codereview.chromium.org/2374343002/diff/140001/content/browser/memory... content/browser/memory/memory_coordinator_impl.h:105: // The memory coordinator stays a same state at least this duration even when stays in the same state https://codereview.chromium.org/2374343002/diff/140001/content/browser/memory... content/browser/memory/memory_coordinator_impl.h:109: // The interval between checking the amount of free memory. between => of ?
https://codereview.chromium.org/2374343002/diff/140001/content/browser/memory... File content/browser/memory/memory_coordinator_impl.cc (right): https://codereview.chromium.org/2374343002/diff/140001/content/browser/memory... content/browser/memory/memory_coordinator_impl.cc:22: const int kDefaultPredictedRendererSizeMB = 120; On 2016/10/14 01:27:59, haraken wrote: > > Not related to this CL, do you know why the median size is so different between > win and others? Unfortunately no. The UMA comes from WorkingSetKBytes.priv [1] and it could be different values among platforms (as the comment says). However, 50MB is a large difference. We might want to investigate it. [1] https://cs.chromium.org/chromium/src/base/process/process_metrics.h?sq=packag... https://codereview.chromium.org/2374343002/diff/140001/content/browser/memory... File content/browser/memory/memory_coordinator_impl.h (right): https://codereview.chromium.org/2374343002/diff/140001/content/browser/memory... content/browser/memory/memory_coordinator_impl.h:91: // When in a NORMAL state and the potential number of new renderers drop below On 2016/10/14 01:27:59, haraken wrote: > > drops Done. https://codereview.chromium.org/2374343002/diff/140001/content/browser/memory... content/browser/memory/memory_coordinator_impl.h:95: // drop below this level, the coordinator will transition to a SUSPENDED On 2016/10/14 01:27:59, haraken wrote: > > drops Done. https://codereview.chromium.org/2374343002/diff/140001/content/browser/memory... content/browser/memory/memory_coordinator_impl.h:99: // renderers rise above this level, the coordinator will transition to a On 2016/10/14 01:27:59, haraken wrote: > > rises Done. https://codereview.chromium.org/2374343002/diff/140001/content/browser/memory... content/browser/memory/memory_coordinator_impl.h:102: // When in a SUSPENDED state and the potential number of new renderers rise On 2016/10/14 01:27:59, haraken wrote: > > rises Done. https://codereview.chromium.org/2374343002/diff/140001/content/browser/memory... content/browser/memory/memory_coordinator_impl.h:105: // The memory coordinator stays a same state at least this duration even when On 2016/10/14 01:27:59, haraken wrote: > > stays in the same state Done. https://codereview.chromium.org/2374343002/diff/140001/content/browser/memory... content/browser/memory/memory_coordinator_impl.h:109: // The interval between checking the amount of free memory. On 2016/10/14 01:27:59, haraken wrote: > > between => of ? Done.
lgtm with nits! https://codereview.chromium.org/2374343002/diff/80001/content/browser/memory/... File content/browser/memory/memory_coordinator_impl.cc (right): https://codereview.chromium.org/2374343002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:88: SetMemoryState(render_process_id, ToMojomMemoryState(current_state_)); On 2016/10/11 02:07:33, bashi1 wrote: > On 2016/10/10 23:53:02, chrisha (slow) wrote: > > Either SetMemoryState needs to be defensive and only allow valid transitions, > or > > we need to check that the state is valid for the given process. For example, > if > > this is a new foreground tab, we shouldn't ever be trying to suspend or > throttle > > it. > > I've added checks in SetMemoryState() to prevent invalid transitions in > https://codereview.chromium.org/2399293002/ > > Should we avoid throttling a new foreground tab? I think throttling a foreground > tab from the beginning makes sense. I'm not sure what the right answer is. Under extreme pressure we should allow throttling a foreground page, but I'd say we should only do that after having purged + suspended all pages, and maybe even only after discarding pages and still being under pressure? We want to make sure the foreground page always has the best experience possible. For now I'd simply say not to throttle them, and we can explore that as a refinement later. https://codereview.chromium.org/2374343002/diff/160001/content/browser/memory... File content/browser/memory/memory_coordinator_impl.cc (right): https://codereview.chromium.org/2374343002/diff/160001/content/browser/memory... content/browser/memory/memory_coordinator_impl.cc:18: const int kDefaultPredictedRendererSizeMB = 40; Expected rather than predicted? We're not trying to make any active predictions here... https://codereview.chromium.org/2374343002/diff/160001/content/browser/memory... content/browser/memory/memory_coordinator_impl.cc:22: const int kDefaultPredictedRendererSizeMB = 120; This is mac and linux? A comment as to the platform this is referring to would be useful. https://codereview.chromium.org/2374343002/diff/160001/content/browser/memory... File content/browser/memory/memory_coordinator_impl.h (right): https://codereview.chromium.org/2374343002/diff/160001/content/browser/memory... content/browser/memory/memory_coordinator_impl.h:33: // * Covert N to a memory state using some thresholds/hysteresis for each state. Convert* https://codereview.chromium.org/2374343002/diff/160001/content/browser/memory... content/browser/memory/memory_coordinator_impl.h:66: // Notifies a state change to clients. ubernit for a little more clarity: ... to in-process clients. https://codereview.chromium.org/2374343002/diff/160001/content/browser/memory... content/browser/memory/memory_coordinator_impl.h:85: // Parameters to determine the global state. Parameters to control the heuristic.
The CQ bit was checked by bashi@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...
bashi@chromium.org changed reviewers: + jam@chromium.org, sky@chromium.org
jam@: could you review content/browser ? sky@: could you review content/test ? https://codereview.chromium.org/2374343002/diff/80001/content/browser/memory/... File content/browser/memory/memory_coordinator_impl.cc (right): https://codereview.chromium.org/2374343002/diff/80001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:88: SetMemoryState(render_process_id, ToMojomMemoryState(current_state_)); On 2016/10/14 20:06:05, chrisha (slow) wrote: > On 2016/10/11 02:07:33, bashi1 wrote: > > On 2016/10/10 23:53:02, chrisha (slow) wrote: > > > Either SetMemoryState needs to be defensive and only allow valid > transitions, > > or > > > we need to check that the state is valid for the given process. For example, > > if > > > this is a new foreground tab, we shouldn't ever be trying to suspend or > > throttle > > > it. > > > > I've added checks in SetMemoryState() to prevent invalid transitions in > > https://codereview.chromium.org/2399293002/ > > > > Should we avoid throttling a new foreground tab? I think throttling a > foreground > > tab from the beginning makes sense. > > I'm not sure what the right answer is. Under extreme pressure we should allow > throttling a foreground page, but I'd say we should only do that after having > purged + suspended all pages, and maybe even only after discarding pages and > still being under pressure? > > We want to make sure the foreground page always has the best experience > possible. For now I'd simply say not to throttle them, and we can explore that > as a refinement later. I see. Added CanThrottleRenderer() which checks whether a tab is backgrounded. https://codereview.chromium.org/2374343002/diff/160001/content/browser/memory... File content/browser/memory/memory_coordinator_impl.cc (right): https://codereview.chromium.org/2374343002/diff/160001/content/browser/memory... content/browser/memory/memory_coordinator_impl.cc:18: const int kDefaultPredictedRendererSizeMB = 40; On 2016/10/14 20:06:05, chrisha (slow) wrote: > Expected rather than predicted? We're not trying to make any active predictions > here... Done. https://codereview.chromium.org/2374343002/diff/160001/content/browser/memory... content/browser/memory/memory_coordinator_impl.cc:22: const int kDefaultPredictedRendererSizeMB = 120; On 2016/10/14 20:06:05, chrisha (slow) wrote: > This is mac and linux? A comment as to the platform this is referring to would > be useful. Done. https://codereview.chromium.org/2374343002/diff/160001/content/browser/memory... File content/browser/memory/memory_coordinator_impl.h (right): https://codereview.chromium.org/2374343002/diff/160001/content/browser/memory... content/browser/memory/memory_coordinator_impl.h:66: // Notifies a state change to clients. On 2016/10/14 20:06:05, chrisha (slow) wrote: > ubernit for a little more clarity: > > ... to in-process clients. Done. https://codereview.chromium.org/2374343002/diff/160001/content/browser/memory... content/browser/memory/memory_coordinator_impl.h:85: // Parameters to determine the global state. On 2016/10/14 20:06:05, chrisha (slow) wrote: > Parameters to control the heuristic. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
lgtm
The CQ bit was checked by bashi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, chrisha@chromium.org Link to the patchset: https://codereview.chromium.org/2374343002/#ps180001 (title: "comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add MemoryCoordinatorImpl This CL adds actual implementation of memory coordinator. The idea is from chrisha@ and a basic strategy is: * There is only one global state. All processes share the same state. * The state level calculation is determined by the amount of free memory and the median of renderer size (platform-dependent). BUG=617492 ========== to ========== Add MemoryCoordinatorImpl This CL adds actual implementation of memory coordinator. The idea is from chrisha@ and a basic strategy is: * There is only one global state. All processes share the same state. * The state level calculation is determined by the amount of free memory and the median of renderer size (platform-dependent). BUG=617492 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Add MemoryCoordinatorImpl This CL adds actual implementation of memory coordinator. The idea is from chrisha@ and a basic strategy is: * There is only one global state. All processes share the same state. * The state level calculation is determined by the amount of free memory and the median of renderer size (platform-dependent). BUG=617492 ========== to ========== Add MemoryCoordinatorImpl This CL adds actual implementation of memory coordinator. The idea is from chrisha@ and a basic strategy is: * There is only one global state. All processes share the same state. * The state level calculation is determined by the amount of free memory and the median of renderer size (platform-dependent). BUG=617492 Committed: https://crrev.com/529cb14c66359213f1d19b0450ff4d73b25c0e10 Cr-Commit-Position: refs/heads/master@{#425786} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/529cb14c66359213f1d19b0450ff4d73b25c0e10 Cr-Commit-Position: refs/heads/master@{#425786} |
