Support threads owning other threads in platform/scheduler/ [chromium/src : master]

1 view

Skip to first unread message

Nate Chapin (Gerrit)

unread,

Apr 4, 2018, 3:44:23 PM4/4/18

to blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, falken...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, scheduler-...@chromium.org, shimazu...@chromium.org, Alexander Timin, Hiroki Nakagawa, Commit Bot, chromium...@chromium.org

altimin, what do you think of this?

I'm trying to find a way to teach the scheduler how to propagate throttling state to nested worker threads, where a worker thread is owned by a different worker thread, rather than directly by a frame.

To view, visit change 994060. To unsubscribe, or for help writing mail filters, visit settings.

Nate Chapin (Gerrit)

unread,

Apr 4, 2018, 3:50:23 PM4/4/18

to blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, falken...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, scheduler-...@chromium.org, shimazu...@chromium.org, Alexander Timin, Hiroki Nakagawa, Commit Bot, chromium...@chromium.org

Patch Set 1:

altimin, what do you think of this?
I'm trying to find a way to teach the scheduler how to propagate throttling state to nested worker threads, where a worker thread is owned by a different worker thread, rather than directly by a frame.

Also, note that this was split out of a monolithic CL (https://chromium-review.googlesource.com/c/chromium/src/+/953746) which I believe implements nested workers in their entirety.

To view, visit change 994060. To unsubscribe, or for help writing mail filters, visit settings.

Alexander Timin (Gerrit)

unread,

Apr 4, 2018, 5:09:12 PM4/4/18

to Nate Chapin, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, falken...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, scheduler-...@chromium.org, shimazu...@chromium.org, Hiroki Nakagawa, Commit Bot, chromium...@chromium.org

Patch Set 1:

Patch Set 1:
altimin, what do you think of this?
I'm trying to find a way to teach the scheduler how to propagate throttling state to nested worker threads, where a worker thread is owned by a different worker thread, rather than directly by a frame.
Also, note that this was split out of a monolithic CL (https://chromium-review.googlesource.com/c/chromium/src/+/953746) which I believe implements nested workers in their entirety.

Based on a quick skim, general direction seems reasonable. I'll do a proper review and drop a few nits tomorrow. (mostly about thread guarantees, DCHECKs for it and replacing IsMainThread with IsParentObjectThread()). Ensuring that we either have a parent frame or a parent worker (and maybe splitting WorkerSchedulerProxy) might be a good idea.

To view, visit change 994060. To unsubscribe, or for help writing mail filters, visit settings.

Nate Chapin (Gerrit)

unread,

Apr 4, 2018, 6:08:28 PM4/4/18

to blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, falken...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, scheduler-...@chromium.org, shimazu...@chromium.org, Alexander Timin, Hiroki Nakagawa, Commit Bot, chromium...@chromium.org

1 comment:

File third_party/WebKit/Source/platform/MemoryCoordinator.cpp:
- Patch Set #2, Line 60: DEFINE_THREAD_SAFE_STATIC_LOCAL(CrossThreadPersistent<MemoryCoordinator>,
  Missed this in initial upload. Called by RegisterThread/UnregisterThread, which are called during WebThreadSupportingGC creation/destruction.

To view, visit change 994060. To unsubscribe, or for help writing mail filters, visit settings.

Hiroki Nakagawa (Gerrit)

unread,

Apr 6, 2018, 2:50:56 AM4/6/18

to Nate Chapin, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, falken...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, scheduler-...@chromium.org, shimazu...@chromium.org, Kenichi Ishibashi, Alexander Timin, Commit Bot, chromium...@chromium.org

+bashi@ for MemoryCoodinator. Can you take a look at my review comment in MemoryCoordinator.cpp?

8 comments:

File third_party/WebKit/Source/core/workers/DedicatedWorkerThread.cpp:
- Patch Set #2, Line 54: return ToDocument(loading_context->GetExecutionContext())
  We might need to return early when loading_context->GetExecutionContext()->IsDocument() is false.
File third_party/WebKit/Source/platform/MemoryCoordinator.cpp:

- Patch Set #2, Line 60: DEFINE_THREAD_SAFE_STATIC_LOCAL(CrossThreadPersistent<MemoryCoordinator>,

- Missed this in initial upload. […]
  I suspect RegisterThread() and UnregisterThread() are not thread-safe.
  +bashi@: Do you think we can make MemoryCoordinator thread-safe or have a workaround?
File third_party/WebKit/Source/platform/WebThreadSupportingGC.cpp:
- Patch Set #2, Line 31:
  Can you check thread-affinity with base::ThreadChecker?
- Patch Set #2, Line 50:
  ditto.
File third_party/WebKit/Source/platform/scheduler/child/worker_scheduler_proxy.h:
- Patch Set #2, Line 23:
```
communication between frame scheduler (main thread) and
// worker scheduler (worker thread)
```
  This can be "between worker scheduler (parent worker thread) and worker scheduler (child worker thread)" for nested workers.
- Patch Set #2, Line 29: // outlives worker thread too.
  Can you also update this comment? DedicatedWorkerThread can be created and destroyed on the parent worker thread.

File third_party/WebKit/Source/platform/scheduler/child/worker_scheduler_proxy.cc:

Patch Set #2, Line 25: }

How about having separate ctors for FrameScheduler and WorkerSchedulerProxy?

WorkerSchedulerProxy::WorkerSchedulerProxy(frame_scheduler) {
  DCHECK(frame_scheduler);
  throttling_observer_handle_ = frame_scheduler->AddThrottlingObserver();
  parent_frame_type_ = GetFrameOriginType();
}

WorkerSchedulerProxy::WorkerSchedulerProxy(parent_scheduler_proxy)
  : parent_scheduler_(parent_scheduler_proxy->worker_scheduler_.get()) {
  DCHECK(parent_scheduler_);
  parent_scheduler_->AddChildWorkerSchedulerProxy(this);
}

Patch Set #2, Line 46: DCHECK(IsMainThread() || parent_scheduler_);
How about checking this using base::ThreadChecker?

To view, visit change 994060. To unsubscribe, or for help writing mail filters, visit settings.

Hiroki Nakagawa (Gerrit)

unread,

Apr 6, 2018, 2:50:56 AM4/6/18

to Kenichi Ishibashi, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, falken...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, scheduler-...@chromium.org, shimazu...@chromium.org, Nate Chapin, Alexander Timin

Hiroki Nakagawa would like Kenichi Ishibashi to review this change.

Support threads owning other threads in platform/scheduler/

This will support scheduling for nested workers.

Change-Id: If005e3ec781b2fc0ecd3ba1ba73fa4b0e6ef1a0f
---
M third_party/WebKit/Source/core/workers/DedicatedWorkerThread.cpp
M third_party/WebKit/Source/platform/MemoryCoordinator.cpp
M third_party/WebKit/Source/platform/WebThread.cpp
M third_party/WebKit/Source/platform/WebThreadSupportingGC.cpp
M third_party/WebKit/Source/platform/scheduler/child/webthread_impl_for_worker_scheduler.cc
M third_party/WebKit/Source/platform/scheduler/child/webthread_impl_for_worker_scheduler.h
M third_party/WebKit/Source/platform/scheduler/child/worker_scheduler_proxy.cc
M third_party/WebKit/Source/platform/scheduler/child/worker_scheduler_proxy.h
M third_party/WebKit/Source/platform/scheduler/worker/worker_thread_scheduler.cc
M third_party/WebKit/Source/platform/scheduler/worker/worker_thread_scheduler.h
M third_party/WebKit/public/platform/WebThread.h
11 files changed, 86 insertions(+), 23 deletions(-)

To view, visit change 994060. To unsubscribe, or for help writing mail filters, visit settings.

Kenichi Ishibashi (Gerrit)

unread,

Apr 6, 2018, 3:33:08 AM4/6/18

to Nate Chapin, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, falken...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, scheduler-...@chromium.org, shimazu...@chromium.org, Hiroki Nakagawa, Alexander Timin, Commit Bot, chromium...@chromium.org

1 comment:

File third_party/WebKit/Source/platform/MemoryCoordinator.cpp:

- Patch Set #2, Line 60: DEFINE_THREAD_SAFE_STATIC_LOCAL(CrossThreadPersistent<MemoryCoordinator>,

- I suspect RegisterThread() and UnregisterThread() are not thread-safe. […]
  Yeah, it seems that RegisterThread() and UnregisterThread() are non thread-safe. There might be couple of options to make them thread-safe, probably using a mutex would be a quick fix?

To view, visit change 994060. To unsubscribe, or for help writing mail filters, visit settings.

Nate Chapin (Gerrit)

unread,

Apr 6, 2018, 4:13:17 PM4/6/18

to blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, falken...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, scheduler-...@chromium.org, shimazu...@chromium.org, Kenichi Ishibashi, Hiroki Nakagawa, Alexander Timin, Commit Bot, chromium...@chromium.org

8 comments:

File third_party/WebKit/Source/core/workers/DedicatedWorkerThread.cpp:
- Patch Set #2, Line 54: if (!loading_context->GetExecutionContext()->IsDocument())

- We might need to return early when loading_context->GetExecutionContext()->IsDocument() is false.

- Yeah, that was done in https://chromium.googlesource.com/chromium/src/+/4be1c8bcb294429b101a53ff7ba9248c6ba134d7, I just apparently uploaded this CL before that one landed.
File third_party/WebKit/Source/platform/MemoryCoordinator.cpp:

- Patch Set #2, Line 60: DEFINE_THREAD_SAFE_STATIC_LOCAL(CrossThreadPersistent<MemoryCoordinator>,

- Yeah, it seems that RegisterThread() and UnregisterThread() are non thread-safe. […]
  Attempted to use a mutex. I have ~zero confidence that I did it correctly.
File third_party/WebKit/Source/platform/WebThreadSupportingGC.cpp:

- Patch Set #2, Line 31:
  Can you check thread-affinity with base::ThreadChecker?

- Done

- Patch Set #2, Line 50:
  ditto.

- Patch Set #2, Line 23:

- ```
// Helper class for communication b
```
  This can be "between worker scheduler (parent worker thread) and worker scheduler (child worker thre […]
  Done
- Patch Set #2, Line 29: // on the owner's thread. It's passed to WorkerScheduler during its
  Can you also update this comment? DedicatedWorkerThread can be created and destroyed on the parent w […]
  Done
File third_party/WebKit/Source/platform/scheduler/child/worker_scheduler_proxy.cc:
- Patch Set #2, Line 25: parent_scheduler_->AddChildWorkerSchedulerProxy(this);
  How about having separate ctors for FrameScheduler and WorkerSchedulerProxy? […]
  Done
- Patch Set #2, Line 46: FrameScheduler::ThrottlingState throttling_state) {

- How about checking this using base::ThreadChecker?

- Done

To view, visit change 994060. To unsubscribe, or for help writing mail filters, visit settings.

Alexander Timin (Gerrit)

unread,

Apr 9, 2018, 9:43:35 AM4/9/18

to Nate Chapin, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, falken...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, scheduler-...@chromium.org, shimazu...@chromium.org, Kenichi Ishibashi, Hiroki Nakagawa, Commit Bot, chromium...@chromium.org

1 comment:

File third_party/WebKit/Source/platform/scheduler/worker/worker_thread_scheduler.cc:
- Patch Set #3, Line 189:
```
for (WorkerSchedulerProxy* child_proxy : child_worker_scheduler_proxies_)
    child_proxy->OnThrottlingStateChanged(throttling_state);
```
  It seems that a OnThrottlingStateChanged notification will have to do multiple thread hops for nested workers, which is unfortunate -- scripts in workers can be arbitrary long, so this notification might be delayed.
  I would strongly prefer if we could all WorkerSchedulerProxy::OnThrottlingStateChanged on the main thread. I wonder if it's possible to register an observer asynchronously -- when a nested WorkerSchedulerProxy is created, a async call is dispatched to the main thread to register it as an observer.

To view, visit change 994060. To unsubscribe, or for help writing mail filters, visit settings.

Kenichi Ishibashi (Gerrit)

unread,

Apr 12, 2018, 12:54:12 AM4/12/18

to Nate Chapin, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, falken...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, scheduler-...@chromium.org, shimazu...@chromium.org, Alexander Timin, Hiroki Nakagawa, Commit Bot, chromium...@chromium.org

1 comment:

File third_party/WebKit/Source/platform/MemoryCoordinator.cpp:
- Patch Set #3, Line 114: for (auto thread : web_threads_) {
  I guess that we need a lock here?

To view, visit change 994060. To unsubscribe, or for help writing mail filters, visit settings.

Alexander Timin (Gerrit)

unread,

Apr 27, 2018, 2:59:05 PM4/27/18

to Nate Chapin, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, falken...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, scheduler-...@chromium.org, shimazu...@chromium.org, Kenichi Ishibashi, Hiroki Nakagawa, Commit Bot, chromium...@chromium.org

As discussed offline, general approach lgtm (but it still terrifies me slightly).

One serious comment that I have is that SetParentThread() seems like a wrong abstraction.

Could we use FrameOrWorkerScheduler and change SetFrameScheduler to SetParentScheduler?
We also could add IsFrameScheduler and IsWorkerScheduler to FrameOrWorkerScheduler (and probably GetWorkerThreadScheduler to WorkerScheduler).

To view, visit change 994060. To unsubscribe, or for help writing mail filters, visit settings.

Nate Chapin (Gerrit)

unread,

May 22, 2018, 5:18:37 PM5/22/18

to blink-re...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, falken...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, scheduler-...@chromium.org, shimazu...@chromium.org, Alexander Timin, Kenichi Ishibashi, Hiroki Nakagawa, Commit Bot, chromium...@chromium.org

Patch Set 5:

As discussed offline, general approach lgtm (but it still terrifies me slightly).
One serious comment that I have is that SetParentThread() seems like a wrong abstraction.
Could we use FrameOrWorkerScheduler and change SetFrameScheduler to SetParentScheduler?
We also could add IsFrameScheduler and IsWorkerScheduler to FrameOrWorkerScheduler (and probably GetWorkerThreadScheduler to WorkerScheduler).

Rewrote to use FrameOrWorkerScheduler, sorry for taking so long to get back to this. PTAL :)

3 comments:

File third_party/blink/renderer/platform/scheduler/child/worker_scheduler.cc:
- Patch Set #10, Line 44: NotifyThrottlingObservers();
  This line drives the propagation across the tree of nested workers.
File third_party/blink/renderer/platform/scheduler/child/worker_scheduler_proxy.cc:
- Patch Set #10, Line 51: FROM_HERE, base::BindOnce(&WorkerScheduler::OnThrottlingStateChanged,
  Instead of WorkerSchedulerProxy holding a WorkerThreadScheduler and notifying it when throttling state changes, it holds a WorkerScheduler, which in turn notifies the WorkerThreadScheduler and any child WorkerSchedulerProxies.
File third_party/blink/renderer/platform/scheduler/public/frame_or_worker_scheduler.h:
- Patch Set #10, Line 22: enum class ObserverType { kLoader, kWorkerScheduler };
  All of the additions to this class should be paired with a removal from FrameScheduler or FrameSchedulerImpl (and vice versa).

To view, visit change 994060. To unsubscribe, or for help writing mail filters, visit settings.

Alexander Timin (Gerrit)

unread,

May 23, 2018, 1:31:58 PM5/23/18

to Nate Chapin, blink-re...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, falken...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, scheduler-...@chromium.org, shimazu...@chromium.org, Kenichi Ishibashi, Hiroki Nakagawa, Commit Bot, chromium...@chromium.org

This looks great, thanks for following up!

Patch set 10:Code-Review +1

2 comments:

File third_party/blink/renderer/core/workers/worker_thread.cc:
- Patch Set #10, Line 421: static_cast<scheduler::WorkerThreadScheduler
  I'm becoming convinced that we need to merge WorkerThreadScheduler and NonMainThreadScheduler. Feel free to add TODO.
File third_party/blink/renderer/platform/scheduler/child/worker_scheduler_proxy.cc:
- Patch Set #10, Line 20: static_cast<FrameScheduler*>(scheduler)
  Instead of relying on static casts I'd prefer to add ToWorkerScheduler() and ToFrameScheduler() methods to FrameOrWorkerScheduler (which might return null).

To view, visit change 994060. To unsubscribe, or for help writing mail filters, visit settings.

Nate Chapin (Gerrit)

unread,

May 23, 2018, 7:51:35 PM5/23/18

to blink-re...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, falken...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, scheduler-...@chromium.org, shimazu...@chromium.org, Alexander Timin, Kenichi Ishibashi, Hiroki Nakagawa, Commit Bot, chromium...@chromium.org

2 comments:

File third_party/blink/renderer/core/workers/worker_thread.cc:

- Patch Set #10, Line 421: static_cast<scheduler::WorkerThreadScheduler

- I'm becoming convinced that we need to merge WorkerThreadScheduler and NonMainThreadScheduler. […]
  https://chromium.googlesource.com/chromium/src/+/fda179ab357760462e8ca54a8097c48b76a13bfe/third_party/blink/renderer/platform/scheduler/public/non_main_thread_scheduler.h#27 already exists. I think that's similar enough to justify not adding another TODO, but feel free to override me :)
File third_party/blink/renderer/platform/scheduler/child/worker_scheduler_proxy.cc:

- Patch Set #10, Line 20:

- Instead of relying on static casts I'd prefer to add ToWorkerScheduler() and ToFrameScheduler() meth […]
  Done

To view, visit change 994060. To unsubscribe, or for help writing mail filters, visit settings.

Hiroki Nakagawa (Gerrit)

unread,

May 23, 2018, 11:47:05 PM5/23/18

to Nate Chapin, blink-re...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, falken...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, scheduler-...@chromium.org, shimazu...@chromium.org, Alexander Timin, Kenichi Ishibashi, Commit Bot, chromium...@chromium.org

LGTM

Patch set 11:Code-Review +1

6 comments:

File third_party/blink/renderer/platform/scheduler/child/worker_scheduler.h:
- Patch Set #11, Line 27: explicit
  Can you remove "explicit"?
File third_party/blink/renderer/platform/scheduler/child/worker_scheduler_proxy.h:
- Patch Set #11, Line 28:
```
// It's owned by DedicatedWorkerThread and is created and destroyed
```

- ```
// on the owner's thread.
```

- "the owner's thread" may sound confusing because, against the name of "DedicatedWorkerThread", it's owned by the parent thread, not by the (child) worker thread.
  So, how about saying "the parent thread" instead "the owner's thread"? "parent" seems well-defined in the first paragraph.
- Patch Set #11, Line 44: the main thread
  Can this be called from the parent worker thread?
- Patch Set #11, Line 51: DCHECK(IsMainThread() || !initialized_);
  ditto.
- Patch Set #11, Line 72: thread_checker_
  Can you rename this to |parent_thread_checker_| to clarify which thread this checker checks?
File third_party/blink/renderer/platform/scheduler/child/worker_scheduler_proxy_unittest.cc:
- Patch Set #11, Line 70: void DisposeWorkerSchedulerOnThread(base::WaitableEvent* completion) {
  DCHECK(thread_task_runner_->BelongsToCurrentThread());

To view, visit change 994060. To unsubscribe, or for help writing mail filters, visit settings.

Nate Chapin (Gerrit)

unread,

May 24, 2018, 4:52:08 PM5/24/18

to blink-re...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, falken...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, scheduler-...@chromium.org, shimazu...@chromium.org, Hiroki Nakagawa, Alexander Timin, Kenichi Ishibashi, Commit Bot, chromium...@chromium.org

6 comments:

File third_party/blink/renderer/platform/scheduler/child/worker_scheduler.h:
- Patch Set #11, Line 27: WorkerSc

- Can you remove "explicit"?

- Patch Set #11, Line 28:

// It's owned by DedicatedWorkerThread and is created and destroyed

- ```
// on the parent thread. 
```
  "the owner's thread" may sound confusing because, against the name of "DedicatedWorkerThread", it's […]
  Done

- Patch Set #11, Line 44:

- Can this be called from the parent worker thread?
  Only from child worker thread during init.
- Patch Set #11, Line 51: base::Optional<FrameOriginType> parent_frame_type() const {
  ditto.
  ditto. :)

- Patch Set #11, Line 72:

- Can you rename this to |parent_thread_checker_| to clarify which thread this checker checks?
  Done
File third_party/blink/renderer/platform/scheduler/child/worker_scheduler_proxy_unittest.cc:

- Patch Set #11, Line 70: void DisposeWorkerSchedulerOnThread(base::WaitableEvent* completion) {
  DCHECK(thread_task_runner_->BelongsToCurrentThread());

- Done

To view, visit change 994060. To unsubscribe, or for help writing mail filters, visit settings.

Nate Chapin (Gerrit)

unread,

May 25, 2018, 6:20:37 PM5/25/18

to blink-re...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, falken...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, scheduler-...@chromium.org, shimazu...@chromium.org, Hiroki Nakagawa, Alexander Timin, Kenichi Ishibashi, Commit Bot, chromium...@chromium.org

Patch set 13:Commit-Queue +2

To view, visit change 994060. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

May 25, 2018, 6:20:48 PM5/25/18

to Nate Chapin, blink-re...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, falken...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, scheduler-...@chromium.org, shimazu...@chromium.org, Hiroki Nakagawa, Alexander Timin, Kenichi Ishibashi, chromium...@chromium.org

CQ is trying the patch.

Note: The patchset sent to CQ was uploaded after this CL was approved.
"Rebase" https://chromium-review.googlesource.com/c/994060/13

Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/994060/13

Bot data: {"action": "start", "triggered_at": "2018-05-25T22:20:35.0Z", "cq_cfg_revision": "6a25833cb93164fb378d8087f9dae5fc28fc9b1e", "revision": "63a7bc068b080b949a57b8c1161fd66217db5d23"}

To view, visit change 994060. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

May 25, 2018, 6:25:31 PM5/25/18

to Nate Chapin, blink-re...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, falken...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, scheduler-...@chromium.org, shimazu...@chromium.org, Hiroki Nakagawa, Alexander Timin, Kenichi Ishibashi, chromium...@chromium.org

Try jobs failed on following builders:
  chromium_presubmit on luci.chromium.try (JOB_FAILED, https://ci.chromium.org/p/chromium/builders/luci.chromium.try/chromium_presubmit/124529)

To view, visit change 994060. To unsubscribe, or for help writing mail filters, visit settings.

Pavel Feldman (Gerrit)

unread,

May 25, 2018, 6:53:04 PM5/25/18

to Nate Chapin, blink-re...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, falken...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, scheduler-...@chromium.org, shimazu...@chromium.org, Hiroki Nakagawa, Alexander Timin, Kenichi Ishibashi, Commit Bot, chromium...@chromium.org

Patch set 13:Code-Review +1

To view, visit change 994060. To unsubscribe, or for help writing mail filters, visit settings.

Nate Chapin (Gerrit)

unread,

May 25, 2018, 6:53:30 PM5/25/18

to blink-re...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, falken...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, scheduler-...@chromium.org, shimazu...@chromium.org, Pavel Feldman, Hiroki Nakagawa, Alexander Timin, Kenichi Ishibashi, Commit Bot, chromium...@chromium.org

Patch set 13:Commit-Queue +2

To view, visit change 994060. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

May 25, 2018, 7:04:05 PM5/25/18

to Nate Chapin, blink-re...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, falken...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, scheduler-...@chromium.org, shimazu...@chromium.org, Pavel Feldman, Hiroki Nakagawa, Alexander Timin, Kenichi Ishibashi, chromium...@chromium.org

Commit Bot merged this change.

Approvals:
  Hiroki Nakagawa: Looks good to me
  Pavel Feldman: Looks good to me
  Alexander Timin: Looks good to me
  Nate Chapin: Commit

Support WorkerSchedulers for nested workers

Currently, FrameSchedulerImpl keeps a set of throttling observers that
it notifies when throttling state changes. This includes each worker's
WorkerSchedulerProxy.

This CL moves throttling observers to the base class,
FrameOrWorkerScheduler, allowing each nested worker to register its
WorkerSchedulerProxy with its parent WorkerScheduler, so the
WorkerScheduler can then propagate throttling state to the nested
worker.

Bug: 829119
Change-Id: If005e3ec781b2fc0ecd3ba1ba73fa4b0e6ef1a0f
Reviewed-on: https://chromium-review.googlesource.com/994060
Reviewed-by: Pavel Feldman <pfel...@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhi...@chromium.org>
Reviewed-by: Alexander Timin <alt...@chromium.org>
Commit-Queue: Nate Chapin <jap...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562052}
---
M third_party/blink/public/platform/web_thread.h
M third_party/blink/renderer/core/frame/local_frame.cc
M third_party/blink/renderer/core/workers/dedicated_worker_thread.cc
M third_party/blink/renderer/core/workers/worker_thread.cc
M third_party/blink/renderer/platform/scheduler/BUILD.gn
M third_party/blink/renderer/platform/scheduler/child/webthread_impl_for_worker_scheduler.cc
M third_party/blink/renderer/platform/scheduler/child/webthread_impl_for_worker_scheduler.h
M third_party/blink/renderer/platform/scheduler/child/worker_scheduler.cc
M third_party/blink/renderer/platform/scheduler/child/worker_scheduler.h
M third_party/blink/renderer/platform/scheduler/child/worker_scheduler_proxy.cc
M third_party/blink/renderer/platform/scheduler/child/worker_scheduler_proxy.h
M third_party/blink/renderer/platform/scheduler/child/worker_scheduler_proxy_unittest.cc
M third_party/blink/renderer/platform/scheduler/child/worker_scheduler_unittest.cc
A third_party/blink/renderer/platform/scheduler/common/frame_or_worker_scheduler.cc
M third_party/blink/renderer/platform/scheduler/main_thread/frame_scheduler_impl.cc
M third_party/blink/renderer/platform/scheduler/main_thread/frame_scheduler_impl.h
M third_party/blink/renderer/platform/scheduler/public/frame_or_worker_scheduler.h
M third_party/blink/renderer/platform/scheduler/public/frame_scheduler.h
M third_party/blink/renderer/platform/scheduler/test/fake_frame_scheduler.h
M third_party/blink/renderer/platform/scheduler/worker/worker_thread_scheduler.cc
M third_party/blink/renderer/platform/scheduler/worker/worker_thread_scheduler.h
M third_party/blink/renderer/platform/web_thread.cc
22 files changed, 303 insertions(+), 237 deletions(-)

To view, visit change 994060. To unsubscribe, or for help writing mail filters, visit settings.

Dmitry Gozman (Gerrit)

unread,

May 27, 2018, 11:45:04 AM5/27/18

to Nate Chapin, Commit Bot, blink-re...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, falken...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, scheduler-...@chromium.org, shimazu...@chromium.org, Pavel Feldman, Hiroki Nakagawa, Alexander Timin, Kenichi Ishibashi, chromium...@chromium.org

1 comment:

File third_party/blink/renderer/platform/scheduler/public/frame_or_worker_scheduler.h:
- Patch Set #14, Line 18: class PLATFORM_EXPORT FrameOrWorkerScheduler {
  Should this be an ExecutionContextScheduler?

To view, visit change 994060. To unsubscribe, or for help writing mail filters, visit settings.

Hiroki Nakagawa (Gerrit)

unread,

May 28, 2018, 12:12:08 AM5/28/18

to Nate Chapin, Commit Bot, blink-re...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, falken...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, scheduler-...@chromium.org, shimazu...@chromium.org, Dmitry Gozman, Pavel Feldman, Alexander Timin, Kenichi Ishibashi, chromium...@chromium.org

1 comment:

File third_party/blink/renderer/platform/scheduler/public/frame_or_worker_scheduler.h:

- Patch Set #14, Line 18: class PLATFORM_EXPORT FrameOrWorkerScheduler {
  Should this be an ExecutionContextScheduler?

- There was a discussion about it before and we decided to name this class FrameOrWorkerScheduler because Frame is technically not an ExecutionContext.
  See https://chromium-review.googlesource.com/c/chromium/src/+/994552/12#message-74ad9ebc7bae3393e3d427866e10d7516b98e798

To view, visit change 994060. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward

0 new messages

Search

Clear search

Close search

Google apps

Main menu