Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2628773003:
Add REAL_TASK_SCHEDULER option in TestBrowserThreadBundle. (Closed)
DescriptionAdd REAL_TASK_SCHEDULER option in TestBrowserThreadBundle.
With this option, TaskScheduler tasks don't run on the main thread.
This is a pre-requisite for the migration of v8 from WorkerPool to
TaskScheduler. v8 currently posts tasks to WorkerPool and waits for
them from the main thread using a WaitableEvent. That doesn't work
if the tasks have to run on the main thread.
BUG=659191
Review-Url: https://codereview.chromium.org/2628773003
Cr-Commit-Position: refs/heads/master@{#443737}
Committed: https://chromium.googlesource.com/chromium/src/+/aedd3c2c977646f2fd410c8a21a107b77e9df1b8
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : fix build error #
Total comments: 10
Patch Set 4 : rebase #Patch Set 5 : CR gab/robliao #15-16 #
Total comments: 2
Patch Set 6 : CR robliao #21 #
Total comments: 2
Patch Set 7 : CR sky #30 #
Depends on Patchset: Messages
Total messages: 41 (28 generated)
The CQ bit was checked by fdoray@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_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by fdoray@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_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by fdoray@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: This issue passed the CQ dry run.
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
PTAL
lgtm w/ nits https://codereview.chromium.org/2628773003/diff/40001/content/public/test/tes... File content/public/test/test_browser_thread_bundle.h (right): https://codereview.chromium.org/2628773003/diff/40001/content/public/test/tes... content/public/test/test_browser_thread_bundle.h:30: // justified as it is easier to understand and debug a single-threaded test. s/test/unit test/ https://codereview.chromium.org/2628773003/diff/40001/content/public/test/tes... content/public/test/test_browser_thread_bundle.h:71: DONT_CREATE_THREADS = 0x20, Change 0xFF syntax to 1 << 0 1 << 1 1 << 2 1 << 3 ... easier to parse and make no mistakes.
https://codereview.chromium.org/2628773003/diff/40001/content/public/test/tes... File content/public/test/test_browser_thread_bundle.cc (right): https://codereview.chromium.org/2628773003/diff/40001/content/public/test/tes... content/public/test/test_browser_thread_bundle.cc:93: new base::test::ScopedAsyncTaskScheduler()); Should Async instead have been an option off of ScopedTaskScheduler? https://codereview.chromium.org/2628773003/diff/40001/content/public/test/tes... File content/public/test/test_browser_thread_bundle.h (right): https://codereview.chromium.org/2628773003/diff/40001/content/public/test/tes... content/public/test/test_browser_thread_bundle.h:28: // If a test needs a TaskScheduler that doesn't run its tasks on the main Nit: s/that doesn't run its tasks on the main thread/runs tasks in a threadpool/ https://codereview.chromium.org/2628773003/diff/40001/content/public/test/tes... content/public/test/test_browser_thread_bundle.h:71: DONT_CREATE_THREADS = 0x20, On 2017/01/12 19:50:27, gab wrote: > Change 0xFF syntax to > > 1 << 0 > 1 << 1 > 1 << 2 > 1 << 3 > ... > > easier to parse and make no mistakes. +1
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
robliao@: PTAnL https://codereview.chromium.org/2628773003/diff/40001/content/public/test/tes... File content/public/test/test_browser_thread_bundle.cc (right): https://codereview.chromium.org/2628773003/diff/40001/content/public/test/tes... content/public/test/test_browser_thread_bundle.cc:93: new base::test::ScopedAsyncTaskScheduler()); On 2017/01/12 20:15:02, robliao wrote: > Should Async instead have been an option off of ScopedTaskScheduler? No. It is very convenient to be able to add a ScopedAsyncTaskScheduler member to a test fixture instead of a ScopedTaskScheduler member + initialization with a custom flag in the constructor. It is also easier to find ScopedTaskScheduler vs. ScopedAsyncTaskSchedulers in code search with different class names. https://codereview.chromium.org/2628773003/diff/40001/content/public/test/tes... File content/public/test/test_browser_thread_bundle.h (right): https://codereview.chromium.org/2628773003/diff/40001/content/public/test/tes... content/public/test/test_browser_thread_bundle.h:28: // If a test needs a TaskScheduler that doesn't run its tasks on the main On 2017/01/12 20:15:02, robliao wrote: > Nit: s/that doesn't run its tasks on the main thread/runs tasks in a threadpool/ Done. (dedicated thread instead of threadpool because tasks will run in a pool of 1 thread) https://codereview.chromium.org/2628773003/diff/40001/content/public/test/tes... content/public/test/test_browser_thread_bundle.h:30: // justified as it is easier to understand and debug a single-threaded test. On 2017/01/12 19:50:27, gab wrote: > s/test/unit test/ Done. https://codereview.chromium.org/2628773003/diff/40001/content/public/test/tes... content/public/test/test_browser_thread_bundle.h:71: DONT_CREATE_THREADS = 0x20, On 2017/01/12 20:15:02, robliao wrote: > On 2017/01/12 19:50:27, gab wrote: > > Change 0xFF syntax to > > > > 1 << 0 > > 1 << 1 > > 1 << 2 > > 1 << 3 > > ... > > > > easier to parse and make no mistakes. > > +1 Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2628773003/diff/40001/content/public/test/tes... File content/public/test/test_browser_thread_bundle.cc (right): https://codereview.chromium.org/2628773003/diff/40001/content/public/test/tes... content/public/test/test_browser_thread_bundle.cc:93: new base::test::ScopedAsyncTaskScheduler()); On 2017/01/12 21:25:56, fdoray wrote: > On 2017/01/12 20:15:02, robliao wrote: > > Should Async instead have been an option off of ScopedTaskScheduler? > > No. It is very convenient to be able to add a ScopedAsyncTaskScheduler member to > a test fixture instead of a ScopedTaskScheduler member + initialization with a > custom flag in the constructor. It is also easier to find ScopedTaskScheduler > vs. ScopedAsyncTaskSchedulers in code search with different class names. Do we expect many uses of ScopedAsyncTaskScheduler? My reading is that we want folks to use ScopedTaskScheduler and only used ScopedAsyncTaskScheduler sparingly. If that's the case, there are likely going to be very few users of this, so I would be fine with those folks going through the extra hoop of initializing the member with the async option in their constructor. Search should be easy as we can look for the use of the option.
lgtm in the meantime. https://codereview.chromium.org/2628773003/diff/80001/content/public/test/tes... File content/public/test/test_browser_thread_bundle.h (right): https://codereview.chromium.org/2628773003/diff/80001/content/public/test/tes... content/public/test/test_browser_thread_bundle.h:66: IO_MAINLOOP = 1 << 0, While we're here, might be worthwhile to comment that 1 << 1 is unused. Otherwise, it looks like a bug. Reference: 1 << 1 used to be REAL_WEBKIT_DEPRECATED_THREAD. https://codereview.chromium.org/18414007/diff/15001/content/public/test/test_... Given this is just test code, it might be okay to just reenumerate this, but that would be beyond the scope of this CL. I'll leave it up to you.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
fdoray@chromium.org changed reviewers: + sky@chromium.org
sky@: PTAL https://codereview.chromium.org/2628773003/diff/80001/content/public/test/tes... File content/public/test/test_browser_thread_bundle.h (right): https://codereview.chromium.org/2628773003/diff/80001/content/public/test/tes... content/public/test/test_browser_thread_bundle.h:66: IO_MAINLOOP = 1 << 0, On 2017/01/12 21:36:01, robliao wrote: > While we're here, might be worthwhile to comment that 1 << 1 is unused. > Otherwise, it looks like a bug. > > Reference: 1 << 1 used to be REAL_WEBKIT_DEPRECATED_THREAD. > https://codereview.chromium.org/18414007/diff/15001/content/public/test/test_... > > Given this is just test code, it might be okay to just reenumerate this, but > that would be beyond the scope of this CL. I'll leave it up to you. Done. Reenumerated. There is no good reason not to reuse the bit previously used by REAL_WEBKIT_DEPRECATED_THREAD.
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: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2628773003/diff/100001/content/public/test/te... File content/public/test/test_browser_thread_bundle.cc (right): https://codereview.chromium.org/2628773003/diff/100001/content/public/test/te... content/public/test/test_browser_thread_bundle.cc:95: scoped_async_task_scheduler_.reset( MakeUnique?
https://codereview.chromium.org/2628773003/diff/100001/content/public/test/te... File content/public/test/test_browser_thread_bundle.cc (right): https://codereview.chromium.org/2628773003/diff/100001/content/public/test/te... content/public/test/test_browser_thread_bundle.cc:95: scoped_async_task_scheduler_.reset( On 2017/01/13 15:27:47, sky wrote: > MakeUnique? Done.
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, robliao@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2628773003/#ps120001 (title: "CR sky #30")
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
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by fdoray@chromium.org
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": 120001, "attempt_start_ts": 1484348666002530,
"parent_rev": "6c5bf8fac0cd765eb4152cb3b7c8f080d02bce2f", "commit_rev":
"aedd3c2c977646f2fd410c8a21a107b77e9df1b8"}
Message was sent while issue was closed.
Description was changed from ========== Add REAL_TASK_SCHEDULER option in TestBrowserThreadBundle. With this option, TaskScheduler tasks don't run on the main thread. This is a pre-requisite for the migration of v8 from WorkerPool to TaskScheduler. v8 currently posts tasks to WorkerPool and waits for them from the main thread using a WaitableEvent. That doesn't work if the tasks have to run on the main thread. BUG=659191 ========== to ========== Add REAL_TASK_SCHEDULER option in TestBrowserThreadBundle. With this option, TaskScheduler tasks don't run on the main thread. This is a pre-requisite for the migration of v8 from WorkerPool to TaskScheduler. v8 currently posts tasks to WorkerPool and waits for them from the main thread using a WaitableEvent. That doesn't work if the tasks have to run on the main thread. BUG=659191 Review-Url: https://codereview.chromium.org/2628773003 Cr-Commit-Position: refs/heads/master@{#443737} Committed: https://chromium.googlesource.com/chromium/src/+/aedd3c2c977646f2fd410c8a21a1... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/aedd3c2c977646f2fd410c8a21a1... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
