|
|
Created:
3 years, 7 months ago by Sébastien Marchand Modified:
3 years, 5 months ago CC:
chromium-reviews, tfarina, browser-components-watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSwitch BookmarkFaviconFetcher to the TaskScheduler API.
BUG=689520
Review-Url: https://codereview.chromium.org/2881143002
Cr-Commit-Position: refs/heads/master@{#488747}
Committed: https://chromium.googlesource.com/chromium/src/+/0c5896f54772633e34c7fa4b7ec3450d93b8cc11
Patch Set 1 #Patch Set 2 : Switch BookmarkFaviconFetcher to the TaskScheduler API. #Patch Set 3 : Switch to a SingleThreadTaskRunner #
Total comments: 6
Patch Set 4 : Use a SequencedTaskRunner #
Total comments: 2
Patch Set 5 : Remove useless include #
Messages
Total messages: 51 (33 generated)
Description was changed from ========== Switch BookmarkFaviconFetcher to the TaskScheduler API. BUG= ========== to ========== Switch BookmarkFaviconFetcher to the TaskScheduler API. BUG=689520 ==========
fdoray@chromium.org changed reviewers: + fdoray@chromium.org
lgtm
The CQ bit was checked by sebmarchand@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...
sebmarchand@chromium.org changed reviewers: + sky@chromium.org
PTAL.
sky: Please hold on before reviewing, some tests are apparently failing under some configurations, investigating.
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_...)
The CQ bit was checked by sebmarchand@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_...)
Description was changed from ========== Switch BookmarkFaviconFetcher to the TaskScheduler API. BUG=689520 ========== to ========== Switch BookmarkFaviconFetcher to the TaskScheduler API. Blocked by crbug.com/722537 BUG=689520 ==========
sebmarchand@chromium.org changed reviewers: - sky@chromium.org
The CQ bit was checked by sebmarchand@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 sebmarchand@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...
Description was changed from ========== Switch BookmarkFaviconFetcher to the TaskScheduler API. Blocked by crbug.com/722537 BUG=689520 ========== to ========== Switch BookmarkFaviconFetcher to the TaskScheduler API. BUG=689520 ==========
sebmarchand@chromium.org changed reviewers: + sky@chromium.org
sky: PTAL now, the blocking bug has been fixed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2881143002/diff/40001/chrome/browser/bookmark... File chrome/browser/bookmarks/bookmark_html_writer.cc (right): https://codereview.chromium.org/2881143002/diff/40001/chrome/browser/bookmark... chrome/browser/bookmarks/bookmark_html_writer.cc:454: scoped_refptr<base::SingleThreadTaskRunner> task_runner_ = Forgive my ignorance, but what happens if ExecuteWriter is called twice in a row. Might they both end up on separate background threads?
sebmarchand@chromium.org changed reviewers: + robliao@chromium.org
https://codereview.chromium.org/2881143002/diff/40001/chrome/browser/bookmark... File chrome/browser/bookmarks/bookmark_html_writer.cc (right): https://codereview.chromium.org/2881143002/diff/40001/chrome/browser/bookmark... chrome/browser/bookmarks/bookmark_html_writer.cc:454: scoped_refptr<base::SingleThreadTaskRunner> task_runner_ = On 2017/05/22 16:40:46, sky wrote: > Forgive my ignorance, but what happens if ExecuteWriter is called twice in a > row. Might they both end up on separate background threads? +fdoray@/robliao@ as I'm not entirely sure, one solution would be to add a "SequencedTaskRunner" member to this class and to use it to run these tasks. (and otherwise I should remove the trailing underscore if I stick with this solution)
https://codereview.chromium.org/2881143002/diff/40001/chrome/browser/bookmark... File chrome/browser/bookmarks/bookmark_html_writer.cc (right): https://codereview.chromium.org/2881143002/diff/40001/chrome/browser/bookmark... chrome/browser/bookmarks/bookmark_html_writer.cc:454: scoped_refptr<base::SingleThreadTaskRunner> task_runner_ = On 2017/05/22 17:19:17, Sébastien Marchand wrote: > On 2017/05/22 16:40:46, sky wrote: > > Forgive my ignorance, but what happens if ExecuteWriter is called twice in a > > row. Might they both end up on separate background threads? > > +fdoray@/robliao@ as I'm not entirely sure, one solution would be to add a > "SequencedTaskRunner" member to this class and to use it to run these tasks. > (and otherwise I should remove the trailing underscore if I stick with this > solution) As coded here, if ExecuteRunner is executed twice in a row, then the code will be executed on separate background threads. If this is undesirable BookmarkFaviconFetcher will need to hold on to a task runner for future use. If this is okay, instead the code should just post a parallel task since the task runner is immediately thrown away. Nit: task_runner_ -> task_runner
https://codereview.chromium.org/2881143002/diff/40001/chrome/browser/bookmark... File chrome/browser/bookmarks/bookmark_html_writer.cc (right): https://codereview.chromium.org/2881143002/diff/40001/chrome/browser/bookmark... chrome/browser/bookmarks/bookmark_html_writer.cc:454: scoped_refptr<base::SingleThreadTaskRunner> task_runner_ = On 2017/05/22 17:27:25, robliao wrote: > On 2017/05/22 17:19:17, Sébastien Marchand wrote: > > On 2017/05/22 16:40:46, sky wrote: > > > Forgive my ignorance, but what happens if ExecuteWriter is called twice in a > > > row. Might they both end up on separate background threads? > > > > +fdoray@/robliao@ as I'm not entirely sure, one solution would be to add a > > "SequencedTaskRunner" member to this class and to use it to run these tasks. > > (and otherwise I should remove the trailing underscore if I stick with this > > solution) > > As coded here, if ExecuteRunner is executed twice in a row, then the code will > be executed on separate background threads. > > If this is undesirable BookmarkFaviconFetcher will need to hold on to a task > runner for future use. > > If this is okay, instead the code should just post a parallel task since the > task runner is immediately thrown away. > > Nit: task_runner_ -> task_runner While likely rare, we should make sure there aren't two running at the same time.
sebmarchand@chromium.org changed reviewers: + gab@chromium.org
+gab@ and fdoray@ fyi. https://codereview.chromium.org/2881143002/diff/40001/chrome/browser/bookmark... File chrome/browser/bookmarks/bookmark_html_writer.cc (right): https://codereview.chromium.org/2881143002/diff/40001/chrome/browser/bookmark... chrome/browser/bookmarks/bookmark_html_writer.cc:454: scoped_refptr<base::SingleThreadTaskRunner> task_runner_ = On 2017/05/22 17:33:55, sky wrote: > On 2017/05/22 17:27:25, robliao wrote: > > On 2017/05/22 17:19:17, Sébastien Marchand wrote: > > > On 2017/05/22 16:40:46, sky wrote: > > > > Forgive my ignorance, but what happens if ExecuteWriter is called twice in > a > > > > row. Might they both end up on separate background threads? > > > > > > +fdoray@/robliao@ as I'm not entirely sure, one solution would be to add a > > > "SequencedTaskRunner" member to this class and to use it to run these tasks. > > > (and otherwise I should remove the trailing underscore if I stick with this > > > solution) > > > > As coded here, if ExecuteRunner is executed twice in a row, then the code will > > be executed on separate background threads. > > > > If this is undesirable BookmarkFaviconFetcher will need to hold on to a task > > runner for future use. > > > > If this is okay, instead the code should just post a parallel task since the > > task runner is immediately thrown away. > > > > Nit: task_runner_ -> task_runner > > While likely rare, we should make sure there aren't two running at the same > time. One way to do this would be to add a SequencedTaskRunner member to this class and to use it to run these tasks in sequence, this'll work if we assume that this class is used as a singleton (otherwise 2 different instances will use a separate task runner and might run in parallel). This isn't really the case, this class could be instantiated directly or used via its global instance (via WriteBookmarks). There doesn't seem to be any synchronization around |g_fetcher|, WriteBookmarks create it if needed and this function destroys it, this could be racy if 2 separate threads call WriteBookmarks. Another option could be to use a single LazyInstance<scoped_refptr<SequencedTaskRunner>> instance in this file, so all the write operations will use the same task runner. This is probably the safest approach for now. One of the goal of this CL was to dogfood the new task scheduler API, I think that this class is kind of hostile to task runners as is and this is probably good feedback to the task scheduler team.
https://codereview.chromium.org/2881143002/diff/40001/chrome/browser/bookmark... File chrome/browser/bookmarks/bookmark_html_writer.cc (right): https://codereview.chromium.org/2881143002/diff/40001/chrome/browser/bookmark... chrome/browser/bookmarks/bookmark_html_writer.cc:454: scoped_refptr<base::SingleThreadTaskRunner> task_runner_ = On 2017/05/23 16:56:48, Sébastien Marchand wrote: > On 2017/05/22 17:33:55, sky wrote: > > On 2017/05/22 17:27:25, robliao wrote: > > > On 2017/05/22 17:19:17, Sébastien Marchand wrote: > > > > On 2017/05/22 16:40:46, sky wrote: > > > > > Forgive my ignorance, but what happens if ExecuteWriter is called twice > in > > a > > > > > row. Might they both end up on separate background threads? > > > > > > > > +fdoray@/robliao@ as I'm not entirely sure, one solution would be to add a > > > > "SequencedTaskRunner" member to this class and to use it to run these > tasks. > > > > (and otherwise I should remove the trailing underscore if I stick with > this > > > > solution) > > > > > > As coded here, if ExecuteRunner is executed twice in a row, then the code > will > > > be executed on separate background threads. > > > > > > If this is undesirable BookmarkFaviconFetcher will need to hold on to a task > > > runner for future use. > > > > > > If this is okay, instead the code should just post a parallel task since the > > > task runner is immediately thrown away. > > > > > > Nit: task_runner_ -> task_runner > > > > While likely rare, we should make sure there aren't two running at the same > > time. > > One way to do this would be to add a SequencedTaskRunner member to this class > and to use it to run these tasks in sequence, this'll work if we assume that > this class is used as a singleton (otherwise 2 different instances will use a > separate task runner and might run in parallel). This isn't really the case, > this class could be instantiated directly or used via its global instance (via > WriteBookmarks). > > There doesn't seem to be any synchronization around |g_fetcher|, WriteBookmarks > create it if needed and this function destroys it, this could be racy if 2 > separate threads call WriteBookmarks. > > Another option could be to use a single > LazyInstance<scoped_refptr<SequencedTaskRunner>> instance in this file, so all > the write operations will use the same task runner. This is probably the safest > approach for now. > > One of the goal of this CL was to dogfood the new task scheduler API, I think > that this class is kind of hostile to task runners as is and this is probably > good feedback to the task scheduler team. I agree with sky that although there's only one call in this file, multiple calls of it must be sequenced with each other. Looks like sequencing only needs to happen for same |path_| though so having a SequencedTaskRunner per BookmarkFaviconFetcher instance sounds like it would work?
On 2017/05/23 17:17:00, gab wrote: > https://codereview.chromium.org/2881143002/diff/40001/chrome/browser/bookmark... > File chrome/browser/bookmarks/bookmark_html_writer.cc (right): > > https://codereview.chromium.org/2881143002/diff/40001/chrome/browser/bookmark... > chrome/browser/bookmarks/bookmark_html_writer.cc:454: > scoped_refptr<base::SingleThreadTaskRunner> task_runner_ = > On 2017/05/23 16:56:48, Sébastien Marchand wrote: > > On 2017/05/22 17:33:55, sky wrote: > > > On 2017/05/22 17:27:25, robliao wrote: > > > > On 2017/05/22 17:19:17, Sébastien Marchand wrote: > > > > > On 2017/05/22 16:40:46, sky wrote: > > > > > > Forgive my ignorance, but what happens if ExecuteWriter is called > twice > > in > > > a > > > > > > row. Might they both end up on separate background threads? > > > > > > > > > > +fdoray@/robliao@ as I'm not entirely sure, one solution would be to add > a > > > > > "SequencedTaskRunner" member to this class and to use it to run these > > tasks. > > > > > (and otherwise I should remove the trailing underscore if I stick with > > this > > > > > solution) > > > > > > > > As coded here, if ExecuteRunner is executed twice in a row, then the code > > will > > > > be executed on separate background threads. > > > > > > > > If this is undesirable BookmarkFaviconFetcher will need to hold on to a > task > > > > runner for future use. > > > > > > > > If this is okay, instead the code should just post a parallel task since > the > > > > task runner is immediately thrown away. > > > > > > > > Nit: task_runner_ -> task_runner > > > > > > While likely rare, we should make sure there aren't two running at the same > > > time. > > > > One way to do this would be to add a SequencedTaskRunner member to this class > > and to use it to run these tasks in sequence, this'll work if we assume that > > this class is used as a singleton (otherwise 2 different instances will use a > > separate task runner and might run in parallel). This isn't really the case, > > this class could be instantiated directly or used via its global instance (via > > WriteBookmarks). > > > > There doesn't seem to be any synchronization around |g_fetcher|, > WriteBookmarks > > create it if needed and this function destroys it, this could be racy if 2 > > separate threads call WriteBookmarks. > > > > Another option could be to use a single > > LazyInstance<scoped_refptr<SequencedTaskRunner>> instance in this file, so all > > the write operations will use the same task runner. This is probably the > safest > > approach for now. > > > > One of the goal of this CL was to dogfood the new task scheduler API, I think > > that this class is kind of hostile to task runners as is and this is probably > > good feedback to the task scheduler team. > > I agree with sky that although there's only one call in this file, multiple > calls of it must be sequenced with each other. > > Looks like sequencing only needs to happen for same |path_| though so having a > SequencedTaskRunner per BookmarkFaviconFetcher instance sounds like it would > work? ping: solution is to have a scoped_refptr<base::SequencedTaskRunner> background_io_task_runner = base::CreateSequencedTaskRunnerWithTraits({base::MayBlock(), base::TaskPriority::BACKGROUND}); member and to post to it at the callsite you're currently posting from. I think that should work (might need a ScopedTaskEnvironment in matching unittests, ref: https://chromium.googlesource.com/chromium/src/+/master/docs/task_scheduler_m...). PS: https://crbug.com/676387 was fixed so you can try running from a sequenced instead of single-threaded context again, thanks!
The CQ bit was checked by sebmarchand@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.
Thanks, looks like I don't need a ScopedTaskEnvironment after all, PTAnL.
On 2017/07/20 16:39:29, Sébastien Marchand wrote: > Thanks, looks like I don't need a ScopedTaskEnvironment after all, PTAnL. Indeed you don't because TestBrowserThreadBundle already brings one in the affected test: TEST_F(BookmarkHTMLWriterTest, Test) { content::TestBrowserThreadBundle thread_bundle; (...) } lgtm w/ nit https://codereview.chromium.org/2881143002/diff/60001/chrome/browser/bookmark... File chrome/browser/bookmarks/bookmark_html_writer.cc (right): https://codereview.chromium.org/2881143002/diff/60001/chrome/browser/bookmark... chrome/browser/bookmarks/bookmark_html_writer.cc:17: #include "base/lazy_instance.h" unused
Thanks. sky@, do you want to take one last look at this? https://codereview.chromium.org/2881143002/diff/60001/chrome/browser/bookmark... File chrome/browser/bookmarks/bookmark_html_writer.cc (right): https://codereview.chromium.org/2881143002/diff/60001/chrome/browser/bookmark... chrome/browser/bookmarks/bookmark_html_writer.cc:17: #include "base/lazy_instance.h" On 2017/07/20 19:11:14, gab wrote: > unused Done.
LGTM
The CQ bit was checked by sebmarchand@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fdoray@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/2881143002/#ps80001 (title: "Remove useless include")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
CQ is committing da patch. Bot data: {"patchset_id": 80001, "attempt_start_ts": 1500664069876650, "parent_rev": "a267e20c2cd2ea22466288d67d58c7a39d3f45be", "commit_rev": "0c5896f54772633e34c7fa4b7ec3450d93b8cc11"}
Message was sent while issue was closed.
Description was changed from ========== Switch BookmarkFaviconFetcher to the TaskScheduler API. BUG=689520 ========== to ========== Switch BookmarkFaviconFetcher to the TaskScheduler API. BUG=689520 Review-Url: https://codereview.chromium.org/2881143002 Cr-Commit-Position: refs/heads/master@{#488747} Committed: https://chromium.googlesource.com/chromium/src/+/0c5896f54772633e34c7fa4b7ec3... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/0c5896f54772633e34c7fa4b7ec3... |