Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2577313002:
memory coordinator: Resume suspended renderer on NORMAL state change (Closed)
Descriptionmemory coordinator: Resume suspended renderer on NORMAL state change
Before this CL we don't resume suspended renderer when the suspension was
initiated by memory coordinator (not purge+suspend experiment). We
should resume suspended renderer when the memory state becomes NORMAL.
BUG=674784
Committed: https://crrev.com/6ba5d880deafed03fe670da4461db9b7ef6d5645
Cr-Commit-Position: refs/heads/master@{#439394}
Patch Set 1 #
Total comments: 4
Patch Set 2 : comments #Patch Set 3 : Remove DCHECK #
Total comments: 3
Patch Set 4 : Don't use is_renderer_suspended_ #
Total comments: 2
Patch Set 5 : rebase #Messages
Total messages: 37 (20 generated)
The CQ bit was checked by bashi@chromium.org to run a CQ dry run
bashi@chromium.org changed reviewers: + haraken@chromium.org, tasak@google.com
tasak@, does this look a correct fix?
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/2577313002/diff/1/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2577313002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:2298: case base::MemoryState::THROTTLED: Don't we need to call ResumeRendererIfNeeded() here as well?
LGTM
wez@chromium.org changed reviewers: + wez@chromium.org
https://codereview.chromium.org/2577313002/diff/1/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2577313002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:2315: is_renderer_suspended_ = true; nit: DCHECK(!is_renderer_suspended_) here?
https://codereview.chromium.org/2577313002/diff/1/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2577313002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:2298: case base::MemoryState::THROTTLED: On 2016/12/16 01:38:21, haraken wrote: > > Don't we need to call ResumeRendererIfNeeded() here as well? Yes. Done. https://codereview.chromium.org/2577313002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:2315: is_renderer_suspended_ = true; On 2016/12/16 01:57:45, Wez wrote: > nit: DCHECK(!is_renderer_suspended_) here? Done.
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.
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/2577313002/diff/40001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2577313002/diff/40001/content/renderer/render... content/renderer/render_thread_impl.cc:2315: if (is_renderer_suspended_) Chatted with tasak@ offline and he said we can't DCHECK here due to IPC lag. Changed to early return.
bashi@chromium.org changed reviewers: + avi@chromium.org
+avi@ for OWNERS review
https://codereview.chromium.org/2577313002/diff/40001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2577313002/diff/40001/content/renderer/render... content/renderer/render_thread_impl.cc:2315: if (is_renderer_suspended_) On 2016/12/16 07:50:39, bashi1 wrote: > Chatted with tasak@ offline and he said we can't DCHECK here due to IPC lag. > Changed to early return. I was trying to remove DCHECK related to is_renderer_suspended. c.f. https://codereview.chromium.org/2564833002/ I missed checking whether the patch was successfully committed or not... ... the patch removes is_renderer_suspended_, but this CL needs. I will remove only DCHECK in OnProcessResume and OnProcessPurgeAndSuspend.
https://codereview.chromium.org/2577313002/diff/40001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2577313002/diff/40001/content/renderer/render... content/renderer/render_thread_impl.cc:2315: if (is_renderer_suspended_) On 2016/12/16 07:55:15, tasak wrote: > On 2016/12/16 07:50:39, bashi1 wrote: > > Chatted with tasak@ offline and he said we can't DCHECK here due to IPC lag. > > Changed to early return. > > I was trying to remove DCHECK related to is_renderer_suspended. c.f. > https://codereview.chromium.org/2564833002/ > I missed checking whether the patch was successfully committed or not... > > ... the patch removes is_renderer_suspended_, but this CL needs. I will remove > only DCHECK in OnProcessResume and OnProcessPurgeAndSuspend. Thanks for the info. Then we should do early return in RendererSchedulerImpl::ResumeRenderer(). I'll wait for your CL to land.
On 2016/12/16 08:03:13, bashi1 wrote: > Thanks for the info. Then we should do early return in > RendererSchedulerImpl::ResumeRenderer(). I'll wait for your CL to land. When I updated the purge-and-suspend logic (making tab_manager to have purge-and-suspend state), I was advised to remove is_suspended_renderer. I think, memory coordinator / tab_manager knows suspended or not. So scheduler should follow requests from memory coordinator / tab manager.
lgtm
https://codereview.chromium.org/2577313002/diff/60001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2577313002/diff/60001/content/renderer/render... content/renderer/render_thread_impl.cc:1899: DCHECK(is_renderer_suspended_); Did you mean to remove |is_renderer_suspended_| entirely?
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...
Rebased. I was thinking that we need to do early return in RenderSchedulerImpl but it seems we don't. I'll submit this CL when dry-run passes. https://codereview.chromium.org/2577313002/diff/60001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2577313002/diff/60001/content/renderer/render... content/renderer/render_thread_impl.cc:1899: DCHECK(is_renderer_suspended_); On 2016/12/17 06:40:20, Wez wrote: > Did you mean to remove |is_renderer_suspended_| entirely? Yes. tasak@ removed it. https://codereview.chromium.org/2564833002/
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 bashi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, avi@chromium.org Link to the patchset: https://codereview.chromium.org/2577313002/#ps80001 (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": 80001, "attempt_start_ts": 1482112247594500,
"parent_rev": "f26c7f5d088c1a3189a84fd0a157898bbd527edb", "commit_rev":
"1983199ae8bbe581ac92cff0453c9a0c2f4bd123"}
Message was sent while issue was closed.
Description was changed from ========== memory coordinator: Resume suspended renderer on NORMAL state change Before this CL we don't resume suspended renderer when the suspension was initiated by memory coordinator (not purge+suspend experiment). We should resume suspended renderer when the memory state becomes NORMAL. BUG=674784 ========== to ========== memory coordinator: Resume suspended renderer on NORMAL state change Before this CL we don't resume suspended renderer when the suspension was initiated by memory coordinator (not purge+suspend experiment). We should resume suspended renderer when the memory state becomes NORMAL. BUG=674784 Review-Url: https://codereview.chromium.org/2577313002 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== memory coordinator: Resume suspended renderer on NORMAL state change Before this CL we don't resume suspended renderer when the suspension was initiated by memory coordinator (not purge+suspend experiment). We should resume suspended renderer when the memory state becomes NORMAL. BUG=674784 Review-Url: https://codereview.chromium.org/2577313002 ========== to ========== memory coordinator: Resume suspended renderer on NORMAL state change Before this CL we don't resume suspended renderer when the suspension was initiated by memory coordinator (not purge+suspend experiment). We should resume suspended renderer when the memory state becomes NORMAL. BUG=674784 Committed: https://crrev.com/6ba5d880deafed03fe670da4461db9b7ef6d5645 Cr-Commit-Position: refs/heads/master@{#439394} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6ba5d880deafed03fe670da4461db9b7ef6d5645 Cr-Commit-Position: refs/heads/master@{#439394}
Message was sent while issue was closed.
On 2016/12/19 01:03:15, bashi1 wrote: > Rebased. I was thinking that we need to do early return in RenderSchedulerImpl > but it seems we don't. I'll submit this CL when dry-run passes. It turned out I have to do early return :( I'll create a CL. [1:1:1219/161418.151951:FATAL:renderer_scheduler_impl.cc(568)] Check failed: Mai nThreadOnly().renderer_backgrounded. #0 0x7f805842bfbe base::debug::StackTrace::StackTrace() #1 0x7f805845054b logging::LogMessage::~LogMessage() #2 0x7f8052444159 blink::scheduler::RendererSchedulerImpl::ResumeRenderer() #3 0x7f8056277fed content::RenderThreadImpl::ResumeRenderer() #4 0x7f8056277e6a content::RenderThreadImpl::OnMemoryStateChange() #5 0x7f8058455299 base::ObserverListThreadSafe<>::NotifyWrapper() #6 0x7f805845546e _ZN4base8internal7InvokerINS0_9BindStateIMNS_22ObserverListThr eadSafeINS_23MemoryCoordinatorClientEEEFvPNS5_19ObserverListContextERKNS_8Callba ckIFvPS4_ELNS0_8CopyModeE1ELNS0_10RepeatModeE1EEEEJ13scoped_refptrIS5_ES7_SD_EEE FvvEE3RunEPNS0_13BindStateBaseE #7 0x7f805842cb9e base::debug::TaskAnnotator::RunTask() #8 0x7f805242bfd0 blink::scheduler::TaskQueueManager::ProcessTaskFromWorkQueue() #9 0x7f805242a009 blink::scheduler::TaskQueueManager::DoWork() #10 0x7f805242df85 _ZN4base8internal13FunctorTraitsIMN5blink9scheduler16TaskQueu eManagerEFvbEvE6InvokeIRKNS_7WeakPtrIS4_EEJRKbEEEvS6_OT_DpOT0_ #11 0x7f805842cb9e base::debug::TaskAnnotator::RunTask() #12 0x7f805845db9d base::MessageLoop::RunTask() #13 0x7f805845e536 base::MessageLoop::DoWork() #14 0x7f805845ff99 base::MessagePumpDefault::Run() #15 0x7f805845d8f5 base::MessageLoop::RunHandler() #16 0x7f8058491aac base::RunLoop::Run() #17 0x7f80562a29e8 content::RendererMain() #18 0x7f80563f4ee7 content::RunZygote() #19 0x7f80563f5618 content::RunNamedProcessTypeMain() #20 0x7f80563f6036 content::ContentMainRunnerImpl::Run() #21 0x7f80563f4a80 content::ContentMain() #22 0x7f8058f42671 ChromeMain |
