Log states of JobController in Net MemoryDumpProvider

net/http/http_stream_factory_impl.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 "net/http/http_stream_factory_impl.h"
 
 #include <string>
 
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
+#include "base/strings/stringprintf.h"
+#include "base/trace_event/memory_allocator_dump.h"
+#include "base/trace_event/process_memory_dump.h"
 #include "net/http/http_network_session.h"
 #include "net/http/http_server_properties.h"
 #include "net/http/http_stream_factory_impl_job.h"
 #include "net/http/http_stream_factory_impl_job_controller.h"
 #include "net/http/http_stream_factory_impl_request.h"
 #include "net/http/transport_security_state.h"
 #include "net/proxy/proxy_info.h"
 #include "net/quic/core/quic_server_id.h"
 #include "net/spdy/bidirectional_stream_spdy_impl.h"
 #include "net/spdy/spdy_http_stream.h"
@@ skipping 271 matching lines @@
   HostPortPair host_port_pair = proxy_info.proxy_server().host_port_pair();
   DCHECK(!host_port_pair.IsEmpty());
 
   url::SchemeHostPort scheme_host_port("https", host_port_pair.host(),
                                        host_port_pair.port());
 
   return session_->http_server_properties()->SupportsRequestPriority(
       scheme_host_port);
 }
 
+void HttpStreamFactoryImpl::DumpMemoryStats(
+    base::trace_event::ProcessMemoryDump* pmd,
+    const std::string& parent_absolute_name) const {
+  std::string name =
+      base::StringPrintf("%s/stream_factory", parent_absolute_name.c_str());
+  base::trace_event::MemoryAllocatorDump* factory =

[ssid, 2017/01/13 22:23:59]

factory_dump? Not to confuse with the Factory object.

[xunjieli, 2017/01/17 17:51:24]

Done.

+      pmd->CreateAllocatorDump(name);
+  factory->AddScalar(base::trace_event::MemoryAllocatorDump::kNameObjectCount,
+                     base::trace_event::MemoryAllocatorDump::kUnitsObjects,
+                     job_controller_set_.size());
+  size_t alt_job_count = 0;
+  size_t preconnect_controller_count = 0;
+  for (const auto& it : job_controller_set_) {
+    if (it->HasPendingAltJob())
+      alt_job_count++;

[Zhongyi Shi, 2017/01/13 21:57:50]

I thought we are trying to logging how many JobControllers are there and why
they are live. Not sure why we only care the job controller having the pending
alt job?

I would prefer to breakdown into following category:

1. JobController with preconnect
2. JobController with two jobs running
3. JobController with only one job running: 
   you might also want to break this down to which job is running
 3.1. JobController with only one job running, job is orphaned.
 3.2. JobController with only one job running, job is bind to request

How does that sound?

[xunjieli, 2017/01/17 17:51:24]

Great question. I think this was the goal while investigating the "leak." In the
longer term, we don't have the capacity to do things in that detail. Here we can
only try log the aggregated data across all controllers. If we want
per-controller data, we need a MemoryAllocatorDump per controller, which is
expensive and will prohibit the net/ dump provider from being turned on in
SlowReports.

I think here the main goal is to make sure that there aren't too many
controllers and the total number will *decrease* over time.
If somehow, the traces we obtained from the wild (through SlowReports) show us
that the number of controllers only increase and not decrease, we can dive in
and find out more.

We can try logging the extra information by creating one dump per controller in
heavy mode. But I am not sure if we gain much by doing that. The dump provider
serves to tell us if a subcomponent is misbehaving in terms of memory
consumption. We can get a rough idea from the aggregate data.
 

[Zhongyi Shi, 2017/01/18 04:56:21]

The rationale makes sense to me, thanks for explaining. The preconnect case
looks good, but why we only count for those job controllers which has pending
ALT job? I thought what you're saying is jobcontroller that has pending jobs. I
don't understand why we cares about the specific case that we have pending ALT
job.

[xunjieli, 2017/01/18 19:49:31]

I didn't do a "pending main job count" is because we have (a) the total # of
controllers, and (b) total # of controllers with pending ALT job. The # of
controllers with pending Main job should be slightly larger than a - b. This
assumes that the number of controllers with both a main job and an alt job is
small.

Do you think we should do a pending main job count as well?

[Zhongyi Shi, 2017/01/18 20:17:45]

It will be a good idea if you also do a pending main job so that we could
calculate the number of job controllers in each type. Does that sound plausible
or do we have the capacity?

[xunjieli, 2017/01/18 20:25:20]

Done. SGTM. PTAL?

+    if (it->is_preconnect())
+      preconnect_controller_count++;
+  }
+  factory->AddScalar("alt_job_count",
+                     base::trace_event::MemoryAllocatorDump::kUnitsObjects,
+                     alt_job_count);
+  factory->AddScalar("preconnect_count",
+                     base::trace_event::MemoryAllocatorDump::kUnitsObjects,
+                     preconnect_controller_count);
+}
+
 }  // namespace net