New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 828835 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Xoogler
Closed: Oct 2
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Task



Sign in to add a comment

Break up ThreadController with MessageLoop

Project Member Reported by kraynov@chromium.org, Apr 4 2018

Issue description

ThreadControllerImpl should talk to MessagePump directly.
 
Cc: alexclarke@chromium.org
Another reason to get rid of MessageLoop:
https://chromium-review.googlesource.com/1124692

When we call RunLoop::Quit() there's no way ThreadController may be aware of that immediatetely (before yielding DoWork).

So it forced us to use this nasty workaround:

void ThreadControllerImpl::DoWork() {
  ...
  for (int i = 0; i < batch_size; i++) {
    TakeTask();
    RunTask();
    DidRunTask();
    if (nesting_depth > 0)
      break;
  }
  ...
}

So every time we're in a nested RunLoop we can't do a batch work because we have to pessimistically expect MessageLoop::Quit() anytime.
Implementing ThreadController and RunLoop::Delagate ourselves without the MessageLoop will help us to handle this case better.
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 4

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/094fbcd1b2755bbfbba761759fb79c0ccf91449f

commit 094fbcd1b2755bbfbba761759fb79c0ccf91449f
Author: Greg Kraynov <kraynov@chromium.org>
Date: Wed Jul 04 09:47:26 2018

Explain why ThreadControllerImpl disables batching optimization.

When we're in a nested RunLoop we can't run tasks in a batch.
This comment update explains why.

Bug:  828835 
Change-Id: I8dcdf7f06d519f0e8087232b2d3c660e26510b60
Reviewed-on: https://chromium-review.googlesource.com/1124694
Reviewed-by: Alex Clarke <alexclarke@chromium.org>
Commit-Queue: Greg Kraynov <kraynov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572526}
[modify] https://crrev.com/094fbcd1b2755bbfbba761759fb79c0ccf91449f/third_party/blink/renderer/platform/scheduler/base/thread_controller_impl.cc

Improvement suggestions list:
- Remove the entire SetDefault/RestoreDefault dance with ThreadController.
- Consider adding wake-up cancellation support for MessagePump.
- Make ThreadController interface as lean as possible.
- Support IO and UI MessagePumps.
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 7

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3634ea3aad27be2946201a3ca51ef30b6927802e

commit 3634ea3aad27be2946201a3ca51ef30b6927802e
Author: Greg Kraynov <kraynov@chromium.org>
Date: Tue Aug 07 08:43:38 2018

Support MessagePump in SequenceManager perf tests.

SequenceManagerPerfTest now runs in both environments.

Bug:  828835 
Change-Id: Ib06dc1bbabd4fe972c428605247f6bf86b2c9825
Reviewed-on: https://chromium-review.googlesource.com/1163883
Reviewed-by: Alex Clarke <alexclarke@chromium.org>
Commit-Queue: Greg Kraynov <kraynov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581168}
[modify] https://crrev.com/3634ea3aad27be2946201a3ca51ef30b6927802e/base/task/sequence_manager/sequence_manager_perftest.cc
[modify] https://crrev.com/3634ea3aad27be2946201a3ca51ef30b6927802e/base/task/sequence_manager/thread_controller_with_message_pump_impl.cc
[modify] https://crrev.com/3634ea3aad27be2946201a3ca51ef30b6927802e/base/task/sequence_manager/thread_controller_with_message_pump_impl.h

Follow-up:
1. Can we cancel a previously scheduled wake-up with MessagePump?
2. MessagePump::DoDelayedWork(TimeTicks& next_delayed_work) doc says that setting next_delayed_work to null should indicate that the queue of delayed tasks is empty. So theoretically this can be used for timer cancellation. However, there's no such contract for ScheduleDelayedWork() method. I think we might want to harmonise APIs and contracts across various MessagePumps.
Project Member

Comment 8 by bugdroid1@chromium.org, Aug 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f20933cb98543ff525ae911aab0800ff7688cadb

commit f20933cb98543ff525ae911aab0800ff7688cadb
Author: Greg Kraynov <kraynov@chromium.org>
Date: Thu Aug 09 10:19:43 2018

Optimise ThreadControllerWithMessagePumpImpl wake-ups.

SetNextDelayedDoWork() now disregarded in most cases because DoWork()
can schedule delayed wake-ups.
ScheduleWork() now directly delegates to MessagePump because it's
cheaper than testing that we're inside of DoWork().

Bug:  828835 
Change-Id: I8417607d73f695099b37bc1066ccdd317b6e190a
Reviewed-on: https://chromium-review.googlesource.com/1167508
Reviewed-by: Alex Clarke <alexclarke@chromium.org>
Commit-Queue: Greg Kraynov <kraynov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581851}
[modify] https://crrev.com/f20933cb98543ff525ae911aab0800ff7688cadb/base/task/sequence_manager/thread_controller_with_message_pump_impl.cc
[modify] https://crrev.com/f20933cb98543ff525ae911aab0800ff7688cadb/base/task/sequence_manager/thread_controller_with_message_pump_impl.h

Project Member

Comment 9 by bugdroid1@chromium.org, Aug 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/40c17227fb55f7d0979cc63e74740861a7328e36

commit 40c17227fb55f7d0979cc63e74740861a7328e36
Author: Greg Kraynov <kraynov@chromium.org>
Date: Thu Aug 09 12:38:05 2018

Add SetTimerSlack() method to SequenceManager.

We're moving away from using MessageLoop in Blink, hence,
we want to make SequenceManager support any missing pieces
and provide a good level of abstraction on top of a thread.

Specifically, this CL will unblock removing MessageLoop
references from content::RenderThreadImpl.

Bug:  828835 
Change-Id: I5a785a26f35839eea3705ecb5abc61672c497457
Reviewed-on: https://chromium-review.googlesource.com/1167516
Reviewed-by: Alexander Timin <altimin@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Greg Kraynov <kraynov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581876}
[modify] https://crrev.com/40c17227fb55f7d0979cc63e74740861a7328e36/base/task/sequence_manager/sequence_manager.h
[modify] https://crrev.com/40c17227fb55f7d0979cc63e74740861a7328e36/base/task/sequence_manager/sequence_manager_impl.cc
[modify] https://crrev.com/40c17227fb55f7d0979cc63e74740861a7328e36/base/task/sequence_manager/sequence_manager_impl.h
[modify] https://crrev.com/40c17227fb55f7d0979cc63e74740861a7328e36/base/task/sequence_manager/thread_controller.h
[modify] https://crrev.com/40c17227fb55f7d0979cc63e74740861a7328e36/base/task/sequence_manager/thread_controller_impl.cc
[modify] https://crrev.com/40c17227fb55f7d0979cc63e74740861a7328e36/base/task/sequence_manager/thread_controller_impl.h
[modify] https://crrev.com/40c17227fb55f7d0979cc63e74740861a7328e36/base/task/sequence_manager/thread_controller_with_message_pump_impl.cc
[modify] https://crrev.com/40c17227fb55f7d0979cc63e74740861a7328e36/base/task/sequence_manager/thread_controller_with_message_pump_impl.h
[modify] https://crrev.com/40c17227fb55f7d0979cc63e74740861a7328e36/content/renderer/render_thread_impl.cc
[modify] https://crrev.com/40c17227fb55f7d0979cc63e74740861a7328e36/third_party/blink/renderer/platform/scheduler/common/scheduler_helper.cc
[modify] https://crrev.com/40c17227fb55f7d0979cc63e74740861a7328e36/third_party/blink/renderer/platform/scheduler/common/scheduler_helper.h
[modify] https://crrev.com/40c17227fb55f7d0979cc63e74740861a7328e36/third_party/blink/renderer/platform/scheduler/main_thread/main_thread_scheduler_impl.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Aug 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f5d6027c9b9e2b2f2c4c5138175e5371536fb762

commit f5d6027c9b9e2b2f2c4c5138175e5371536fb762
Author: Greg Kraynov <kraynov@chromium.org>
Date: Fri Aug 10 12:28:51 2018

Remove MessageLoop from content::RenderThreadImpl.

It only needs main thread scheduler to initialize,
MessageLoop was unnecessary there.

Bug:  828835 
Change-Id: I70235bbd06af1ca6c576cc77e3e4722a27225069
Reviewed-on: https://chromium-review.googlesource.com/1169211
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Greg Kraynov <kraynov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582125}
[modify] https://crrev.com/f5d6027c9b9e2b2f2c4c5138175e5371536fb762/content/renderer/in_process_renderer_thread.cc
[modify] https://crrev.com/f5d6027c9b9e2b2f2c4c5138175e5371536fb762/content/renderer/render_thread_impl.cc
[modify] https://crrev.com/f5d6027c9b9e2b2f2c4c5138175e5371536fb762/content/renderer/render_thread_impl.h
[modify] https://crrev.com/f5d6027c9b9e2b2f2c4c5138175e5371536fb762/content/renderer/render_thread_impl_browsertest.cc
[modify] https://crrev.com/f5d6027c9b9e2b2f2c4c5138175e5371536fb762/content/renderer/renderer_main.cc

📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/14ace40a640000
😿 Pinpoint job stopped with an error.
https://pinpoint-dot-chromeperf.appspot.com/job/14dc7c32640000

The swarming task expired. The bots are likely overloaded, dead, or misconfigured.
Status: Fixed (was: Started)
Implemented as ThreadControllerImplWithMessagePump

Sign in to add a comment