Make image_decoder a Leaky LazyInstance

chrome/browser/image_decoder.cc

Index: chrome/browser/image_decoder.cc
diff --git a/chrome/browser/image_decoder.cc b/chrome/browser/image_decoder.cc
index 2f9e83b7ab8953f41eb2d5a4bfc091e8f7e81ce7..11b1362c374077a93c122881b228633d3b13473f 100644
--- a/chrome/browser/image_decoder.cc
+++ b/chrome/browser/image_decoder.cc
@@ -13,38 +13,142 @@
 using content::BrowserThread;
 using content::UtilityProcessHost;
 
-ImageDecoder::ImageDecoder(Delegate* delegate,
-                           const std::string& image_data,
-                           ImageCodec image_codec)
-    : delegate_(delegate),
-      image_data_(image_data.begin(), image_data.end()),
-      image_codec_(image_codec),
-      task_runner_(NULL),
-      shrink_to_fit_(false) {
-}
-
-ImageDecoder::ImageDecoder(Delegate* delegate,
-                           const std::vector<char>& image_data,
-                           ImageCodec image_codec)
-    : delegate_(delegate),
-      image_data_(image_data.begin(), image_data.end()),
-      image_codec_(image_codec),
-      task_runner_(NULL),
-      shrink_to_fit_(false) {
-}
-
-ImageDecoder::~ImageDecoder() {}
-
-void ImageDecoder::Start(scoped_refptr<base::SequencedTaskRunner> task_runner) {
-  task_runner_ = task_runner;
+// static, Leaky to allow access from any thread.
+base::LazyInstance<ImageDecoder>::Leaky image_decoder_instance_ =
+    LAZY_INSTANCE_INITIALIZER;
+
+// static
+ImageDecoder* ImageDecoder::GetInstance() {
+  return image_decoder_instance_.Pointer();
+}
+
+ImageDecoder::ImageDecoder() : image_decoder_impl_(new ImageDecoderImpl()) {
+}
+
+ImageDecoder::~ImageDecoder() {
+}
+
+void ImageDecoder::Start(Delegate* delegate,
+                         const std::string& image_data,
+                         ImageCodec image_codec,
+                         scoped_refptr<base::SequencedTaskRunner> task_runner,
+                         bool shrink_to_fit) {
+  DCHECK(task_runner);
+  DCHECK(delegate);
   BrowserThread::PostTask(
-     BrowserThread::IO, FROM_HERE,
-     base::Bind(&ImageDecoder::DecodeImageInSandbox, this, image_data_));
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(
+          &ImageDecoder::ImageDecoderImpl::DecodeImageInSandbox,
+          image_decoder_impl_, delegate,
+          std::vector<unsigned char>(image_data.begin(), image_data.end()),
+          image_codec, task_runner, shrink_to_fit));
+}
+
+void ImageDecoder::RemoveDelegate(Delegate* delegate) {
+  BrowserThread::PostTask(

[dcheng, 2015/03/19 14:25:46]

So this is tricky.

Due to IDMap's non-thread safe nature, we have to do this on the IO thread.

But the object being destroyed is probably not on the IO thread. Another task
might already be on the IO thread, ready to send a callback to the object we're
removing.

I guess we're better off rolling our own solution here =/

[Theresa, 2015/03/19 21:54:56]

I could go back to using a std::map and use base::AutoLock
lock(request_info_id_map_) in the methods that manipulate the map. Rather than
having this method post a task to the IO thread, it could acquire the lock and
remove the appropriate entry(ies) from the map. This would at least avoid the
issue of the RemoveDelegate request getting tacked to the back of the IO
thread's queue of tasks behind an OnImageDecoded callback to the object we're
removing.

[dcheng, 2015/03/20 00:14:45]

Yeah, I think that's the best idea.
It might also be nice to have an O(1) cancel, depending on how deeply queued
this can get.

[Theresa, 2015/03/20 19:09:01]

Changed to a lock protected std::map.

atm, I think how deeply queued this gets depends on what the user is doing. For
most uses of ImageDecoder, we're really only decoding 1 or 2 images. If the user
is in the enhanced bookmark manager scrolling really quickly past hundreds of
bookmarks, this map could have a couple dozen items in it. When I was doing some
performance profiling, the image decode requests for bookmarks only took 2-3
milliseconds. While the user is scrolling the requests queue up, some get
processed and taken out of the map, more requests queue up, etc. (so even if
you're scrolling past hundreds of bookmarks, hundreds of requests don't get
queued because there's this queue-process-queue-process sort of thing
happening). I can do some more tests around how big this map gets if that would
be useful.

Going forward, though, I don't think we can make strong guarantees about how big
this map gets. I could keep a second std::unordered_map of ImageRequest* -> list
of id's to make Cancel closer to O(1). Maybe the image_request_id_map_ should be
a std::unordered_map for O(1) lookups as well?

+      BrowserThread::IO, FROM_HERE,
+      base::Bind(
+          &ImageDecoder::ImageDecoderImpl::RemoveDelegate,
+          image_decoder_impl_, delegate));
+}
+
+// ImageDecoder::ImageDecoderImpl Methods
+
+ImageDecoder::ImageDecoderImpl::ImageDecoderImpl()
+    : delegate_id_counter_(0),
+      last_request_(base::TimeTicks::Now()),
+      batch_mode_started_(false) {
 }
 
-bool ImageDecoder::OnMessageReceived(const IPC::Message& message) {
+ImageDecoder::ImageDecoderImpl::~ImageDecoderImpl() {
+}
+
+ImageDecoder::ImageDecoderImpl::RequestInfo::RequestInfo(
+    Delegate* delegate,
+    scoped_refptr<base::SequencedTaskRunner> task_runner)
+    : delegate(delegate), task_runner(task_runner) {
+}
+
+ImageDecoder::ImageDecoderImpl::RequestInfo::~RequestInfo() {
+}
+
+void ImageDecoder::ImageDecoderImpl::DecodeImageInSandbox(
+    Delegate* delegate,
+    const std::vector<unsigned char>& image_data,
+    ImageCodec image_codec,
+    scoped_refptr<base::SequencedTaskRunner> task_runner,
+    bool shrink_to_fit) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  if (!batch_mode_started_) {
+    StartBatchMode();
+  }
+
+  last_request_ = base::TimeTicks::Now();
+  RequestInfo* request_info = new RequestInfo(delegate, task_runner);
+  request_info_id_map_.AddWithID(request_info, delegate_id_counter_);

[dcheng, 2015/03/19 14:25:46]

For future reference, I think you can just use plain old Add(), which will
automatically create an ID.

[Theresa, 2015/03/19 21:54:56]

Acknowledged. I didn't read the API closely enough to notice that plain old
Add() returns an ID that could be used instead of keeping a
delegate_id_counter_.

+
+  if (image_codec == ROBUST_JPEG_CODEC) {

[dcheng, 2015/03/19 14:25:46]

Consider writing this as a switch, so if someone ever adds new values, we don't
miss it.

[Theresa, 2015/03/19 21:54:56]

Done.

+    utility_process_host_->Send(
+        new ChromeUtilityMsg_RobustJPEGDecodeImage(image_data,
+                                                   delegate_id_counter_));
+  } else {
+    utility_process_host_->Send(new ChromeUtilityMsg_DecodeImage(
+        image_data, shrink_to_fit, delegate_id_counter_));
+  }
+
+  ++delegate_id_counter_;
+}
+
+void ImageDecoder::ImageDecoderImpl::RemoveDelegate(Delegate* delegate) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  std::vector<int> canceled_requests;
+
+  for (IDMap<RequestInfo>::iterator iter(&request_info_id_map_);
+      !iter.IsAtEnd(); iter.Advance()) {
+    if (iter.GetCurrentValue()->delegate == delegate) {
+      canceled_requests.push_back(iter.GetCurrentKey());
+    }
+  }
+
+  for (auto id : canceled_requests) {
+    RequestInfo* request_info = request_info_id_map_.Lookup(id);
+    request_info_id_map_.Remove(id);
+    delete request_info;

[dcheng, 2015/03/19 14:25:46]

For future reference, IDMap has a setting that indicates it should own the
pointer
(https://code.google.com/p/chromium/codesearch#chromium/src/base/id_map.h&l=19),
so you don't have to do manual memory management yourself.

[Theresa, 2015/03/19 21:54:56]

Acknowledged.

+    request_info = nullptr;
+  }
+}
+
+void ImageDecoder::ImageDecoderImpl::StartBatchMode() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  utility_process_host_ =
+      UtilityProcessHost::Create(this, base::MessageLoopProxy::current().get())
+          ->AsWeakPtr();
+  utility_process_host_->StartBatchMode();
+  batch_mode_started_ = true;
+  batch_mode_timer_.Start(FROM_HERE,
+                          kBatchModeTimeout,
+                          this,
+                          &ImageDecoder::ImageDecoderImpl::StopBatchMode);
+}
+
+void ImageDecoder::ImageDecoderImpl::StopBatchMode() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  if ((base::TimeTicks::Now() - last_request_) < kBatchModeTimeout) {
+    return;
+  }
+
+  if (utility_process_host_) {
+    utility_process_host_->EndBatchMode();
+    utility_process_host_.reset();
+  }
+  batch_mode_started_ = false;
+  batch_mode_timer_.Stop();
+}
+
+bool ImageDecoder::ImageDecoderImpl::OnMessageReceived(
+    const IPC::Message& message) {
   bool handled = true;
-  IPC_BEGIN_MESSAGE_MAP(ImageDecoder, message)
+  IPC_BEGIN_MESSAGE_MAP(ImageDecoder::ImageDecoderImpl, message)
     IPC_MESSAGE_HANDLER(ChromeUtilityHostMsg_DecodeImage_Succeeded,
                         OnDecodeImageSucceeded)
     IPC_MESSAGE_HANDLER(ChromeUtilityHostMsg_DecodeImage_Failed,
@@ -54,28 +158,35 @@ bool ImageDecoder::OnMessageReceived(const IPC::Message& message) {
   return handled;
 }
 
-void ImageDecoder::OnDecodeImageSucceeded(const SkBitmap& decoded_image) {
-  DCHECK(task_runner_->RunsTasksOnCurrentThread());
-  if (delegate_)
-    delegate_->OnImageDecoded(this, decoded_image);
-}
+void ImageDecoder::ImageDecoderImpl::OnDecodeImageSucceeded(
+    const SkBitmap& decoded_image,
+    int id) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  if (request_info_id_map_.Lookup(id)) {
+    RequestInfo* request_info = request_info_id_map_.Lookup(id);
+    request_info->task_runner->PostTask(
+        FROM_HERE,
+        base::Bind(&Delegate::OnImageDecoded,
+                   base::Unretained(request_info->delegate),
+                   decoded_image));
 
-void ImageDecoder::OnDecodeImageFailed() {
-  DCHECK(task_runner_->RunsTasksOnCurrentThread());
-  if (delegate_)
-    delegate_->OnDecodeImageFailed(this);
+    request_info_id_map_.Remove(id);
+    delete request_info;
+    request_info = nullptr;
+  }
 }
 
-void ImageDecoder::DecodeImageInSandbox(
-    const std::vector<unsigned char>& image_data) {
+void ImageDecoder::ImageDecoderImpl::OnDecodeImageFailed(int id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  UtilityProcessHost* utility_process_host;
-  utility_process_host = UtilityProcessHost::Create(this, task_runner_.get());
-  if (image_codec_ == ROBUST_JPEG_CODEC) {
-    utility_process_host->Send(
-        new ChromeUtilityMsg_RobustJPEGDecodeImage(image_data));
-  } else {
-    utility_process_host->Send(
-        new ChromeUtilityMsg_DecodeImage(image_data, shrink_to_fit_));
+  if (request_info_id_map_.Lookup(id)) {
+    RequestInfo* request_info = request_info_id_map_.Lookup(id);
+    request_info->task_runner->PostTask(
+        FROM_HERE,
+        base::Bind(&Delegate::OnDecodeImageFailed,
+                   base::Unretained(request_info->delegate)));
+
+    request_info_id_map_.Remove(id);
+    delete request_info;
+    request_info = nullptr;
   }
 }