Migrate jumplist update task from FILE thread to base/task_scheduler

Migrate jumplist update task from FILE thread to base/task_scheduler

Currently, the jumplist update task is running on FILE thread, which can
hang the FILE thread so that no other tasks can be running on this thread 
during that time. The FILE thread has been deprecated and it is preferred
to base/task_scheduler/post_task.h for new classes requiring a background 
file I/O task runner. 

This CL does this migration for the jumplist update task.

BUG=40407, 179576
Review-Url: https://codereview.chromium.org/2779403002
Cr-Commit-Position: refs/heads/master@{#462280}
Committed: https://chromium.googlesource.com/chromium/src/+/971169a30d72887e285e03db2e524a6b3a15d3bb

[chengx, 2017-03-29 23:50:49]

The CQ bit was checked by chengx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 23:51:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chengx, 2017-03-30 01:31:25]

chengx@chromium.org changed reviewers:
+ gab@chromium.org

[chengx, 2017-03-30 01:31:26]

Hi Gabriel, I tried to migrate the delete task from FILE thread to
task_scheduler here, but got a crash when triggering the code locally. I have
put the crash details in "attempt 4" in the jumplist design doc. I have limited
knowledge on the task_scheduler, so can you please give me some help? Thanks!

[commit-bot: I haz the power, 2017-03-30 01:47:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 01:47:36]

Dry run: 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_...)

[chengx, 2017-03-30 17:48:47]

The CQ bit was checked by chengx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 17:49:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 19:46:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 19:46:33]

Dry run: 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_...)

[chengx, 2017-03-30 20:50:47]

The CQ bit was checked by chengx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 20:51:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chengx, 2017-03-30 21:50:48]

The CQ bit was checked by chengx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 21:51:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chengx, 2017-03-30 23:06:28]

Patchset #2 (id:20001) has been deleted

[chengx, 2017-03-30 23:07:08]

Patchset #2 (id:40001) has been deleted

[chengx, 2017-03-30 23:15:24]

The CQ bit was checked by chengx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 23:16:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chengx, 2017-03-30 23:27:48]

The CQ bit was checked by chengx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 23:28:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 01:28:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-31 01:28:16]

Dry run: 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_...)

[chengx, 2017-03-31 03:01:38]

Description was changed from

==========
Migrate jumplist delete tasks from FILE thread to base/task_scheduler.

BUG=40407
==========

to

==========
Migrate jumplist delete tasks from FILE thread to base/task_scheduler

BUG=40407
==========

[chengx, 2017-03-31 22:33:52]

The CQ bit was checked by chengx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 22:34:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-01 00:38:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-01 00:38:12]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[gab, 2017-04-03 14:25:10]

On 2017/03/30 01:31:26, chengx wrote:
> Hi Gabriel, I tried to migrate the delete task from FILE thread to
> task_scheduler here, but got a crash when triggering the code locally. I have
> put the crash details in "attempt 4" in the jumplist design doc. I have
limited
> knowledge on the task_scheduler, so can you please give me some help? Thanks!

One possible explanation is that creating the icons requires COM. The FILE
thread previously gave you COM for free. In TaskScheduler land you need to
CreateCOMSTATaskRunnerWithTraits() instead of
CreateSingleThreadTaskRunnerWithTraits() -- in which case you'll have to remove
the TODO to make it a SequencedTaskRunner as COM needs to be single-threaded :(.

PS: You shouldn't need TaskTraits::WithBaseSyncPrimitives().

@robliao: think COM can be the issue here? If so, is there a way we can make
such errors easier to diagnose (check in scope of Chrome's COM init when
initializing a ScopedComPtr or whatever its successor is?)

[chengx, 2017-04-03 20:50:25]

Patchset #3 (id:80001) has been deleted

[chengx, 2017-04-03 21:21:28]

The CQ bit was checked by chengx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 21:22:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-03 22:15:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-03 22:15:21]

Dry run: This issue passed the CQ dry run.

[chengx, 2017-04-03 23:42:40]

Patchset #3 (id:100001) has been deleted

[chengx, 2017-04-03 23:50:54]

Description was changed from

==========
Migrate jumplist delete tasks from FILE thread to base/task_scheduler

BUG=40407
==========

to

==========
Migrate jumplist update task from FILE thread to base/task_scheduler

Currently, the jumplist update task is running on FILE thread, which can
hang the FILE thread so that no other tasks can be running on this 
thread during that time. The FILE thread has been deprecated and it is 
preferred to base/task_scheduler/post_task.h for new classes requiring a 
background file I/O task runner. 

This CL does this migration for jumplist update task.

BUG=40407,179576
==========

[chengx, 2017-04-03 23:58:12]

Using CreateCOMSTATaskRunnerWithTraits() instead of
CreateSingleThreadTaskRunnerWithTraits() works! I think using some strategy to
make this easier to diagnose should help a lot. 

Thanks gab@, PTAL~

On 2017/04/03 14:25:10, gab wrote:
> On 2017/03/30 01:31:26, chengx wrote:
> > Hi Gabriel, I tried to migrate the delete task from FILE thread to
> > task_scheduler here, but got a crash when triggering the code locally. I
have
> > put the crash details in "attempt 4" in the jumplist design doc. I have
> limited
> > knowledge on the task_scheduler, so can you please give me some help?
Thanks!
> 
> One possible explanation is that creating the icons requires COM. The FILE
> thread previously gave you COM for free. In TaskScheduler land you need to
> CreateCOMSTATaskRunnerWithTraits() instead of
> CreateSingleThreadTaskRunnerWithTraits() -- in which case you'll have to
remove
> the TODO to make it a SequencedTaskRunner as COM needs to be single-threaded
:(.
> 
> PS: You shouldn't need TaskTraits::WithBaseSyncPrimitives().
> 
> @robliao: think COM can be the issue here? If so, is there a way we can make
> such errors easier to diagnose (check in scope of Chrome's COM init when
> initializing a ScopedComPtr or whatever its successor is?)

[chengx, 2017-04-04 03:06:38]

Description was changed from

==========
Migrate jumplist update task from FILE thread to base/task_scheduler

Currently, the jumplist update task is running on FILE thread, which can
hang the FILE thread so that no other tasks can be running on this 
thread during that time. The FILE thread has been deprecated and it is 
preferred to base/task_scheduler/post_task.h for new classes requiring a 
background file I/O task runner. 

This CL does this migration for jumplist update task.

BUG=40407,179576
==========

to

==========
Migrate jumplist update task from FILE thread to base/task_scheduler

Currently, the jumplist update task is running on FILE thread, which can
hang the FILE thread so that no other tasks can be running on this thread 
during that time. The FILE thread has been deprecated and it is preferred
to base/task_scheduler/post_task.h for new classes requiring a background 
file I/O task runner. 

This CL does this migration for jumplist update task.

BUG=40407,179576
==========

[chengx, 2017-04-04 03:07:05]

Description was changed from

==========
Migrate jumplist update task from FILE thread to base/task_scheduler

Currently, the jumplist update task is running on FILE thread, which can
hang the FILE thread so that no other tasks can be running on this thread 
during that time. The FILE thread has been deprecated and it is preferred
to base/task_scheduler/post_task.h for new classes requiring a background 
file I/O task runner. 

This CL does this migration for jumplist update task.

BUG=40407,179576
==========

to

==========
Migrate jumplist update task from FILE thread to base/task_scheduler

Currently, the jumplist update task is running on FILE thread, which can
hang the FILE thread so that no other tasks can be running on this thread 
during that time. The FILE thread has been deprecated and it is preferred
to base/task_scheduler/post_task.h for new classes requiring a background 
file I/O task runner. 

This CL does this migration for the jumplist update task.

BUG=40407,179576
==========

[gab, 2017-04-04 21:08:57]

On 2017/04/03 23:58:12, chengx wrote:
> Using CreateCOMSTATaskRunnerWithTraits() instead of
> CreateSingleThreadTaskRunnerWithTraits() works! I think using some strategy to
> make this easier to diagnose should help a lot. 

Cool, filed http://crbug.com/708303 to improve assertions when getting this
wrong.


> 
> Thanks gab@, PTAL~
> 
> On 2017/04/03 14:25:10, gab wrote:
> > On 2017/03/30 01:31:26, chengx wrote:
> > > Hi Gabriel, I tried to migrate the delete task from FILE thread to
> > > task_scheduler here, but got a crash when triggering the code locally. I
> have
> > > put the crash details in "attempt 4" in the jumplist design doc. I have
> > limited
> > > knowledge on the task_scheduler, so can you please give me some help?
> Thanks!
> > 
> > One possible explanation is that creating the icons requires COM. The FILE
> > thread previously gave you COM for free. In TaskScheduler land you need to
> > CreateCOMSTATaskRunnerWithTraits() instead of
> > CreateSingleThreadTaskRunnerWithTraits() -- in which case you'll have to
> remove
> > the TODO to make it a SequencedTaskRunner as COM needs to be single-threaded
> :(.
> > 
> > PS: You shouldn't need TaskTraits::WithBaseSyncPrimitives().
> > 
> > @robliao: think COM can be the issue here? If so, is there a way we can make
> > such errors easier to diagnose (check in scope of Chrome's COM init when
> > initializing a ScopedComPtr or whatever its successor is?)

[gab, 2017-04-04 21:11:46]

https://codereview.chromium.org/2779403002/diff/120001/chrome/browser/win/jum...
File chrome/browser/win/jumplist.cc (right):

https://codereview.chromium.org/2779403002/diff/120001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:616: FROM_HERE,
base::Bind(&DeleteDirectoryAndLogResults, icon_dir_old,
std::move(icon_dir_old)

and

#include <utilit>

for std::move

https://codereview.chromium.org/2779403002/diff/120001/chrome/browser/win/jum...
File chrome/browser/win/jumplist_file_util.cc (right):

https://codereview.chromium.org/2779403002/diff/120001/chrome/browser/win/jum...
chrome/browser/win/jumplist_file_util.cc:116:
UMA_HISTOGRAM_ENUMERATION("WinJumplist.DeleteStatusJumpListIconsOld",
Why do we need a separate histogram? Why isn't histograms.xml modified
accordingly?

[gab, 2017-04-04 21:12:06]

https://codereview.chromium.org/2779403002/diff/120001/chrome/browser/win/jum...
File chrome/browser/win/jumplist.cc (right):

https://codereview.chromium.org/2779403002/diff/120001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:616: FROM_HERE,
base::Bind(&DeleteDirectoryAndLogResults, icon_dir_old,
On 2017/04/04 21:11:46, gab wrote:
> std::move(icon_dir_old)
> 
> and
> 
> #include <utility>
> 
> for std::move

[chengx, 2017-04-05 00:20:58]

The CQ bit was checked by chengx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 00:22:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chengx, 2017-04-05 00:23:24]

PTAL~

https://codereview.chromium.org/2779403002/diff/120001/chrome/browser/win/jum...
File chrome/browser/win/jumplist.cc (right):

https://codereview.chromium.org/2779403002/diff/120001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:616: FROM_HERE,
base::Bind(&DeleteDirectoryAndLogResults, icon_dir_old,
On 2017/04/04 21:11:46, gab wrote:
> std::move(icon_dir_old)
> 
> and
> 
> #include <utilit>
> 
> for std::move

Done.

https://codereview.chromium.org/2779403002/diff/120001/chrome/browser/win/jum...
File chrome/browser/win/jumplist_file_util.cc (right):

https://codereview.chromium.org/2779403002/diff/120001/chrome/browser/win/jum...
chrome/browser/win/jumplist_file_util.cc:116:
UMA_HISTOGRAM_ENUMERATION("WinJumplist.DeleteStatusJumpListIconsOld",
On 2017/04/04 21:11:46, gab wrote:
> Why do we need a separate histogram? Why isn't histograms.xml modified
> accordingly?

JumpListIcons folder and JumpListIconsOld folder operation are now completely
independent of each other, and JumpListIconsOld will be gone eventually. Using
two histograms allows independent analysis of the two folder operations in a
much clearer way.

[robliao, 2017-04-05 00:27:50]

robliao@chromium.org changed reviewers:
+ robliao@chromium.org

[robliao, 2017-04-05 00:27:50]

Drive by!

https://codereview.chromium.org/2779403002/diff/140001/chrome/browser/win/jum...
File chrome/browser/win/jumplist.cc (right):

https://codereview.chromium.org/2779403002/diff/140001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:608: single_thread_task_runner_->PostTask(
Is this expected to be called a lot? If not, you can grab the
CreateCOMSTATaskRunnerWithTraits here on demand and avoid persisting a
single_thread_task_runner_.

[commit-bot: I haz the power, 2017-04-05 00:56:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-05 00:56:28]

Dry run: This issue passed the CQ dry run.

[chengx, 2017-04-05 03:25:50]

Hi Robert, please see my reply to your comments. Thanks!

https://codereview.chromium.org/2779403002/diff/140001/chrome/browser/win/jum...
File chrome/browser/win/jumplist.cc (right):

https://codereview.chromium.org/2779403002/diff/140001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:608: single_thread_task_runner_->PostTask(
On 2017/04/05 00:27:50, robliao wrote:
> Is this expected to be called a lot? If not, you can grab the
> CreateCOMSTATaskRunnerWithTraits here on demand and avoid persisting a
> single_thread_task_runner_.

It will be called each time a Chrome tab is closed. I think that's a lot.

[gab, 2017-04-05 20:53:27]

lgtm w/ comment

https://codereview.chromium.org/2779403002/diff/120001/chrome/browser/win/jum...
File chrome/browser/win/jumplist_file_util.cc (right):

https://codereview.chromium.org/2779403002/diff/120001/chrome/browser/win/jum...
chrome/browser/win/jumplist_file_util.cc:116:
UMA_HISTOGRAM_ENUMERATION("WinJumplist.DeleteStatusJumpListIconsOld",
On 2017/04/05 00:23:24, chengx wrote:
> On 2017/04/04 21:11:46, gab wrote:
> > Why do we need a separate histogram? Why isn't histograms.xml modified
> > accordingly?
> 
> JumpListIcons folder and JumpListIconsOld folder operation are now completely
> independent of each other, and JumpListIconsOld will be gone eventually. Using
> two histograms allows independent analysis of the two folder operations in a
> much clearer way.

I see, hadn't noticed the different enums, prefixing with
FolderDeleteResult::END could help there.

Also, new histograms.xml entry?

https://codereview.chromium.org/2779403002/diff/140001/chrome/browser/win/jum...
File chrome/browser/win/jumplist.cc (right):

https://codereview.chromium.org/2779403002/diff/140001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:608: single_thread_task_runner_->PostTask(
On 2017/04/05 03:25:50, chengx wrote:
> On 2017/04/05 00:27:50, robliao wrote:
> > Is this expected to be called a lot? If not, you can grab the
> > CreateCOMSTATaskRunnerWithTraits here on demand and avoid persisting a
> > single_thread_task_runner_.
> 
> It will be called each time a Chrome tab is closed. I think that's a lot. 

Yeah agreed that's a lot.

@robliao: I'm also seeing other single-threaded things that wouldn't mind
sharing a thread. Maybe we need to split our create methods to declare whether
you need exclusion or don't mind sharing...

[chengx, 2017-04-05 22:35:55]

The CQ bit was checked by chengx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 22:36:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chengx, 2017-04-05 22:40:27]

On 2017/04/05 20:53:27, gab wrote:
> lgtm w/ comment
> 
>
https://codereview.chromium.org/2779403002/diff/120001/chrome/browser/win/jum...
> File chrome/browser/win/jumplist_file_util.cc (right):
> 
>
https://codereview.chromium.org/2779403002/diff/120001/chrome/browser/win/jum...
> chrome/browser/win/jumplist_file_util.cc:116:
> UMA_HISTOGRAM_ENUMERATION("WinJumplist.DeleteStatusJumpListIconsOld",
> On 2017/04/05 00:23:24, chengx wrote:
> > On 2017/04/04 21:11:46, gab wrote:
> > > Why do we need a separate histogram? Why isn't histograms.xml modified
> > > accordingly?
> > 
> > JumpListIcons folder and JumpListIconsOld folder operation are now
completely
> > independent of each other, and JumpListIconsOld will be gone eventually.
Using
> > two histograms allows independent analysis of the two folder operations in a
> > much clearer way.
> 
> I see, hadn't noticed the different enums, prefixing with
> FolderDeleteResult::END could help there.
> 
> Also, new histograms.xml entry?

I changed to FolderDeleteResult::END. Agreed it helps readability. Yes, using
two histograms here is by design. Since we are not trying to delete all the
files inside the directory at one time, the delete status can't be used to infer
if the directory is empty or not. Therefore, these two histograms are almost
independent of each other.

[robliao, 2017-04-05 23:02:30]

lgtm.

https://codereview.chromium.org/2779403002/diff/140001/chrome/browser/win/jum...
File chrome/browser/win/jumplist.cc (right):

https://codereview.chromium.org/2779403002/diff/140001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:608: single_thread_task_runner_->PostTask(
On 2017/04/05 20:53:27, gab wrote:
> On 2017/04/05 03:25:50, chengx wrote:
> > On 2017/04/05 00:27:50, robliao wrote:
> > > Is this expected to be called a lot? If not, you can grab the
> > > CreateCOMSTATaskRunnerWithTraits here on demand and avoid persisting a
> > > single_thread_task_runner_.
> > 
> > It will be called each time a Chrome tab is closed. I think that's a lot. 
> 
> Yeah agreed that's a lot.
> 
> @robliao: I'm also seeing other single-threaded things that wouldn't mind
> sharing a thread. Maybe we need to split our create methods to declare whether
> you need exclusion or don't mind sharing...

Yeah. I'm pondering if the API should expose that kind of control to the caller
or not (e.g. will people know which one to choose).

We can discuss within the team.

[commit-bot: I haz the power, 2017-04-05 23:33:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-05 23:33:50]

Dry run: This issue passed the CQ dry run.

[chengx, 2017-04-05 23:34:16]

The CQ bit was checked by chengx@chromium.org

[chengx, 2017-04-05 23:34:17]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org
Link to the patchset: https://codereview.chromium.org/2779403002/#ps160001
(title: "Fix nits.")

[commit-bot: I haz the power, 2017-04-05 23:34:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 23:56:01]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1491435256836920,
"parent_rev": "9636d30c8546d025213d74d5c586d1ec4a6b36cb", "commit_rev":
"971169a30d72887e285e03db2e524a6b3a15d3bb"}

[commit-bot: I haz the power, 2017-04-05 23:56:50]

Description was changed from

==========
Migrate jumplist update task from FILE thread to base/task_scheduler

Currently, the jumplist update task is running on FILE thread, which can
hang the FILE thread so that no other tasks can be running on this thread 
during that time. The FILE thread has been deprecated and it is preferred
to base/task_scheduler/post_task.h for new classes requiring a background 
file I/O task runner. 

This CL does this migration for the jumplist update task.

BUG=40407,179576
==========

to

==========
Migrate jumplist update task from FILE thread to base/task_scheduler

Currently, the jumplist update task is running on FILE thread, which can
hang the FILE thread so that no other tasks can be running on this thread 
during that time. The FILE thread has been deprecated and it is preferred
to base/task_scheduler/post_task.h for new classes requiring a background 
file I/O task runner. 

This CL does this migration for the jumplist update task.

BUG=40407,179576

Review-Url: https://codereview.chromium.org/2779403002
Cr-Commit-Position: refs/heads/master@{#462280}
Committed:
https://chromium.googlesource.com/chromium/src/+/971169a30d72887e285e03db2e52...
==========

[commit-bot: I haz the power, 2017-04-05 23:56:53]

Committed patchset #5 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/971169a30d72887e285e03db2e52...

[chengx, 2017-04-10 17:53:05]

A revert of this CL (patchset #5 id:160001) has been created in
https://codereview.chromium.org/2810853002/ by chengx@chromium.org.

The reason for reverting is: Both the new/old Canary versions are running
RunUpdateOnFileThread callback function significantly slower suddenly. While
this means this CL is irrelevant, I think it still should be reverted to see
what happens. This allows further investigation. .

[gab, 2017-04-10 18:07:51]

On 2017/04/10 17:53:05, chengx wrote:
> A revert of this CL (patchset #5 id:160001) has been created in
> https://codereview.chromium.org/2810853002/ by
https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chengx@chromium.org.
> 
> The reason for reverting is: Both the new/old Canary versions are running
> RunUpdateOnFileThread callback function significantly slower suddenly. While
> this means this CL is irrelevant, I think it still should be reverted to see
> what happens. This allows further investigation. .

This is expected per TaskPriority::BACKGROUND. You can use
TaskPriority::USER_VISIBLE if you need those to complete in a user visible
delay. It all depends on whether you want to add contention on the system for
this or are happy with a delay on the icon update when the system is under
contention (IMO the latter may be preferable -- what is "much" slower and at
which percentiles?).

[chengx, 2017-04-10 19:37:32]

On 2017/04/10 18:07:51, gab wrote:
> On 2017/04/10 17:53:05, chengx wrote:
> > A revert of this CL (patchset #5 id:160001) has been created in
> > https://codereview.chromium.org/2810853002/ by
> https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chengx@chromium.org.
> > 
> > The reason for reverting is: Both the new/old Canary versions are running
> > RunUpdateOnFileThread callback function significantly slower suddenly. While
> > this means this CL is irrelevant, I think it still should be reverted to see
> > what happens. This allows further investigation. .
> 
> This is expected per TaskPriority::BACKGROUND. You can use
> TaskPriority::USER_VISIBLE if you need those to complete in a user visible
> delay. It all depends on whether you want to add contention on the system for
> this or are happy with a delay on the icon update when the system is under
> contention (IMO the latter may be preferable -- what is "much" slower and at
> which percentiles?).

Thanks! Unfortunately, the confusion is beyond this. Please see the design doc,
page 11, the "Results [I am very confused]" for the whole story.