Fix login visual hitch on chromebook

chrome/browser/chromeos/login/user_image_loader.cc

Index: chrome/browser/chromeos/login/user_image_loader.cc
diff --git a/chrome/browser/chromeos/login/user_image_loader.cc b/chrome/browser/chromeos/login/user_image_loader.cc
index 89fae745c7470cdf743eb2dede89d70dd382861f..022d4b811b5da1688dae149f4d6c3f9298388442 100644
--- a/chrome/browser/chromeos/login/user_image_loader.cc
+++ b/chrome/browser/chromeos/login/user_image_loader.cc
@@ -9,6 +9,7 @@
 #include "base/file_util.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop.h"
+#include "base/threading/worker_pool.h"
 #include "chrome/browser/chromeos/login/helper.h"
 #include "chrome/browser/chromeos/login/user_image.h"
 #include "content/public/browser/browser_thread.h"
@@ -44,14 +45,22 @@ void UserImageLoader::Start(const std::string& filepath,
   target_message_loop_ = MessageLoop::current();
 
   ImageInfo image_info(size, loaded_cb);
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-      base::Bind(&UserImageLoader::LoadImage, this, filepath, image_info));
+  base::SequencedWorkerPool* pool = BrowserThread::GetBlockingPool();
+  SequenceToken sequence_token = pool->GetSequenceToken();
+  scoped_refptr<base::SequencedTaskRunner> task_runner = pool->
+      GetSequencedTaskRunnerWithShutdownBehavior(sequence_token,
+          base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN);

[Joao da Silva, 2013/01/21 08:07:39]

If this is using a sequenced task runner then it won't execute tasks in
parallel. Just post directly to the blocking pool.

[bshe, 2013/01/21 16:04:40]

pool->GetSequenceToken will return a different token each time. I thought
sequenced task with different token can execute paralle. But I maybe wrong? Are
you suggesting to use GetTaskRunnerWithShutdownBehavior here?

On 2013/01/21 08:07:39, Joao da Silva wrote:

[Joao da Silva, 2013/01/21 16:46:58]

Yes, you're right. Either call GetTaskRunnerWithShutdownBehavior(), or just use
the blocking pool directly (it's a TaskRunner too).

[bshe, 2013/01/21 22:53:07]

It looks like UtilityProcessHost::Create require a SequencedTaskRunner as
parameter. If we passing a TaskRunner to image_decoder, we will have to
dynamic_cast TaskRunner to SequencedTaskRunner, which is really bad.

I cant think of a good way to solve this. So I kept the SequencedTaskRunner in
this patch. Any suggestion is welcome.

At the same time, the performance with sequence task runner seems acceptable. I
have tested them on lumpy. And with this patch, wallpaper finished loading
around 400-500ms earlier than before.

On 2013/01/21 16:46:58, Joao da Silva wrote:

[bshe, 2013/01/22 03:48:57]

Actually, the sequence task runner is running parallel according to chrome
tracing. Since I am getting a new sequence token each time. These tasks can be
ran at the same time.
Please see the screenshot I got here:
https://code.google.com/p/chromium/issues/detail?id=168297#c17

In the picture: thread 1889 is the task to load a custom user profile image;
thread 1909 loads exactly the same image for a different user; and thread 1924
loads a wallpaper.

On 2013/01/21 22:53:07, bshe wrote:

+  task_runner->PostTask(
+      FROM_HERE,
+      base::Bind(&UserImageLoader::LoadImage, this, filepath, image_info,
+                 sequence_token));
 }
 
 void UserImageLoader::LoadImage(const std::string& filepath,
-                                const ImageInfo& image_info) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+                                const ImageInfo& image_info,
+                                const SequenceToken& sequence_token) {
+  DCHECK(BrowserThread::GetBlockingPool()->
+      IsRunningSequenceOnCurrentThread(sequence_token));
 
   std::string image_data;
   file_util::ReadFileToString(FilePath(filepath), &image_data);
@@ -59,17 +68,19 @@ void UserImageLoader::LoadImage(const std::string& filepath,
   scoped_refptr<ImageDecoder> image_decoder =
       new ImageDecoder(this, image_data, image_codec_);
   image_info_map_.insert(std::make_pair(image_decoder.get(), image_info));
-  image_decoder->Start();
+  image_decoder->Start(sequence_token);
 }
 
 void UserImageLoader::OnImageDecoded(const ImageDecoder* decoder,
                                      const SkBitmap& decoded_image) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-
-  ImageInfoMap::iterator info_it = image_info_map_.find(decoder);
-  if (info_it == image_info_map_.end()) {
-    NOTREACHED();
-    return;
+  ImageInfoMap::iterator info_it;
+  {
+    base::AutoLock lock(lock_);
+    info_it = image_info_map_.find(decoder);
+    if (info_it == image_info_map_.end()) {
+      NOTREACHED();
+      return;
+    }
   }
 
   const ImageInfo& image_info = info_it->second;

[Joao da Silva, 2013/01/21 08:07:39]

info_it may be invalid at this point, because another thread may have just
erased it or another iterator from the map.

Locking access to image_info_map_ adds complexity and is very easy to break in
the future. I'd suggest the following instead:

- Start() posts to the blocking pool directly, to have parallel tasks;
- OnImageDecoded() can then enter from several threads; just make it post again
to UI:

void UserImageLoader::OnImageDecoded(decoder, decoded_image) {
  BrowserThread::PostTask(
    BrowserThread::UI, FROM_HERE,
    base::Bind(&UserImageLoader::ProcessDecodedImage, this, decoder,
decoded_image));
}

And add a new ProcessDecodedImage() method that always runs on UI and does what
OnImageDecoded() is doing now.

Same thing for OnDecodeImageFailed.

[bshe, 2013/01/21 16:04:40]

If post back to UI, I worry this may be blocked on some tasks running on UI at
start up. And the whole point of this CL is to unblock the operation as much as
possible especially at start up.

I remember I tried to execute the image_decoder->Start from UI thread and
measure the time spend on each phrase. The time it takes from decoding finished
on uitility process to start execute this function is around 100ms(cant remember
the exact number but it is a large number). So we must wait some tasks to finish
on UI thread before execute this. I will double check with your suggested
approach and get some concrete measurement on real device.

On 2013/01/21 08:07:39, Joao da Silva wrote:

[Joao da Silva, 2013/01/21 16:46:58]

I'd be very surprised if UI is stuck at any moment, but you may be right. I
won't stop you if you prefer the locks :-) we just try to avoid them as much as
possible in chrome.

[bshe, 2013/01/21 22:53:07]

Thanks. I will try to use lock to unblock the task as much as possible.

I have modified the lock block. It should be safe now.

On 2013/01/21 16:46:58, Joao da Silva wrote:

@@ -102,23 +113,32 @@ void UserImageLoader::OnImageDecoded(const ImageDecoder* decoder,
       FROM_HERE,
       base::Bind(image_info.loaded_cb, user_image));
 
-  image_info_map_.erase(info_it);
+  {
+    base::AutoLock lock(lock_);
+    image_info_map_.erase(info_it);
+  }
 }
 
 void UserImageLoader::OnDecodeImageFailed(const ImageDecoder* decoder) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-
-  ImageInfoMap::iterator info_it = image_info_map_.find(decoder);
-  if (info_it == image_info_map_.end()) {
-    NOTREACHED();
-    return;
+  ImageInfoMap::iterator info_it;
+  {
+    base::AutoLock lock(lock_);
+    info_it = image_info_map_.find(decoder);
+    if (info_it == image_info_map_.end()) {
+      NOTREACHED();
+      return;
+    }
   }
 
   const ImageInfo& image_info = info_it->second;
   target_message_loop_->PostTask(
       FROM_HERE,
       base::Bind(image_info.loaded_cb, UserImage()));
-  image_info_map_.erase(decoder);
+
+  {
+    base::AutoLock lock(lock_);
+    image_info_map_.erase(decoder);
+  }
 }
 
 }  // namespace chromeos