Skip to content

Commit

Permalink
Improve performance of proxy resolver by tracing DNS dependencies.
Browse files Browse the repository at this point in the history
This replaces the multi-threaded V8 proxy resolver implementation, with a faster single-threaded one. The single-threaded version uses some magic to avoid blocking on DNS dependencies, so it is able to handle more parallel requests than the multi-threaded one.

Design document: https://docs.google.com/a/chromium.org/document/d/16Ij5OcVnR3s0MH4Z5XkhI9VTPoMJdaBn9rKreAmGOdE/edit

This has the benefit of reducing the number of threads that Chrome uses for PAC evaluation, while at the same time speeding up proxy resolving for PAC scripts that do DNS resolving (due to better parallelism).

I ran a benchmark to evaluate the effectiveness of this new approach. The benchmark simulates loading the http://www.newyorktimes.com webpage with slow DNS (where each DNS resolve takes 2 seconds), and a maximum DNS resolver parallelism of 10 requests. This webpage resolves the proxy for 221 URLs, across 40 unique hostnames.

  - Proxy resolving using the old multithreaded code took 24.076 seconds [*]
  - Proxy resolving using the new code took 8.011 seconds [*]
  - Without a PAC script, resolving the DNS took 8.003 seconds

The new proxy resolving times (8.011s) are much closer to the theoretical best (8.003s)!

[*] The PAC script I used for the test was a fairly complex script 20kb (a version of google's corp PAC script modified to always call dnsResolve(host)).

I will be adding histograms in a follow-up CL, to measure how often requests need to be restarted, or fall-back to synchronous mode.

BUG=119151

Review URL: https://codereview.chromium.org/11885009

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@179714 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
eroman@chromium.org committed Jan 30, 2013
1 parent 616b701 commit 501e2d4
Show file tree
Hide file tree
Showing 51 changed files with 2,342 additions and 1,612 deletions.
1 change: 1 addition & 0 deletions build/android/pylib/gtest/test_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ def GetDataFilesForTestSuite(self):
'net/data/cache_tests',
'net/data/filter_unittests',
'net/data/ftp',
'net/data/proxy_resolver_v8_tracing_unittest',
'net/data/proxy_resolver_v8_unittest',
'net/data/ssl/certificates',
'net/data/url_request_unittest/',
Expand Down
1 change: 0 additions & 1 deletion chrome/browser/net/connection_tester.cc
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,6 @@ class ExperimentURLRequestContext : public net::URLRequestContext {
experiment_proxy_service->reset(
net::CreateProxyServiceUsingV8ProxyResolver(
proxy_config_service->release(),
0u,
new net::ProxyScriptFetcherImpl(proxy_request_context_),
dhcp_factory.Create(proxy_request_context_),
host_resolver(),
Expand Down
1 change: 0 additions & 1 deletion chrome/browser/net/proxy_service_factory.cc
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,6 @@ net::ProxyService* ProxyServiceFactory::CreateProxyService(

proxy_service = net::CreateProxyServiceUsingV8ProxyResolver(
proxy_config_service,
num_pac_threads,
new net::ProxyScriptFetcherImpl(context),
dhcp_factory.Create(context),
context->host_resolver(),
Expand Down
15 changes: 12 additions & 3 deletions net/base/capturing_net_log.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "net/base/capturing_net_log.h"

#include "base/json/json_writer.h"
#include "base/logging.h"
#include "base/values.h"

Expand Down Expand Up @@ -34,22 +35,22 @@ CapturingNetLog::CapturedEntry::operator=(const CapturedEntry& entry) {
time = entry.time;
source = entry.source;
phase = entry.phase;
params.reset(entry.params.get() ? entry.params->DeepCopy() : NULL);
params.reset(entry.params ? entry.params->DeepCopy() : NULL);
return *this;
}

bool CapturingNetLog::CapturedEntry::GetStringValue(
const std::string& name,
std::string* value) const {
if (!params.get())
if (!params)
return false;
return params->GetString(name, value);
}

bool CapturingNetLog::CapturedEntry::GetIntegerValue(
const std::string& name,
int* value) const {
if (!params.get())
if (!params)
return false;
return params->GetInteger(name, value);
}
Expand All @@ -58,6 +59,14 @@ bool CapturingNetLog::CapturedEntry::GetNetErrorCode(int* value) const {
return GetIntegerValue("net_error", value);
}

std::string CapturingNetLog::CapturedEntry::GetParamsJson() const {
if (!params)
return std::string();
std::string json;
base::JSONWriter::Write(params.get(), &json);
return json;
}

CapturingNetLog::CapturingNetLog()
: last_id_(0),
log_level_(LOG_ALL_BUT_BYTES) {
Expand Down
4 changes: 4 additions & 0 deletions net/base/capturing_net_log.h
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@ class CapturingNetLog : public NetLog {
// log entry.
bool GetNetErrorCode(int* value) const;

// Returns the parameters as a JSON string, or empty string if there are no
// parameters.
std::string GetParamsJson() const;

EventType type;
base::TimeTicks time;
Source source;
Expand Down
10 changes: 9 additions & 1 deletion net/base/mock_host_resolver.cc
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ int MockHostResolverBase::Resolve(const RequestInfo& info,
RequestHandle* handle,
const BoundNetLog& net_log) {
DCHECK(CalledOnValidThread());
num_resolve_++;
size_t id = next_request_id_++;
int rv = ResolveFromIPLiteralOrCache(info, addresses);
if (rv != ERR_DNS_CACHE_MISS) {
Expand All @@ -97,6 +98,7 @@ int MockHostResolverBase::Resolve(const RequestInfo& info,
int MockHostResolverBase::ResolveFromCache(const RequestInfo& info,
AddressList* addresses,
const BoundNetLog& net_log) {
num_resolve_from_cache_++;
DCHECK(CalledOnValidThread());
next_request_id_++;
int rv = ResolveFromIPLiteralOrCache(info, addresses);
Expand Down Expand Up @@ -134,7 +136,9 @@ void MockHostResolverBase::ResolveAllPending() {
MockHostResolverBase::MockHostResolverBase(bool use_caching)
: synchronous_mode_(false),
ondemand_mode_(false),
next_request_id_(1) {
next_request_id_(1),
num_resolve_(0),
num_resolve_from_cache_(0) {
rules_ = CreateCatchAllHostResolverProc();

if (use_caching) {
Expand Down Expand Up @@ -305,6 +309,10 @@ void RuleBasedHostResolverProc::AddSimulatedFailure(
rules_.push_back(rule);
}

void RuleBasedHostResolverProc::ClearRules() {
rules_.clear();
}

int RuleBasedHostResolverProc::Resolve(const std::string& host,
AddressFamily address_family,
HostResolverFlags host_resolver_flags,
Expand Down
20 changes: 19 additions & 1 deletion net/base/mock_host_resolver.h
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,16 @@ class MockHostResolverBase : public HostResolver,
// ResolveAllPending().
bool has_pending_requests() const { return !requests_.empty(); }

// The number of times that Resolve() has been called.
size_t num_resolve() const {
return num_resolve_;
}

// The number of times that ResolveFromCache() has been called.
size_t num_resolve_from_cache() const {
return num_resolve_from_cache_;
}

protected:
explicit MockHostResolverBase(bool use_caching);

Expand All @@ -117,6 +127,9 @@ class MockHostResolverBase : public HostResolver,
RequestMap requests_;
size_t next_request_id_;

size_t num_resolve_;
size_t num_resolve_from_cache_;

DISALLOW_COPY_AND_ASSIGN(MockHostResolverBase);
};

Expand Down Expand Up @@ -158,8 +171,10 @@ class RuleBasedHostResolverProc : public HostResolverProc {
// Same as AddRule(), but the replacement is expected to be an IPv4 or IPv6
// literal. This can be used in place of AddRule() to bypass the system's
// host resolver (the address list will be constructed manually).
// If |canonical-name| is non-empty, it is copied to the resulting AddressList
// If |canonical_name| is non-empty, it is copied to the resulting AddressList
// but does not impact DNS resolution.
// |ip_literal| can be a single IP address like "192.168.1.1" or a comma
// separated list of IP addresses, like "::1,192:168.1.2".
void AddIPLiteralRule(const std::string& host_pattern,
const std::string& ip_literal,
const std::string& canonical_name);
Expand All @@ -175,6 +190,9 @@ class RuleBasedHostResolverProc : public HostResolverProc {
// Simulate a lookup failure for |host| (it also can be a pattern).
void AddSimulatedFailure(const std::string& host);

// Deletes all the rules that have been added.
void ClearRules();

// HostResolverProc methods:
virtual int Resolve(const std::string& host,
AddressFamily address_family,
Expand Down
31 changes: 31 additions & 0 deletions net/data/proxy_resolver_v8_tracing_unittest/dns.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
var g_iteration = 0;

function FindProxyForURL(url, host) {
alert('iteration: ' + g_iteration++);

var ips = [
myIpAddress(),
dnsResolve(''),
dnsResolveEx('host1'),
dnsResolve('host2'),
dnsResolve('host3'),
myIpAddress(),
dnsResolve('host3'),
dnsResolveEx('host1'),
myIpAddress(),
dnsResolve('host2'),
dnsResolveEx('host6'),
myIpAddressEx(),
dnsResolve('host1'),
];

for (var i = 0; i < ips.length; ++i) {
// Stringize everything.
ips[i] = '' + ips[i];
}

var proxyHost = ips.join('-');
proxyHost = proxyHost.replace(/[^0-9a-zA-Z.-]/g, '_');

return "PROXY " + proxyHost + ":99";
}
14 changes: 14 additions & 0 deletions net/data/proxy_resolver_v8_tracing_unittest/dns_during_init.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
var g_ips = [
dnsResolve('host1'),
dnsResolve('host2')
];

alert('Watsup');
alert('Watsup2');

function FindProxyForURL(url, host) {
// Note that host1 and host2 should not resolve using the same cache as was
// used for g_ips!
var ips = g_ips.concat([dnsResolve('host1'), dnsResolve('host2')]);
return 'PROXY ' + ips.join('-') + ':99';
}
8 changes: 8 additions & 0 deletions net/data/proxy_resolver_v8_tracing_unittest/error.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
function FindProxyForURL(url, host) {
if (host == 'throw-an-error') {
alert('Prepare to DIE!');
var x = null;
return x.split('-'); // Throws exception.
}
return "PROXY i-approve-this-message:42";
}
14 changes: 14 additions & 0 deletions net/data/proxy_resolver_v8_tracing_unittest/global_sideffects1.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
var g_iteration = 0;

function FindProxyForURL(url, host) {
g_iteration++;

var ips = [
dnsResolve('host1'),
dnsResolve('crazy' + g_iteration)
];

alert('iteration: ' + g_iteration);

return 'PROXY ' + ips.join('-') + ':100';
}
18 changes: 18 additions & 0 deletions net/data/proxy_resolver_v8_tracing_unittest/global_sideffects2.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
var g_iteration = 0;

function FindProxyForURL(url, host) {
g_iteration++;

var ips;

if (g_iteration < 3) {
ips = [
dnsResolve('host1'),
dnsResolve('host2')
];
} else {
ips = [ dnsResolve('host' + g_iteration) ];
}

return 'PROXY ' + ips.join('-') + ':100';
}
13 changes: 13 additions & 0 deletions net/data/proxy_resolver_v8_tracing_unittest/global_sideffects3.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
var g_iteration = 0;

function FindProxyForURL(url, host) {
g_iteration++;

var results = [];
for (var i = 1; i <= g_iteration; ++i) {
results.push('' + dnsResolve('host' + i));
}

alert('iteration: ' + g_iteration);
return 'PROXY ' + results.join('-') + ':' + g_iteration;
}
15 changes: 15 additions & 0 deletions net/data/proxy_resolver_v8_tracing_unittest/global_sideffects4.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
var g_iteration = 0;

function FindProxyForURL(url, host) {
g_iteration++;

for (var i = 0; i < g_iteration; ++i) {
myIpAddress();
}

var result = '' + dnsResolve('host' + g_iteration);
result += g_iteration;

alert('iteration: ' + g_iteration);
return 'PROXY ' + result + ':34';
}
3 changes: 3 additions & 0 deletions net/data/proxy_resolver_v8_tracing_unittest/simple.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
function FindProxyForURL(url, host) {
return "PROXY " + host + ":99";
}
8 changes: 8 additions & 0 deletions net/data/proxy_resolver_v8_tracing_unittest/simple_dns.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
var g_iteration = 0;

function FindProxyForURL(url, host) {
g_iteration++;
myIpAddress();
var ip = dnsResolve(host);
return "PROXY " + ip + ':' + g_iteration;
}
13 changes: 13 additions & 0 deletions net/data/proxy_resolver_v8_tracing_unittest/too_many_alerts.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
var g_iteration = 0;

function FindProxyForURL(url, host) {
g_iteration++;

dnsResolve(host);

for (var i = 0; i < 50; i++) {
alert('Gee, all these alerts are silly!');
}

return "PROXY foo:" + g_iteration;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
var g_iteration = 0;

function FindProxyForURL(url, host) {
g_iteration++;

dnsResolve(host);

for (var i = 0; i < 1000; i++) {
alert('');
}

return "PROXY foo:" + g_iteration;
}
6 changes: 0 additions & 6 deletions net/http/http_network_transaction_spdy2_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8084,12 +8084,6 @@ class CapturingProxyResolver : public ProxyResolver {
return LOAD_STATE_IDLE;
}

virtual LoadState GetLoadStateThreadSafe(
RequestHandle request) const OVERRIDE {
NOTREACHED();
return LOAD_STATE_IDLE;
}

virtual void CancelSetPacScript() {
NOTREACHED();
}
Expand Down
6 changes: 0 additions & 6 deletions net/http/http_network_transaction_spdy3_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8083,12 +8083,6 @@ class CapturingProxyResolver : public ProxyResolver {
return LOAD_STATE_IDLE;
}

virtual LoadState GetLoadStateThreadSafe(
RequestHandle request) const OVERRIDE {
NOTREACHED();
return LOAD_STATE_IDLE;
}

virtual void CancelSetPacScript() {
NOTREACHED();
}
Expand Down
Loading

0 comments on commit 501e2d4

Please sign in to comment.