Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2718963002:
Drop the global memory state from memory coordinator (Closed)
DescriptionDrop the global memory state from memory coordinator
Instaed of the global memory state, introduce a new internal
state for memory coordinator called MemoryCondition. See [1]
for the motivation.
[1] https://docs.google.com/document/d/1XZFywVyc60mJOP76BO--AXlhrgi73noIiD5dhS6LXzA/edit#
BUG=696844
Review-Url: https://codereview.chromium.org/2718963002
Cr-Commit-Position: refs/heads/master@{#454512}
Committed: https://chromium.googlesource.com/chromium/src/+/810232f5fc6a9c5f26ded564bc4546ce04362a78
Patch Set 1 #Patch Set 2 : tweak #
Total comments: 4
Patch Set 3 : tests and tweaks #Patch Set 4 : format #
Total comments: 21
Patch Set 5 : comments #Patch Set 6 : Observe #Patch Set 7 : Fix build #Patch Set 8 : Fix another build error #
Total comments: 14
Patch Set 9 : comments #Patch Set 10 : tweak #Patch Set 11 : Fix tests #
Total comments: 3
Patch Set 12 : comment #
Total comments: 4
Patch Set 13 : comments #Messages
Total messages: 48 (34 generated)
Description was changed from ========== WIP: Introduce an internal state for memory coordinator I named it "MemoryCondition" for now but we may change the name. BUG= ========== to ========== WIP: Drop the global memory state from memory coordinator Introduce an internal state instead of the global memory state. I named it "MemoryCondition" for now but we may change the name. BUG= ==========
https://codereview.chromium.org/2718963002/diff/20001/content/browser/memory/... File content/browser/memory/memory_coordinator_impl.cc (left): https://codereview.chromium.org/2718963002/diff/20001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:336: RecordStateChange(prev_state, next_state, Most of UMAs recorded here weren't helpful. We will remove them. https://codereview.chromium.org/2718963002/diff/20001/content/browser/memory/... File content/browser/memory/memory_coordinator_impl.cc (right): https://codereview.chromium.org/2718963002/diff/20001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:291: const NotificationDetails& details) { FYI: This is called when a user switches tabs. https://codereview.chromium.org/2718963002/diff/20001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:306: SetChildMemoryState(iter->first, new_state); This basically means: Foregrounded renderers: change to NORMAL if needed Backgrounded renderers: change to THROTTLED when free memory is low https://codereview.chromium.org/2718963002/diff/20001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:353: // processes and handle them one-by-one. I think we need to discuss what we should do here.
Description was changed from ========== WIP: Drop the global memory state from memory coordinator Introduce an internal state instead of the global memory state. I named it "MemoryCondition" for now but we may change the name. BUG= ========== to ========== Drop the global memory state from memory coordinator Instaed of the global memory state, introduce a new internal state for memory coordinator called MemoryCondition. See [1] for the motivation. [1] https://docs.google.com/document/d/1XZFywVyc60mJOP76BO--AXlhrgi73noIiD5dhS6LX... BUG=696844 ==========
bashi@chromium.org changed reviewers: + chrisha@chromium.org, haraken@chromium.org
haraken@, chrisha@: Updated tests and renamed bunch of things. Could you take a look? Sorry for asking a huge size of review :( It's a bit difficult to create small CLs for this change... If this is too big to review, I'll try to separate into small ones. https://codereview.chromium.org/2718963002/diff/60001/content/browser/memory/... File content/browser/memory/memory_condition_observer.h (right): https://codereview.chromium.org/2718963002/diff/60001/content/browser/memory/... content/browser/memory/memory_condition_observer.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. FYI: I should have passed higher similarity to `git cl upload` but this is basically a renaming of MemoryStateUpdater.
Mostly looks good. https://codereview.chromium.org/2718963002/diff/60001/content/browser/memory/... File content/browser/memory/memory_coordinator_impl.cc (left): https://codereview.chromium.org/2718963002/diff/60001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:130: last_state_change_ = base::TimeTicks::Now(); Why can we remove this? https://codereview.chromium.org/2718963002/diff/60001/content/browser/memory/... File content/browser/memory/memory_coordinator_impl.cc (right): https://codereview.chromium.org/2718963002/diff/60001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:237: if (!iter->second.is_visible && Add a comment about what this condition is doing. https://codereview.chromium.org/2718963002/diff/60001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:276: if (next_condition == MemoryCondition::NORMAL) { Add more comments on these branches. https://codereview.chromium.org/2718963002/diff/60001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:280: if (prev_condition == MemoryCondition::NORMAL && else if https://codereview.chromium.org/2718963002/diff/60001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:287: } Are you intentionally not calling UpdateCurrentMemoryState? https://codereview.chromium.org/2718963002/diff/60001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:290: if (prev_condition != MemoryCondition::CRITICAL && else if https://codereview.chromium.org/2718963002/diff/60001/content/browser/memory/... File content/browser/memory/memory_coordinator_impl.h (right): https://codereview.chromium.org/2718963002/diff/60001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.h:84: // Temporariy sets memory state of the browser process for testing. Temporarily https://codereview.chromium.org/2718963002/diff/60001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.h:199: MemoryState current_state_ = MemoryState::NORMAL; current_state_ => browser_memory_state_ ? https://codereview.chromium.org/2718963002/diff/60001/content/browser/memory/... File content/browser/memory/memory_monitor_android.cc (right): https://codereview.chromium.org/2718963002/diff/60001/content/browser/memory/... content/browser/memory/memory_monitor_android.cc:79: coordinator->ForceSetMemoryCondition(MemoryCondition::WARNING, Shouldn't this be CRITICAL? https://codereview.chromium.org/2718963002/diff/60001/content/browser/memory/... content/browser/memory/memory_monitor_android.cc:82: coordinator->ForceSetMemoryCondition(MemoryCondition::CRITICAL, WARNING?
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 CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
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 CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
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...
https://codereview.chromium.org/2718963002/diff/60001/content/browser/memory/... File content/browser/memory/memory_coordinator_impl.cc (left): https://codereview.chromium.org/2718963002/diff/60001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:130: last_state_change_ = base::TimeTicks::Now(); On 2017/02/28 11:33:45, haraken wrote: > > Why can we remove this? I slightly changed logic here. Instead of initializing |last_state_change_| here, I added last_state_change_.is_null() check in UpdateCurrentMemoryState(). This allows us to update memory condition from the beginning. https://codereview.chromium.org/2718963002/diff/60001/content/browser/memory/... File content/browser/memory/memory_coordinator_impl.cc (right): https://codereview.chromium.org/2718963002/diff/60001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:237: if (!iter->second.is_visible && On 2017/02/28 11:33:45, haraken wrote: > > Add a comment about what this condition is doing. Added comments. Also changed state calculation to make it consistent with UpdateConditionIfNeeded(). https://codereview.chromium.org/2718963002/diff/60001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:276: if (next_condition == MemoryCondition::NORMAL) { On 2017/02/28 11:33:45, haraken wrote: > > Add more comments on these branches. Done. https://codereview.chromium.org/2718963002/diff/60001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:280: if (prev_condition == MemoryCondition::NORMAL && On 2017/02/28 11:33:45, haraken wrote: > > else if Done. https://codereview.chromium.org/2718963002/diff/60001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:287: } On 2017/02/28 11:33:45, haraken wrote: > > Are you intentionally not calling UpdateCurrentMemoryState? Yes (I wanted to make this CL simpler), but agreed, we would want to do: - UpdateCurrentMemoryState(NORMAL) - Set NORMAL to foreground renderers Changed conditions/actions. https://codereview.chromium.org/2718963002/diff/60001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:290: if (prev_condition != MemoryCondition::CRITICAL && On 2017/02/28 11:33:45, haraken wrote: > > else if Done. https://codereview.chromium.org/2718963002/diff/60001/content/browser/memory/... File content/browser/memory/memory_coordinator_impl.h (right): https://codereview.chromium.org/2718963002/diff/60001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.h:84: // Temporariy sets memory state of the browser process for testing. On 2017/02/28 11:33:45, haraken wrote: > > Temporarily Done. https://codereview.chromium.org/2718963002/diff/60001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.h:199: MemoryState current_state_ = MemoryState::NORMAL; On 2017/02/28 11:33:45, haraken wrote: > > current_state_ => browser_memory_state_ ? Yes. Done. https://codereview.chromium.org/2718963002/diff/60001/content/browser/memory/... File content/browser/memory/memory_monitor_android.cc (right): https://codereview.chromium.org/2718963002/diff/60001/content/browser/memory/... content/browser/memory/memory_monitor_android.cc:79: coordinator->ForceSetMemoryCondition(MemoryCondition::WARNING, On 2017/02/28 11:33:45, haraken wrote: > > Shouldn't this be CRITICAL? Oops. Done. Thanks! https://codereview.chromium.org/2718963002/diff/60001/content/browser/memory/... content/browser/memory/memory_monitor_android.cc:82: coordinator->ForceSetMemoryCondition(MemoryCondition::CRITICAL, On 2017/02/28 11:33:45, haraken wrote: > > WARNING? Done.
Mostly looks good. https://codereview.chromium.org/2718963002/diff/140001/content/browser/memory... File content/browser/memory/memory_condition_observer.h (right): https://codereview.chromium.org/2718963002/diff/140001/content/browser/memory... content/browser/memory/memory_condition_observer.h:20: // be in a critical state". Call this N. Drop "s. Compute the number of... https://codereview.chromium.org/2718963002/diff/140001/content/browser/memory... content/browser/memory/memory_condition_observer.h:38: MemoryCondition CalculateNextCondition(); Can this be private? https://codereview.chromium.org/2718963002/diff/140001/content/browser/memory... File content/browser/memory/memory_coordinator_impl.cc (right): https://codereview.chromium.org/2718963002/diff/140001/content/browser/memory... content/browser/memory/memory_coordinator_impl.cc:208: base::MemoryState MemoryCoordinatorImpl::GetBrowserMemoryState() const { Can we remove this method in a follow-up? It looks confusing to have both GetBrowserMemoryState and GetCurrentMemoryState. https://codereview.chromium.org/2718963002/diff/140001/content/browser/memory... content/browser/memory/memory_coordinator_impl.cc:299: NotifyStateToChildren(MemoryState::NORMAL); We should NOT call NotifyStateToChildren since we're calling SetChildMemoryState below, right? https://codereview.chromium.org/2718963002/diff/140001/content/browser/memory... content/browser/memory/memory_coordinator_impl.cc:314: void MemoryCoordinatorImpl::UpdateBrowserMemoryState( I'd rename this to NotifyStateToBrowser to make it more consistent with NotifyStateToChildren. https://codereview.chromium.org/2718963002/diff/140001/content/browser/memory... content/browser/memory/memory_coordinator_impl.cc:318: (now - last_state_change_ < minimum_state_transition_period_)) We don't have this "delaying" logic when notifying child processes. Is it intended? https://codereview.chromium.org/2718963002/diff/140001/content/browser/memory... content/browser/memory/memory_coordinator_impl.cc:369: // Populate the global state as an initial state of a newly created process. global state => memory state
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 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...
https://codereview.chromium.org/2718963002/diff/140001/content/browser/memory... File content/browser/memory/memory_condition_observer.h (right): https://codereview.chromium.org/2718963002/diff/140001/content/browser/memory... content/browser/memory/memory_condition_observer.h:20: // be in a critical state". Call this N. On 2017/03/02 04:36:23, haraken wrote: > > Drop "s. Compute the number of... Done. https://codereview.chromium.org/2718963002/diff/140001/content/browser/memory... content/browser/memory/memory_condition_observer.h:38: MemoryCondition CalculateNextCondition(); On 2017/03/02 04:36:23, haraken wrote: > > Can this be private? Yes. I intended to make this private but I forgot to do that :( Done. https://codereview.chromium.org/2718963002/diff/140001/content/browser/memory... File content/browser/memory/memory_coordinator_impl.cc (right): https://codereview.chromium.org/2718963002/diff/140001/content/browser/memory... content/browser/memory/memory_coordinator_impl.cc:208: base::MemoryState MemoryCoordinatorImpl::GetBrowserMemoryState() const { On 2017/03/02 04:36:23, haraken wrote: > > Can we remove this method in a follow-up? It looks confusing to have both > GetBrowserMemoryState and GetCurrentMemoryState. Acknowledged. https://codereview.chromium.org/2718963002/diff/140001/content/browser/memory... content/browser/memory/memory_coordinator_impl.cc:299: NotifyStateToChildren(MemoryState::NORMAL); On 2017/03/02 04:36:23, haraken wrote: > > We should NOT call NotifyStateToChildren since we're calling SetChildMemoryState > below, right? Oops, I meant UpdateBrowserMemoryState(). Fixed. I'll add more tests in follow-up CLs once we decide what we should do here... https://codereview.chromium.org/2718963002/diff/140001/content/browser/memory... content/browser/memory/memory_coordinator_impl.cc:314: void MemoryCoordinatorImpl::UpdateBrowserMemoryState( On 2017/03/02 04:36:23, haraken wrote: > > I'd rename this to NotifyStateToBrowser to make it more consistent with > NotifyStateToChildren. Done. https://codereview.chromium.org/2718963002/diff/140001/content/browser/memory... content/browser/memory/memory_coordinator_impl.cc:318: (now - last_state_change_ < minimum_state_transition_period_)) On 2017/03/02 04:36:23, haraken wrote: > > We don't have this "delaying" logic when notifying child processes. Is it > intended? Yes. The tricky part of child processes is that we want to change state immediately when visibility is changed. When a tab is foregrounded, we should set NORMAL state if it was in THROTTLED state. Maybe we want to have a period to suppress state change for child processes but I'd like to think about it later. The basic rule of OnMemoryStateChange() is that it shouldn't touch previous memory allocations so I think it's reasonable not to have delaying logic for now. https://codereview.chromium.org/2718963002/diff/140001/content/browser/memory... content/browser/memory/memory_coordinator_impl.cc:369: // Populate the global state as an initial state of a newly created process. On 2017/03/02 04:36:23, haraken wrote: > > global state => memory state Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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...
https://codereview.chromium.org/2718963002/diff/190001/content/browser/memory... File content/browser/memory/memory_coordinator_impl.cc (right): https://codereview.chromium.org/2718963002/diff/190001/content/browser/memory... content/browser/memory/memory_coordinator_impl.cc:397: if (memory_state == browser_memory_state_) unittests catched the bug. I should have added this line.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
haraken@: friendly ping :)
LGTM https://codereview.chromium.org/2718963002/diff/190001/content/browser/memory... File content/browser/memory/memory_coordinator_impl.cc (right): https://codereview.chromium.org/2718963002/diff/190001/content/browser/memory... content/browser/memory/memory_coordinator_impl.cc:359: // foregrounded. You don't need to fix this in this CL but this assumption may not be correct. e.g., a case where users restore many tabs at the same time. Either way, it looks better to share the following code with MemoryCoordinatorImpl::Observe.
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: + kinuko@chromium.org
kinuko@: could you rs-review for content/? https://codereview.chromium.org/2718963002/diff/190001/content/browser/memory... File content/browser/memory/memory_coordinator_impl.cc (right): https://codereview.chromium.org/2718963002/diff/190001/content/browser/memory... content/browser/memory/memory_coordinator_impl.cc:359: // foregrounded. On 2017/03/03 02:08:00, haraken wrote: > > You don't need to fix this in this CL but this assumption may not be correct. > e.g., a case where users restore many tabs at the same time. > > Either way, it looks better to share the following code with > MemoryCoordinatorImpl::Observe. Agreed. Added TODO.
lgtm (% nits that caught my eyes) https://codereview.chromium.org/2718963002/diff/210001/content/browser/memory... File content/browser/memory/memory_coordinator_impl.cc (right): https://codereview.chromium.org/2718963002/diff/210001/content/browser/memory... content/browser/memory/memory_coordinator_impl.cc:217: base::MemoryState memory_state) { nit: please consistently use MemoryState without base::, it could be confusing https://codereview.chromium.org/2718963002/diff/210001/content/browser/memory... File content/browser/memory/memory_coordinator_impl.h (right): https://codereview.chromium.org/2718963002/diff/210001/content/browser/memory... content/browser/memory/memory_coordinator_impl.h:184: void NotifyStateToBrowser(MemoryState state); UpdateBrowserStateAndNotifyStateToClients? The phrase 'notify state to browser' in the browser-side code puzzled me a bit, we're in browser process and whom we're notifying seem to be clients...
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 CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thank you for review! https://codereview.chromium.org/2718963002/diff/210001/content/browser/memory... File content/browser/memory/memory_coordinator_impl.cc (right): https://codereview.chromium.org/2718963002/diff/210001/content/browser/memory... content/browser/memory/memory_coordinator_impl.cc:217: base::MemoryState memory_state) { On 2017/03/03 03:06:26, kinuko wrote: > nit: please consistently use MemoryState without base::, it could be confusing Done. https://codereview.chromium.org/2718963002/diff/210001/content/browser/memory... File content/browser/memory/memory_coordinator_impl.h (right): https://codereview.chromium.org/2718963002/diff/210001/content/browser/memory... content/browser/memory/memory_coordinator_impl.h:184: void NotifyStateToBrowser(MemoryState state); On 2017/03/03 03:06:26, kinuko wrote: > UpdateBrowserStateAndNotifyStateToClients? > > The phrase 'notify state to browser' in the browser-side code puzzled me a bit, > we're in browser process and whom we're notifying seem to be clients... Done.
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, kinuko@chromium.org Link to the patchset: https://codereview.chromium.org/2718963002/#ps230001 (title: "comments")
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": 230001, "attempt_start_ts": 1488515585551730,
"parent_rev": "c8c1429588c2190eb5562d57c0e0ba3b4cbe6f8d", "commit_rev":
"810232f5fc6a9c5f26ded564bc4546ce04362a78"}
Message was sent while issue was closed.
Description was changed from ========== Drop the global memory state from memory coordinator Instaed of the global memory state, introduce a new internal state for memory coordinator called MemoryCondition. See [1] for the motivation. [1] https://docs.google.com/document/d/1XZFywVyc60mJOP76BO--AXlhrgi73noIiD5dhS6LX... BUG=696844 ========== to ========== Drop the global memory state from memory coordinator Instaed of the global memory state, introduce a new internal state for memory coordinator called MemoryCondition. See [1] for the motivation. [1] https://docs.google.com/document/d/1XZFywVyc60mJOP76BO--AXlhrgi73noIiD5dhS6LX... BUG=696844 Review-Url: https://codereview.chromium.org/2718963002 Cr-Commit-Position: refs/heads/master@{#454512} Committed: https://chromium.googlesource.com/chromium/src/+/810232f5fc6a9c5f26ded564bc45... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:230001) as https://chromium.googlesource.com/chromium/src/+/810232f5fc6a9c5f26ded564bc45... |
