Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2518073002:
Worker: Use per-frame task runners from a worker thread (Closed)
|
Created:
4 years, 1 month ago by nhiroki Modified:
4 years ago CC:
chromium-reviews, shimazu+worker_chromium.org, kinuko+worker_chromium.org, blink-reviews, horo+watch_chromium.org, falken+watch_chromium.org, kinuko+watch, blink-worker-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWorker: Use per-frame task runners from a worker thread
This is a part of the project to migrate to per-frame scheduler.
A general design doc about the project is available here:
https://docs.google.com/document/d/10It1DFRP7H3gev9hQA-7dtTcvcMFhrxuCt3-k21yjgQ/edit?usp=sharing
BUG=667310
Committed: https://crrev.com/3ea28ac89f4e7b4603017fa1b8fcb6695d9ad79c
Cr-Commit-Position: refs/heads/master@{#434615}
Patch Set 1 #Patch Set 2 : fix compile failures #Patch Set 3 : fix test crashes #
Total comments: 2
Patch Set 4 : rebase #
Messages
Total messages: 31 (22 generated)
The CQ bit was checked by nhiroki@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: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by nhiroki@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...
nhiroki@chromium.org changed reviewers: + falken@chromium.org, haraken@chromium.org
PTAL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Just to confirm: These messages don't need to be TaskType::PostedMessage (i.e., the control messages don't need to be ordered with normal posted messages), right? LGTM.
The CQ bit was checked by nhiroki@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: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
code lgtm. it'd be nice for the CL description to have a short justification or link for why this change is happening (you can follow the trail of linked/blocking bugs to find the design doc but a more direct link or description could be better). https://codereview.chromium.org/2518073002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp (right): https://codereview.chromium.org/2518073002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp:29: // Notify the main thread that the debugger task is waiting for the signal. Are these comments (ln 24 and 29) still accurate now that it's using getParentFrameTaskRunner instead of mainThread()->getWebTaskRunner()?
Description was changed from ========== Worker: Use per-frame task runners from a worker thread BUG=667310 ========== to ========== Worker: Use per-frame task runners from a worker thread This is a part of the project to migrate to per-frame scheduler. A general design doc about the project is available here: https://docs.google.com/document/d/10It1DFRP7H3gev9hQA-7dtTcvcMFhrxuCt3-k21yj... BUG=667310 ==========
On 2016/11/22 00:45:38, haraken wrote: > Just to confirm: These messages don't need to be TaskType::PostedMessage (i.e., > the control messages don't need to be ordered with normal posted messages), > right? > > LGTM. Yes, I think we don't have to make them ordered with normal posted messages because... (1) regarding WebSharedWorkerImpl.cc, SharedWorker provides inter-process MessagePort communication instead of a regular in-process postMessage() like DedicatedWorker, that is, SharedWorker doesn't use TaskType::PostedMessage. (2) regarding WorkerThread.cpp, it should be safe even if the termination message (TaskType::Internal) overtakes the posted messages (TaskType::PostedMessage) because in the case the posted messages is gracefully cancelled by weakptr at [1]. In addition, this is for forcible termination task and presumably we don't have to ensure the message ordering among them in such an emergency case. [1] https://chromium.googlesource.com/chromium/src/+/6a8002607cc8e991e382adb29c7c...
On 2016/11/22 05:45:26, falken wrote: > code lgtm. it'd be nice for the CL description to have a short justification or > link for why this change is happening (you can follow the trail of > linked/blocking bugs to find the design doc but a more direct link or > description could be better). Updated.
https://codereview.chromium.org/2518073002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp (right): https://codereview.chromium.org/2518073002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp:29: // Notify the main thread that the debugger task is waiting for the signal. On 2016/11/22 05:45:26, falken wrote: > Are these comments (ln 24 and 29) still accurate now that it's using > getParentFrameTaskRunner instead of mainThread()->getWebTaskRunner()? Yes, it's still accurate. ParentFrameTaskRunners are internally associated with main document's frame on the main thread, and this task will be posted to the main thread. Some DCHECKs will confirm it.
The CQ bit was checked by nhiroki@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 nhiroki@chromium.org
The CQ bit was checked by nhiroki@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, falken@chromium.org Link to the patchset: https://codereview.chromium.org/2518073002/#ps60001 (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": 60001, "attempt_start_ts": 1480315078542840,
"parent_rev": "45d044f8102c01fa08bd17942538fa874b69de8d", "commit_rev":
"ae5f4b9c96619bd118b44817bdbf8d574d15ae61"}
Message was sent while issue was closed.
Description was changed from ========== Worker: Use per-frame task runners from a worker thread This is a part of the project to migrate to per-frame scheduler. A general design doc about the project is available here: https://docs.google.com/document/d/10It1DFRP7H3gev9hQA-7dtTcvcMFhrxuCt3-k21yj... BUG=667310 ========== to ========== Worker: Use per-frame task runners from a worker thread This is a part of the project to migrate to per-frame scheduler. A general design doc about the project is available here: https://docs.google.com/document/d/10It1DFRP7H3gev9hQA-7dtTcvcMFhrxuCt3-k21yj... BUG=667310 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Worker: Use per-frame task runners from a worker thread This is a part of the project to migrate to per-frame scheduler. A general design doc about the project is available here: https://docs.google.com/document/d/10It1DFRP7H3gev9hQA-7dtTcvcMFhrxuCt3-k21yj... BUG=667310 ========== to ========== Worker: Use per-frame task runners from a worker thread This is a part of the project to migrate to per-frame scheduler. A general design doc about the project is available here: https://docs.google.com/document/d/10It1DFRP7H3gev9hQA-7dtTcvcMFhrxuCt3-k21yj... BUG=667310 Committed: https://crrev.com/3ea28ac89f4e7b4603017fa1b8fcb6695d9ad79c Cr-Commit-Position: refs/heads/master@{#434615} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/3ea28ac89f4e7b4603017fa1b8fcb6695d9ad79c Cr-Commit-Position: refs/heads/master@{#434615} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
