New issue
Advanced search Search tips

Issue 846255 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 16
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Add TaskType::kWorkerThreadTaskQueueV8

Project Member Reported by hajimehoshi@chromium.org, May 24 2018

Issue description

Now NonMainThreadScheduler::V8TaskRunner returns a v8 task runner with kMainThreadTaskQueueV8 at https://chromium-review.googlesource.com/c/chromium/src/+/1051549. This doesn't make sense since V8 task runner for the main thread and for the non-main thread is different. Actually keishi@ was confused when he tried to add a new task runner for BlinkGC, which can work on both main and non-main thread. Let's keep TaskType consistent in terms of threading.

So I plan to add kWorkerThreadTaskQueueV8, but I'd like to confirm: Is NonMainThreadScheduler::V8TaskRunner only valid for worker thread? If yes, only TaskType::kWorkerThreadTaskQueueV8 is enough.

 
Components: Blink>Scheduling
NonMainThreadScheduler::V8TaskRunner is called only at two places:

* third_party/blink/renderer/bindings/core/v8/v8_initializer.cc:

I feel like we should use the main thread one there.


* third_party/blink/renderer/core/inspector/thread_debugger.cc

I'm not sure we can always use the main thread one there. If it is ok to use main thread's v8 task runner there, we can eliminate NonMainThreadScheduler::v8_task_runner_.
On the other hand, I found a place where V8TaskRunner() is not used but should be:

https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/workers/worker_backing_thread.cc?l=80

The isolate task runner should be V8 task runner.

Then my current conclusion is still adding TaskType::kWorkerThreadTaskQueueV8. Alexander, does this make sense?
Yes, I think it makes total sense.
Project Member

Comment 5 by bugdroid1@chromium.org, May 25 2018

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

commit b8531a72488de4eb7d9c6396472a3f88b7ab3e69
Author: Hajime Hoshi <hajimehoshi@chromium.org>
Date: Fri May 25 08:02:17 2018

Add TaskType::kWorkerThreadTaskQueueV8 and use it for workers

This CL fixes worker's isolate to use V8 task runner instead of worker
thread's default task runner.

Bug:  841172 ,  846255 
Change-Id: I3d462d0188f1860cad8b30ce083e24f9161afc84
Reviewed-on: https://chromium-review.googlesource.com/1071310
Reviewed-by: Alexander Timin <altimin@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Hajime Hoshi <hajimehoshi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561805}
[modify] https://crrev.com/b8531a72488de4eb7d9c6396472a3f88b7ab3e69/third_party/blink/public/platform/task_type.h
[modify] https://crrev.com/b8531a72488de4eb7d9c6396472a3f88b7ab3e69/third_party/blink/renderer/core/workers/worker_backing_thread.cc
[modify] https://crrev.com/b8531a72488de4eb7d9c6396472a3f88b7ab3e69/third_party/blink/renderer/platform/scheduler/child/worker_scheduler.cc
[modify] https://crrev.com/b8531a72488de4eb7d9c6396472a3f88b7ab3e69/third_party/blink/renderer/platform/scheduler/main_thread/frame_scheduler_impl.cc
[modify] https://crrev.com/b8531a72488de4eb7d9c6396472a3f88b7ab3e69/third_party/blink/renderer/platform/scheduler/main_thread/main_thread_scheduler_impl.cc
[modify] https://crrev.com/b8531a72488de4eb7d9c6396472a3f88b7ab3e69/third_party/blink/renderer/platform/scheduler/public/non_main_thread_scheduler.h
[modify] https://crrev.com/b8531a72488de4eb7d9c6396472a3f88b7ab3e69/third_party/blink/renderer/platform/scheduler/worker/compositor_thread_scheduler.cc
[modify] https://crrev.com/b8531a72488de4eb7d9c6396472a3f88b7ab3e69/third_party/blink/renderer/platform/scheduler/worker/compositor_thread_scheduler.h
[modify] https://crrev.com/b8531a72488de4eb7d9c6396472a3f88b7ab3e69/third_party/blink/renderer/platform/scheduler/worker/non_main_thread_scheduler.cc
[modify] https://crrev.com/b8531a72488de4eb7d9c6396472a3f88b7ab3e69/third_party/blink/renderer/platform/scheduler/worker/worker_thread_scheduler.cc
[modify] https://crrev.com/b8531a72488de4eb7d9c6396472a3f88b7ab3e69/third_party/blink/renderer/platform/scheduler/worker/worker_thread_scheduler.h
[modify] https://crrev.com/b8531a72488de4eb7d9c6396472a3f88b7ab3e69/tools/metrics/histograms/enums.xml

Status: Fixed (was: Assigned)

Sign in to add a comment