Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1519863002:
Expose WorkerPool to blink (Closed)
|
Created:
5 years ago by Justin Novosad Modified:
5 years ago Reviewers:
danakj, jochen (gone - plz use gerrit), Sami, gab, esprehn, kinuko, haraken, mithro, Noel Gordon CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, jam, xlai (Olivia) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExpose WorkerPool to blink
Adding an API to blink's platform to allow blink to submit
background tasks to chromium's worker thread pool.
The plan is to use this for HTMLCanvasElement::toBlob
BUG=567887
Committed: https://crrev.com/5a3b91b21b97dcd68af85b06d1dd143571e596e3
Cr-Commit-Position: refs/heads/master@{#366251}
Patch Set 1 #Patch Set 2 : fix accidental edit #
Total comments: 4
Patch Set 3 : onion soupification #
Total comments: 1
Patch Set 4 : simplify DEPS #
Total comments: 1
Patch Set 5 : make non copyable #Patch Set 6 : build fix #Patch Set 7 : fix build for realz #
Total comments: 2
Patch Set 8 : remove unnecessary include #
Total comments: 2
Patch Set 9 : use closures + make safe #
Total comments: 8
Patch Set 10 : threading subdir + namespace #
Total comments: 1
Patch Set 11 : DEPS tweak #
Depends on Patchset: Messages
Total messages: 65 (27 generated)
junov@chromium.org changed reviewers: + jochen@chromium.org
@jochen: lucky you, you are the only person to be an OWNER in all these places! PTAL
+cc xlai: FYI
jochen@chromium.org changed reviewers: + esprehn@chromium.org, haraken@chromium.org, skyostil@chromium.org
should we use that API directly? should this be something the scheduler knows about? if we go with this API, can you also update the implementation in components/html_viewer https://codereview.chromium.org/1519863002/diff/20001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebBackgroundTask.h (right): https://codereview.chromium.org/1519863002/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebBackgroundTask.h:15: ShortRunningTask, this should be TaskSizeShortRunningTask or similar (starting with the enum's name is the key)
Description was changed from ========== Expose WorkerPool to blink Adding an API to blink::Platform to allow blink to submit background tasks to chromium's worker thread pool. The plan is to use this for HTMLCanvasElement::toBlob BUG=567887 ========== to ========== Expose WorkerPool to blink Adding an API to blink::Platform to allow blink to submit background tasks to chromium's worker thread pool. The plan is to use this for HTMLCanvasElement::toBlob BUG=567887 ==========
junov@chromium.org changed reviewers: + gab@chromium.org
junov@chromium.org changed reviewers: + tansell@chromium.org
On 2015/12/14 12:12:15, jochen wrote: > should we use that API directly? > > should this be something the scheduler knows about? > I don't know. I just want to be able to use this in blink. gab, tansell: guidance?
On 2015/12/14 16:27:27, Justin Novosad wrote: > On 2015/12/14 12:12:15, jochen wrote: > > should we use that API directly? > > > > should this be something the scheduler knows about? > > > > I don't know. I just want to be able to use this in blink. > gab, tansell: guidance? AFAIK the Blink scheduler is still only a scheduler for the main thread. A process-wide scheduler is coming to the browser soon and should be able to be ported to the renderer later, but in the meantime I think this approach is reasonable (we'll merely have to redirect the impl of BlinkPlatformImpl::postTaskOnBackgroundThread() when a better option becomes available). Should also double-check with the ServiceWorker team in Tokyo as they also were talking about needing a thread pool in the renderer (I'll add them to the bug for discussion).
We can use base in platform now, just add a WorkerPool class to platform with static methods that does what you want, you don't need the virtual abstraction like this and the plumbing. https://codereview.chromium.org/1519863002/diff/20001/content/child/blink_pla... File content/child/blink_platform_impl.cc (right): https://codereview.chromium.org/1519863002/diff/20001/content/child/blink_pla... content/child/blink_platform_impl.cc:582: base::WorkerPool::PostTask( this method is static, just implement this directly in platform, don't plumb it out like this. https://codereview.chromium.org/1519863002/diff/20001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebBackgroundTask.h (right): https://codereview.chromium.org/1519863002/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebBackgroundTask.h:12: class BLINK_PLATFORM_EXPORT WebBackgroundTask { hmm, this needs to be thread safe, that's a little scary, shouldn't this use the ThreadSafe stuff from WTF that ensures threadSafe callbacks?
On 2015/12/16 00:04:19, esprehn wrote: > We can use base in platform now, just add a WorkerPool class to platform with > static methods that does what you want, you don't need the virtual abstraction > like this and the plumbing. I was wondering about this. Onion soup FTW!
https://codereview.chromium.org/1519863002/diff/20001/content/child/blink_pla... File content/child/blink_platform_impl.cc (right): https://codereview.chromium.org/1519863002/diff/20001/content/child/blink_pla... content/child/blink_platform_impl.cc:582: base::WorkerPool::PostTask( On 2015/12/16 00:04:18, esprehn wrote: > this method is static, just implement this directly in platform, don't plumb it > out like this. I tried doing this and it has issues. We still have a checkdeps rule that says stuff under third_party cannot include stuff from base.
That's not true, we have base/ usage in a growing number of places in blink. :) Did you forget to update the DEPS file in platform/ ? -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/12/16 19:04:33, esprehn wrote: > That's not true, we have base/ usage in a growing number of places in > blink. :) > > Did you forget to update the DEPS file in platform/ ? I rebase onto ToT a few minutes ago. To be clear, are you talking about Source/platform or public/platform. I put my stuff in Source/platform
That's not true, we have base/ usage in a growing number of places in blink. :) Did you forget to update the DEPS file in platform/ ? -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
New patch
junov@chromium.org changed reviewers: + danakj@chromium.org
+danakj for base/OWNERS apparently adding DEPS requires an owner of the dependency to approve.
base DEPS LGTM https://codereview.chromium.org/1519863002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/DEPS (right): https://codereview.chromium.org/1519863002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/DEPS:2: "+base", if you're adding base/ you can remove the base/foo/ subdirs
On 2015/12/16 19:41:59, danakj (behind on reviews) wrote: > if you're adding base/ you can remove the base/foo/ subdirs Done.
lgtm https://codereview.chromium.org/1519863002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/BackgroundTaskTest.cpp (right): https://codereview.chromium.org/1519863002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/BackgroundTaskTest.cpp:16: class PingTask : public BackgroundTask { wtf_make_noncopyable
The CQ bit was checked by junov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, jochen@chromium.org Link to the patchset: https://codereview.chromium.org/1519863002/#ps80001 (title: "make non copyable")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1519863002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1519863002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by junov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/1519863002/#ps100001 (title: "build fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1519863002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1519863002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by junov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/1519863002/#ps120001 (title: "fix build for realz")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1519863002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1519863002/120001
The CQ bit was unchecked by esprehn@chromium.org
Description was changed from ========== Expose WorkerPool to blink Adding an API to blink::Platform to allow blink to submit background tasks to chromium's worker thread pool. The plan is to use this for HTMLCanvasElement::toBlob BUG=567887 ========== to ========== Expose WorkerPool to blink Adding an API to blink's platform to allow blink to submit background tasks to chromium's worker thread pool. The plan is to use this for HTMLCanvasElement::toBlob BUG=567887 ==========
The CQ bit was checked by esprehn@chromium.org
The CQ bit was unchecked by esprehn@chromium.org
https://codereview.chromium.org/1519863002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/BackgroundTask.cpp (right): https://codereview.chromium.org/1519863002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/BackgroundTask.cpp:11: #include "public/platform/Platform.h" Why do you need Platform? https://codereview.chromium.org/1519863002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/BackgroundTask.h (right): https://codereview.chromium.org/1519863002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/BackgroundTask.h:25: virtual void run() = 0; You should talk to kinuko, I think want tasks to be annotated with cross thread copier things. But this is probably okay to start.
On 2015/12/17 19:17:27, esprehn wrote: > https://codereview.chromium.org/1519863002/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/BackgroundTask.cpp (right): > > https://codereview.chromium.org/1519863002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/BackgroundTask.cpp:11: #include > "public/platform/Platform.h" > Why do you need Platform? Done. > > https://codereview.chromium.org/1519863002/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/BackgroundTask.h (right): > > https://codereview.chromium.org/1519863002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/BackgroundTask.h:25: virtual void run() = 0; > You should talk to kinuko, I think want tasks to be annotated with cross thread > copier things. But this is probably okay to start. I reached out. Will follow-up if needed.
The CQ bit was checked by junov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/1519863002/#ps140001 (title: "remove unnecessary include")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1519863002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1519863002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
LGTM /me curious: can we kill a task after it has been posted to background thread? Not sure of the answer myself.
On 2015/12/18 01:37:46, noel gordon wrote: > LGTM > > /me curious: can we kill a task after it has been posted to background thread? > Not sure of the answer myself. There is no direct cancellation provided by the scheduler. The approach Olivia is implementing for toBlob uses a cancellation flag that can be set from the main thread and causes the task to abort. The flag is checked at each scanline iteration of the encoding task, so we can avoid running to completion when the task is cancelled after it has already started.
The CQ bit was checked by junov@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1519863002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1519863002/140001
The CQ bit was unchecked by junov@chromium.org
kinuko@chromium.org changed reviewers: + kinuko@chromium.org
https://codereview.chromium.org/1519863002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/BackgroundTask.cpp (right): https://codereview.chromium.org/1519863002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/BackgroundTask.cpp:19: base::WorkerPool::PostTask(FROM_HERE, base::Bind(&BackgroundTask::run, base::Owned(task)), taskSize == TaskSizeLongRunningTask); Given that the task's going to run on a different thread from a pool we probably want to do something like this: https://codereview.chromium.org/807423002 https://codereview.chromium.org/1519863002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/BackgroundTask.h (right): https://codereview.chromium.org/1519863002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/BackgroundTask.h:13: class PLATFORM_EXPORT BackgroundTask { Why do we need to introduce a new base task class? Could this just take a closure so that we can use threadSafeBind to make sure we safely deep-copy parameters for the task when needed?
I applied the corrections. @kinuko: I think I get it, but I am not 100% sure. Please check that I properly addressed the thread safety issue.
Thanks! The code looks good now, I have a few more comments. https://codereview.chromium.org/1519863002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/BackgroundTaskRunner.cpp (right): https://codereview.chromium.org/1519863002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/BackgroundTaskRunner.cpp:21: base::WorkerPool::PostTask(FROM_HERE, base::Bind(&RunBackgroundTask, closure), taskSize == TaskSizeLongRunningTask); Hmm well ok passing PassOwnPtr to base::Bind works in a somewhat expected way... https://codereview.chromium.org/1519863002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/BackgroundTaskRunner.h (right): https://codereview.chromium.org/1519863002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/BackgroundTaskRunner.h:14: class PLATFORM_EXPORT BackgroundTaskRunner { I suppose we don't need to expose this? https://codereview.chromium.org/1519863002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/BackgroundTaskRunner.h:16: BackgroundTaskRunner() { } STATIC_ONLY(BackgroundTaskRunner) ? (Or change this class to namespace per recent discussion on blink-dev) https://codereview.chromium.org/1519863002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/DEPS (right): https://codereview.chromium.org/1519863002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/DEPS:2: "+base", This worries me a bit-- the onion soup doc says base deps can be added through whitelist, but this allows everything at once. Could we still allow things a bit more slowly like this? (What other reviewers think?) "+base/bind.h" "+base/location.h" If this list gets too bigger we can start considering adding "+base". Actually I even want to disallow bind.h to be included other than in BackgroundTaskRunner.cpp for now, would it make sense to create a subdirectory like platform/threading and have the new files for WorkerPool in it, add its own DEPS to allow bind.h?
https://codereview.chromium.org/1519863002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/DEPS (right): https://codereview.chromium.org/1519863002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/DEPS:2: "+base", On 2015/12/18 15:21:30, kinuko wrote: > This worries me a bit-- the onion soup doc says base deps can be added through > whitelist, but this allows everything at once. Could we still allow things a bit > more slowly like this? (What other reviewers think?) > > "+base/bind.h" > "+base/location.h" > > If this list gets too bigger we can start considering adding "+base". > > Actually I even want to disallow bind.h to be included other than in > BackgroundTaskRunner.cpp for now, would it make sense to create a subdirectory > like platform/threading and have the new files for WorkerPool in it, add its own > DEPS to allow bind.h? I'd prefer whitelisting header files. Elliott?
https://codereview.chromium.org/1519863002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/BackgroundTaskRunner.h (right): https://codereview.chromium.org/1519863002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/BackgroundTaskRunner.h:14: class PLATFORM_EXPORT BackgroundTaskRunner { On 2015/12/18 15:21:30, kinuko wrote: > I suppose we don't need to expose this? The next CL is going to call this from core. https://codereview.chromium.org/1519863002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/BackgroundTaskRunner.h:16: BackgroundTaskRunner() { } On 2015/12/18 15:21:30, kinuko wrote: > STATIC_ONLY(BackgroundTaskRunner) ? > > (Or change this class to namespace per recent discussion on blink-dev) Acknowledged. https://codereview.chromium.org/1519863002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/DEPS (right): https://codereview.chromium.org/1519863002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/DEPS:2: "+base", On 2015/12/18 15:30:44, haraken wrote: > On 2015/12/18 15:21:30, kinuko wrote: > > This worries me a bit-- the onion soup doc says base deps can be added through > > whitelist, but this allows everything at once. Could we still allow things a > bit > > more slowly like this? (What other reviewers think?) > > > > "+base/bind.h" > > "+base/location.h" > > > > If this list gets too bigger we can start considering adding "+base". > > > > Actually I even want to disallow bind.h to be included other than in > > BackgroundTaskRunner.cpp for now, would it make sense to create a subdirectory > > like platform/threading and have the new files for WorkerPool in it, add its > own > > DEPS to allow bind.h? > > I'd prefer whitelisting header files. > > Elliott? Acknowledged.
New patch.
Thanks, lgtm https://codereview.chromium.org/1519863002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/threading/DEPS (right): https://codereview.chromium.org/1519863002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/threading/DEPS:4: "+base/threading", nit: base/location.h can probably be in the parent dir's DEPS (we can widen these whenever necessary so I think this is fine too)
The CQ bit was checked by junov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, jochen@chromium.org, noel@chromium.org, kinuko@chromium.org Link to the patchset: https://codereview.chromium.org/1519863002/#ps200001 (title: "DEPS tweak")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1519863002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1519863002/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Expose WorkerPool to blink Adding an API to blink's platform to allow blink to submit background tasks to chromium's worker thread pool. The plan is to use this for HTMLCanvasElement::toBlob BUG=567887 ========== to ========== Expose WorkerPool to blink Adding an API to blink's platform to allow blink to submit background tasks to chromium's worker thread pool. The plan is to use this for HTMLCanvasElement::toBlob BUG=567887 Committed: https://crrev.com/5a3b91b21b97dcd68af85b06d1dd143571e596e3 Cr-Commit-Position: refs/heads/master@{#366251} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/5a3b91b21b97dcd68af85b06d1dd143571e596e3 Cr-Commit-Position: refs/heads/master@{#366251} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
