[MD Settings] Remove jank for webui.

content/browser/webui/url_data_manager_backend.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/webui/url_data_manager_backend.h"
 
 #include <set>
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/debug/alias.h"
 #include "base/lazy_instance.h"
 #include "base/location.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/ref_counted_memory.h"
 #include "base/memory/weak_ptr.h"
 #include "base/profiler/scoped_tracker.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "base/threading/worker_pool.h"
 #include "base/trace_event/trace_event.h"
 #include "content/browser/blob_storage/chrome_blob_storage_context.h"
 #include "content/browser/histogram_internals_request_job.h"
 #include "content/browser/net/view_blob_internals_job_factory.h"
 #include "content/browser/net/view_http_cache_job_factory.h"
 #include "content/browser/resource_context_impl.h"
 #include "content/browser/webui/shared_resources_data_source.h"
 #include "content/browser/webui/url_data_source_impl.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
@@ skipping 62 matching lines @@
   std::string result;
   if (request->extra_request_headers().GetHeader(
           net::HttpRequestHeaders::kOrigin, &result))
     return result;
   net::HttpRequestHeaders headers;
   if (request->GetFullRequestHeaders(&headers))
     headers.GetHeader(net::HttpRequestHeaders::kOrigin, &result);
   return result;
 }
 
+// Copy data from source buffer into IO buffer destination.
+// TODO(groby): Very similar to URLRequestSimpleJob, unify at some point.
+void CopyData(const scoped_refptr<net::IOBuffer>& buf,
+              int buf_size,
+              const scoped_refptr<base::RefCountedMemory>& data,
+              int64_t data_offset) {
+  if (buf_size > 0) {

[Dan Beam, 2016/07/19 23:27:33]

when will this ever be == 0?

[groby-ooo-7-16, 2016/07/20 01:01:40]

It used to be possible in a previous iteration of the code :) (Removing)

+    // TODO(pkasting): Remove ScopedTracker below once crbug.com/455423 is
+    // fixed.
+    tracked_objects::ScopedTracker tracking_profile(
+        FROM_HERE_WITH_EXPLICIT_FUNCTION(
+            "455423 URLRequestChromeJob::CompleteRead memcpy"));
+    memcpy(buf->data(), data->front() + data_offset, buf_size);
+  }
+}
+
 }  // namespace
 
 // URLRequestChromeJob is a net::URLRequestJob that manages running
 // chrome-internal resource requests asynchronously.
 // It hands off URL requests to ChromeURLDataManager, which asynchronously
 // calls back once the data is available.
 class URLRequestChromeJob : public net::URLRequestJob {
  public:
   // |is_incognito| set when job is generated from an incognito profile.
   URLRequestChromeJob(net::URLRequest* request,
@@ skipping 80 matching lines @@
   // (This pattern is shared by most net::URLRequestJob implementations.)
   void StartAsync();
 
   // Due to a race condition, DevTools relies on a legacy thread hop to the UI
   // thread before calling StartAsync.
   // TODO(caseq): Fix the race condition and remove this thread hop in
   // https://crbug.com/616641.
   static void DelayStartForDevTools(
       const base::WeakPtr<URLRequestChromeJob>& job);
 
-  // Do the actual copy from data_ (the data we're serving) into |buf|.
-  // Separate from ReadRawData so we can handle async I/O. Returns the number of
-  // bytes read.
-  int CompleteRead(net::IOBuffer* buf, int buf_size);
+  // Post a task to copy |data_| to |buf_| on a worker thread, to avoid browser
+  // jank. (|data_| might be mem-mapped, so a memcpy can trigger file ops).
+  int PostReadTask(net::IOBuffer* buf, int buf_size);
 
   // The actual data we're serving.  NULL until it's been fetched.
   scoped_refptr<base::RefCountedMemory> data_;
   // The current offset into the data that we're handing off to our
   // callers via the Read interfaces.
   int data_offset_;
 
   // For async reads, we keep around a pointer to the buffer that
   // we're reading into.
   scoped_refptr<net::IOBuffer> pending_buf_;
@@ skipping 22 matching lines @@
   // If not empty, "Access-Control-Allow-Origin:" is set to the value of this
   // string.
   std::string access_control_allow_origin_;
 
   // True when job is generated from an incognito profile.
   const bool is_incognito_;
 
   // The backend is owned by net::URLRequestContext and always outlives us.
   URLDataManagerBackend* backend_;
 
+  scoped_refptr<base::TaskRunner> task_runner_;
   base::WeakPtrFactory<URLRequestChromeJob> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(URLRequestChromeJob);
 };
 
 URLRequestChromeJob::URLRequestChromeJob(net::URLRequest* request,
                                          net::NetworkDelegate* network_delegate,
                                          URLDataManagerBackend* backend,
                                          bool is_incognito)
     : net::URLRequestJob(request, network_delegate),
       data_offset_(0),
       pending_buf_size_(0),
       allow_caching_(true),
       add_content_security_policy_(true),
       deny_xframe_options_(true),
       send_content_type_header_(false),
       is_incognito_(is_incognito),
       backend_(backend),
+      task_runner_(base::WorkerPool::GetTaskRunner(false)),

[Dan Beam, 2016/07/19 23:27:33]

do we want to ref the workerpool the whole time this class exists?

[groby-ooo-7-16, 2016/07/20 01:01:40]

We'll need it at the very least until all read requests are satisfied. That's
pretty much the entire duration of the job's life.

Either way, it's not really a difference because the task runners are a global -
it doesn't matter how long we hold them : ) (And this way, this is closer to
URLRequestSimpleJob, which I'd _like_ to combine with this at some point)

[mmenke, 2016/07/20 16:47:58]

You don't need to hold onto it - it's a leaked global.  You can just inline
base::WorkerPool::GetTaskRunner wherever you need to use it.

[groby-ooo-7-16, 2016/07/20 18:10:11]

Sounds good. 

       weak_factory_(this) {
   DCHECK(backend);
 }
 
 URLRequestChromeJob::~URLRequestChromeJob() {
   CHECK(!backend_->HasPendingJob(this));
 }
 
 void URLRequestChromeJob::Start() {
   const GURL url = request_->url();
@@ skipping 76 matching lines @@
   }
 }
 
 void URLRequestChromeJob::MimeTypeAvailable(const std::string& mime_type) {
   set_mime_type(mime_type);
   NotifyHeadersComplete();
 }
 
 void URLRequestChromeJob::DataAvailable(base::RefCountedMemory* bytes) {
   TRACE_EVENT_ASYNC_END0("browser", "DataManager:Request", this);
-  if (bytes) {
-    data_ = bytes;
-    if (pending_buf_.get()) {
-      CHECK(pending_buf_->data());
-      int result = CompleteRead(pending_buf_.get(), pending_buf_size_);
-      pending_buf_ = NULL;
+  DCHECK(!data_);
+
+  // If we don't have data, signal an error.

[mmenke, 2016/07/20 16:47:58]

Here and below, some people hold pretty strongly to avoiding "we" in comments,
since it's ambiguous ("we" the chromium authors? "we" the class? etc).  Up to
you if you want to reword, just thought I'd mention it.

[groby-ooo-7-16, 2016/07/20 18:10:12]

Clearly, it's the royal "we" ;) Fixed

+  if (!bytes) {
+    ReadRawDataComplete(net::ERR_FAILED);
+    return;
+  }
+
+  // We have data, let's try to satisfy the request.
+  data_ = bytes;
+
+  if (pending_buf_.get()) {

[mmenke, 2016/07/20 16:47:58]

optioanl nit:  .get() not needed.  (I feel if it's worth the effort of making
scoped_refptrs work in conditionals without the .get, then it should be the
preferred pattern)

[groby-ooo-7-16, 2016/07/20 18:10:12]

Thanks for flagging, fixed.

+    int result = PostReadTask(pending_buf_.get(), pending_buf_size_);
+    if (result != net::ERR_IO_PENDING)
       ReadRawDataComplete(result);
-    }
-  } else {
-    // The request failed.
-    ReadRawDataComplete(net::ERR_FAILED);
+    pending_buf_ = nullptr;
   }
 }
 
 base::WeakPtr<URLRequestChromeJob> URLRequestChromeJob::AsWeakPtr() {
   return weak_factory_.GetWeakPtr();
 }
 
 int URLRequestChromeJob::ReadRawData(net::IOBuffer* buf, int buf_size) {
+  DCHECK(!pending_buf_.get());
+
+  // If data isn't available yet, mark this as asynchronous.
   if (!data_.get()) {
-    DCHECK(!pending_buf_.get());
-    CHECK(buf->data());
     pending_buf_ = buf;
     pending_buf_size_ = buf_size;
     return net::ERR_IO_PENDING;
   }
 
-  // Otherwise, the data is available.
-  return CompleteRead(buf, buf_size);
+  return PostReadTask(buf, buf_size);
 }
 
-int URLRequestChromeJob::CompleteRead(net::IOBuffer* buf, int buf_size) {
+int URLRequestChromeJob::PostReadTask(net::IOBuffer* buf, int buf_size) {

[mmenke, 2016/07/20 16:47:58]

Seems a little weird to take bug and buf_size as arguments, since they're class
variables.  I'd suggest just unconditionally setting them in ReadRawData, and
then making this take no arguments.  Makes both paths more uniform.

Would also need to clear pending_buf_ in this method, instead of in
DataAvailable.

[groby-ooo-7-16, 2016/07/20 18:10:11]

That was my original solution - I switched to this for three reasons:

1) The current setup makes it slightly clearer (to me?) that pending_buf_ really
is only satisfying the first request - after that, all requests are satisfied
immediately.
2) It mirrors the ReadRawData API, which is fitting, since ReadRawData just
posts its params to a task.
3) I have a dislike for functions that use object state to pass parameters. Yes,
I'm still using |data_|, but that is state that is actually modified. The buffer
is only state as long as it's a pending buffer due to data not being available. 

If you'd like me to switch over anyways, please let me know. 

[mmenke, 2016/07/20 18:19:43]

I don't have a particularly strong opinion here, just feel that making both
paths as similar as possible is the least likely to lead to problems down the
line (And you're using the class variables to pass parameters to DataAvailable
from here, already).  If you think this is clearer, happy to defer to you.

[groby-ooo-7-16, 2016/07/26 02:37:45]

Acknowledged.

+  DCHECK(buf && data_);

[mmenke, 2016/07/20 16:47:58]

Should split this into two DCHECKs, so if one fails, it's clearer which.

[groby-ooo-7-16, 2016/07/20 18:10:12]

Done.

+  CHECK(buf->data());
+
   int remaining = data_->size() - data_offset_;
   if (buf_size > remaining)
     buf_size = remaining;
-  if (buf_size > 0) {
-    // TODO(pkasting): Remove ScopedTracker below once crbug.com/455423 is
-    // fixed.
-    tracked_objects::ScopedTracker tracking_profile(
-        FROM_HERE_WITH_EXPLICIT_FUNCTION(
-            "455423 URLRequestChromeJob::CompleteRead memcpy"));
-    memcpy(buf->data(), data_->front() + data_offset_, buf_size);
-    data_offset_ += buf_size;
-  }
-  return buf_size;
+
+  if (buf_size == 0)
+    return 0;
+

[Dan Beam, 2016/07/19 23:27:33]

can we just do

if (buf_size == 0) {
  task_runner_ = nullptr;
  return 0;
}

// maybe if (!task_runner_)
task_runner_ = base::WorkerPool::GetTaskRunner(false);
task_runner_->PostTaskAndReply(...)

[groby-ooo-7-16, 2016/07/20 01:01:40]

We could - but there's no benefit to it. See above on lifetime. (Plus, as above,
using the SimpleJob looms. We're almost there, but not quite. See bug for
discussion why I can't use it yet)

+  task_runner_->PostTaskAndReply(

[mmenke, 2016/07/20 16:47:58]

Could just use WorkerPool::PostTaskAndReply, and get rid of the TaskRunner.

[groby-ooo-7-16, 2016/07/20 18:10:11]

I looked at it, but it creates two additional objects
(PostTaskAndReplyWorkerPool on the stack, PostTaskAndReplyRelay on the heap),
and it involves posting an additional task (PostTaskAndReplyRelay::Run, which
will then do the actual PostTaskAndReply), presumably increasing latency. 

And I _really_ would like to keep this as low latency as possible. (You'd think
it doesn't matter, but md-settings loads a _lot_ of resources, in a chained way.
It adds up)

Again, if I'm missing something or am overly cautious, please LMK and I'll fix.

[mmenke, 2016/07/20 18:19:43]

Code size generally matters more, and the single call seems better, from that
standpoint.

If WorkerPool::PostTaskAndReply is really slower, wouldn't it make more sense to
modify WorkerPool::PostTaskAndReply itself to just be:

GetTaskRunner(task_is_slow)->PostTaskAndReply(from_here, task, reply);?

I have no idea why it currently doesn't just do this.

[groby-ooo-7-16, 2016/07/26 02:37:45]

Digging deeper, the relay is necessary anyways, and so is the intermediate
workerpool. The task_runner_ call does the same thing, just in different places.
Oh, well, at least I learned a bit more about our infrastructure :)

Anyways, changed and Done.

+      FROM_HERE, base::Bind(&CopyData, base::RetainedRef(buf), buf_size, data_,
+                            data_offset_),
+      base::Bind(&URLRequestChromeJob::ReadRawDataComplete, AsWeakPtr(),
+                 buf_size));
+  data_offset_ += buf_size;
+
+  return net::ERR_IO_PENDING;
 }
 
 void URLRequestChromeJob::DelayStartForDevTools(
     const base::WeakPtr<URLRequestChromeJob>& job) {
   BrowserThread::PostTask(
       BrowserThread::IO,
       FROM_HERE,
       base::Bind(&URLRequestChromeJob::StartAsync, job));
 }
 
@@ skipping 372 matching lines @@
 
 }  // namespace
 
 net::URLRequestJobFactory::ProtocolHandler*
 CreateDevToolsProtocolHandler(content::ResourceContext* resource_context,
                               bool is_incognito) {
   return new DevToolsJobFactory(resource_context, is_incognito);
 }
 
 }  // namespace content