Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2595813002:
Disable PurgeAndSuspend when MemoryCoordinator is enabled. (Closed)
DescriptionDisable PurgeAndSuspend when MemoryCoordinator is enabled.
PurgeAndSuspend depends on OnMemoryStateChange(). However,
always suspending renderer in OnMemoryStateChange(), it breaks
many things. So only enabling such suspend / resume feature when
only PurgeAndSuspend is enabled.
BUG=675735, 675811
Committed: https://crrev.com/ff1e6242e014db17e8bb736741595705c81dc774
Cr-Commit-Position: refs/heads/master@{#440340}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Fixed comment. #Messages
Total messages: 26 (14 generated)
The CQ bit was checked by tasak@google.com 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...
tasak@google.com changed reviewers: + avi@chromium.org, bashi@chromium.org, haraken@chromium.org
Would you review this CL?
LGTM https://codereview.chromium.org/2595813002/diff/1/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2595813002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1783: // MemoryCoordinator finch experiment when MemoryCoordinator is enabled. Just say: TODO(bashi): Enable the tab suspension when MemoryCoordinator is enabled. https://codereview.chromium.org/2595813002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1900: // MemoryCoordinator finch experiment when MemoryCoordinator is enabled. Ditto. https://codereview.chromium.org/2595813002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:2303: // When only PurgeAndSuspend is enabled, suspend blink scheduler. Ditto. https://codereview.chromium.org/2595813002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:2311: // When only PurgeAndSuspend is enabled, resume blink scheduler. Ditto.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with one question. Thank you for the fix. https://codereview.chromium.org/2595813002/diff/1/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2595813002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1784: if (base::FeatureList::IsEnabled(features::kMemoryCoordinator)) Why do we need this guard? Isn't having a guard in SuspendRenderer() enough?
Thank you for review. https://codereview.chromium.org/2595813002/diff/1/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2595813002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1784: if (base::FeatureList::IsEnabled(features::kMemoryCoordinator)) On 2016/12/21 08:44:02, bashi1 wrote: > Why do we need this guard? Isn't having a guard in SuspendRenderer() enough? To avoid invoking record_purge_suspend_metric_closure_. If we invoke this, PurgeAndSuspend finch experiment has wrong data (i.e. PurgeAndSuspend is enabled but no purge / suspend renderer is invoked). This means, PurgeAndSuspend finch experiment runs with 25% control and 25% experiment.
https://codereview.chromium.org/2595813002/diff/1/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2595813002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1784: if (base::FeatureList::IsEnabled(features::kMemoryCoordinator)) On 2016/12/21 08:49:31, tasak wrote: > On 2016/12/21 08:44:02, bashi1 wrote: > > Why do we need this guard? Isn't having a guard in SuspendRenderer() enough? > > To avoid invoking record_purge_suspend_metric_closure_. > If we invoke this, PurgeAndSuspend finch experiment has wrong data (i.e. > PurgeAndSuspend is enabled but no purge / suspend renderer is invoked). > > This means, PurgeAndSuspend finch experiment runs with 25% control and 25% > experiment. How about calling PostDelayedTask() in SuspendRenderer then?
https://codereview.chromium.org/2595813002/diff/1/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2595813002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1784: if (base::FeatureList::IsEnabled(features::kMemoryCoordinator)) On 2016/12/21 08:55:32, bashi1 wrote: > On 2016/12/21 08:49:31, tasak wrote: > > On 2016/12/21 08:44:02, bashi1 wrote: > > > Why do we need this guard? Isn't having a guard in SuspendRenderer() enough? > > > > To avoid invoking record_purge_suspend_metric_closure_. > > If we invoke this, PurgeAndSuspend finch experiment has wrong data (i.e. > > PurgeAndSuspend is enabled but no purge / suspend renderer is invoked). > > > > This means, PurgeAndSuspend finch experiment runs with 25% control and 25% > > experiment. > > How about calling PostDelayedTask() in SuspendRenderer then? To enable A/B testing, the PostDelayedTask is also needed to invoke when PurgeAndSuspend is disabled (=control of PurgeAndSuspend finch experiment).
https://codereview.chromium.org/2595813002/diff/1/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2595813002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1784: if (base::FeatureList::IsEnabled(features::kMemoryCoordinator)) On 2016/12/21 08:58:28, tasak wrote: > On 2016/12/21 08:55:32, bashi1 wrote: > > On 2016/12/21 08:49:31, tasak wrote: > > > On 2016/12/21 08:44:02, bashi1 wrote: > > > > Why do we need this guard? Isn't having a guard in SuspendRenderer() > enough? > > > > > > To avoid invoking record_purge_suspend_metric_closure_. > > > If we invoke this, PurgeAndSuspend finch experiment has wrong data (i.e. > > > PurgeAndSuspend is enabled but no purge / suspend renderer is invoked). > > > > > > This means, PurgeAndSuspend finch experiment runs with 25% control and 25% > > > experiment. > > > > How about calling PostDelayedTask() in SuspendRenderer then? > > To enable A/B testing, the PostDelayedTask is also needed to invoke when > PurgeAndSuspend is disabled (=control of PurgeAndSuspend finch experiment). OK
The CQ bit was checked by tasak@google.com 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.
https://codereview.chromium.org/2595813002/diff/1/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2595813002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1783: // MemoryCoordinator finch experiment when MemoryCoordinator is enabled. On 2016/12/21 04:03:23, haraken wrote: > > Just say: > > TODO(bashi): Enable the tab suspension when MemoryCoordinator is enabled. Done. https://codereview.chromium.org/2595813002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1900: // MemoryCoordinator finch experiment when MemoryCoordinator is enabled. On 2016/12/21 04:03:23, haraken wrote: > > Ditto. Done. https://codereview.chromium.org/2595813002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:2303: // When only PurgeAndSuspend is enabled, suspend blink scheduler. On 2016/12/21 04:03:23, haraken wrote: > > Ditto. Done. https://codereview.chromium.org/2595813002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:2311: // When only PurgeAndSuspend is enabled, resume blink scheduler. On 2016/12/21 04:03:23, haraken wrote: > > Ditto. Done.
The CQ bit was checked by tasak@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, avi@chromium.org, bashi@chromium.org Link to the patchset: https://codereview.chromium.org/2595813002/#ps20001 (title: "Fixed comment.")
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": 20001, "attempt_start_ts": 1482382200224100,
"parent_rev": "762317eb268d7aa5b6c460f03dc789bdcea67660", "commit_rev":
"428d526f5e1e75663ce6f17b8bdc955d3ece4fee"}
Message was sent while issue was closed.
Description was changed from ========== Disable PurgeAndSuspend when MemoryCoordinator is enabled. PurgeAndSuspend depends on OnMemoryStateChange(). However, always suspending renderer in OnMemoryStateChange(), it breaks many things. So only enabling such suspend / resume feature when only PurgeAndSuspend is enabled. BUG=675735, 675811 ========== to ========== Disable PurgeAndSuspend when MemoryCoordinator is enabled. PurgeAndSuspend depends on OnMemoryStateChange(). However, always suspending renderer in OnMemoryStateChange(), it breaks many things. So only enabling such suspend / resume feature when only PurgeAndSuspend is enabled. BUG=675735, 675811 Review-Url: https://codereview.chromium.org/2595813002 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Disable PurgeAndSuspend when MemoryCoordinator is enabled. PurgeAndSuspend depends on OnMemoryStateChange(). However, always suspending renderer in OnMemoryStateChange(), it breaks many things. So only enabling such suspend / resume feature when only PurgeAndSuspend is enabled. BUG=675735, 675811 Review-Url: https://codereview.chromium.org/2595813002 ========== to ========== Disable PurgeAndSuspend when MemoryCoordinator is enabled. PurgeAndSuspend depends on OnMemoryStateChange(). However, always suspending renderer in OnMemoryStateChange(), it breaks many things. So only enabling such suspend / resume feature when only PurgeAndSuspend is enabled. BUG=675735, 675811 Committed: https://crrev.com/ff1e6242e014db17e8bb736741595705c81dc774 Cr-Commit-Position: refs/heads/master@{#440340} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ff1e6242e014db17e8bb736741595705c81dc774 Cr-Commit-Position: refs/heads/master@{#440340} |
