Skip to content

Commit

Permalink
Misc cleanup for HostResolver::Options
Browse files Browse the repository at this point in the history 
*Rename to ManagerOptions to better clarify the purpose of the struct.
*Move GetDispatcherLimits() into host_resolver_manager.cc::anonymous to
 make Options actually PoD.
*Set default values directly on fields to simplify construction.
*Move related constants into Options.
*Rename |max_retry_attempts| -> |max_system_retry_attempts| to clarify
 that it only affects the system resolver.

Motivation is to simplify and improve things before adding to Options
to assist with removing manager configuration from ContextHostResolver.

Slightly counter-intuitively to its purpose, leaving the struct in
HostResolver instead of moving to HostResolverManager. It still gets
passed through HostResolver methods on creation of standalone resolvers
and if the struct is used in both places, HostResolver is the more
common place to put it.

Bug: 934402
Change-Id: I251b71ec889691a616b2908d2d258cd53c3f9aa9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1577852
Commit-Queue: Eric Orth <ericorth@chromium.org>
Reviewed-by: Luke Halliwell <halliwell@chromium.org>
Reviewed-by: Asanka Herath <asanka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#653803}
  • Loading branch information
Eric Orth authored and Commit Bot committed Apr 24, 2019
1 parent a33f139 commit 4d635c1
Show file tree
Hide file tree
Showing 22 changed files with 139 additions and 142 deletions.
2 changes: 1 addition & 1 deletion chromecast/browser/url_request_context_factory.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ void URLRequestContextFactory::InitializeSystemContextDependencies() {
return;

host_resolver_manager_ = std::make_unique<net::HostResolverManager>(
net::HostResolver::Options(), nullptr);
net::HostResolver::ManagerOptions(), nullptr);
cert_verifier_ =
net::CertVerifier::CreateDefault(/*cert_net_fetcher=*/nullptr);
ssl_config_service_.reset(new net::SSLConfigServiceDefaults);
Expand Down
4 changes: 2 additions & 2 deletions net/dns/context_host_resolver_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ const IPEndPoint kEndpoint(IPAddress(1, 2, 3, 4), 100);
class ContextHostResolverTest : public TestWithScopedTaskEnvironment {
protected:
void SetUp() override {
manager_ =
std::make_unique<HostResolverManager>(HostResolver::Options(), nullptr);
manager_ = std::make_unique<HostResolverManager>(
HostResolver::ManagerOptions(), nullptr);
}

void SetMockDnsRules(MockDnsClientRuleList rules) {
Expand Down
4 changes: 2 additions & 2 deletions net/dns/fuzzed_context_host_resolver.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ class FuzzedMdnsSocketFactory : public MDnsSocketFactory {
class FuzzedHostResolverManager : public HostResolverManager {
public:
// |data_provider| and |net_log| must outlive the FuzzedHostResolver.
FuzzedHostResolverManager(const Options& options,
FuzzedHostResolverManager(const HostResolver::ManagerOptions& options,
NetLog* net_log,
base::FuzzedDataProvider* data_provider)
: HostResolverManager(options, net_log),
Expand Down Expand Up @@ -345,7 +345,7 @@ class FuzzedHostResolverManager : public HostResolverManager {
} // namespace

FuzzedContextHostResolver::FuzzedContextHostResolver(
const Options& options,
const ManagerOptions& options,
NetLog* net_log,
base::FuzzedDataProvider* data_provider,
bool enable_caching)
Expand Down
2 changes: 1 addition & 1 deletion net/dns/fuzzed_context_host_resolver.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class NetLog;
// methods that make system calls are stubbed out.
class FuzzedContextHostResolver : public ContextHostResolver {
public:
FuzzedContextHostResolver(const Options& options,
FuzzedContextHostResolver(const ManagerOptions& options,
NetLog* net_log,
base::FuzzedDataProvider* data_provider,
bool enable_caching);
Expand Down
86 changes: 7 additions & 79 deletions net/dns/host_resolver.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,7 @@
#include "base/bind.h"
#include "base/logging.h"
#include "base/macros.h"
#include "base/metrics/field_trial.h"
#include "base/strings/string_number_conversions.h"
#include "base/strings/string_split.h"
#include "base/values.h"
#include "net/base/address_list.h"
#include "net/base/net_errors.h"
Expand All @@ -24,77 +22,6 @@

namespace net {

namespace {

// Maximum of 6 concurrent resolver threads (excluding retries).
// Some routers (or resolvers) appear to start to provide host-not-found if
// too many simultaneous resolutions are pending. This number needs to be
// further optimized, but 8 is what FF currently does. We found some routers
// that limit this to 6, so we're temporarily holding it at that level.
const size_t kDefaultMaxProcTasks = 6u;

} // namespace

PrioritizedDispatcher::Limits HostResolver::Options::GetDispatcherLimits()
const {
PrioritizedDispatcher::Limits limits(NUM_PRIORITIES, max_concurrent_resolves);

// If not using default, do not use the field trial.
if (limits.total_jobs != HostResolver::kDefaultParallelism)
return limits;

// Default, without trial is no reserved slots.
limits.total_jobs = kDefaultMaxProcTasks;

// Parallelism is determined by the field trial.
std::string group =
base::FieldTrialList::FindFullName("HostResolverDispatch");

if (group.empty())
return limits;

// The format of the group name is a list of non-negative integers separated
// by ':'. Each of the elements in the list corresponds to an element in
// |reserved_slots|, except the last one which is the |total_jobs|.
std::vector<base::StringPiece> group_parts = base::SplitStringPiece(
group, ":", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
if (group_parts.size() != NUM_PRIORITIES + 1) {
NOTREACHED();
return limits;
}

std::vector<size_t> parsed(group_parts.size());
size_t total_reserved_slots = 0;

for (size_t i = 0; i < group_parts.size(); ++i) {
if (!base::StringToSizeT(group_parts[i], &parsed[i])) {
NOTREACHED();
return limits;
}
}

size_t total_jobs = parsed.back();
parsed.pop_back();
for (size_t i = 0; i < parsed.size(); ++i) {
total_reserved_slots += parsed[i];
}

// There must be some unreserved slots available for the all priorities.
if (total_reserved_slots > total_jobs ||
(total_reserved_slots == total_jobs && parsed[MINIMUM_PRIORITY] == 0)) {
NOTREACHED();
return limits;
}

limits.total_jobs = total_jobs;
limits.reserved_slots = parsed;
return limits;
}

HostResolver::Options::Options()
: max_concurrent_resolves(kDefaultParallelism),
max_retry_attempts(kDefaultRetryAttempts) {}

std::unique_ptr<HostResolver> HostResolver::Factory::CreateResolver(
HostResolverManager* manager,
base::StringPiece host_mapping_rules,
Expand All @@ -105,7 +32,7 @@ std::unique_ptr<HostResolver> HostResolver::Factory::CreateResolver(

std::unique_ptr<HostResolver> HostResolver::Factory::CreateStandaloneResolver(
NetLog* net_log,
const Options& options,
const ManagerOptions& options,
base::StringPiece host_mapping_rules,
bool enable_caching) {
return HostResolver::CreateStandaloneResolver(
Expand Down Expand Up @@ -194,7 +121,7 @@ std::unique_ptr<HostResolver> HostResolver::CreateResolver(
// static
std::unique_ptr<HostResolver> HostResolver::CreateStandaloneResolver(
NetLog* net_log,
base::Optional<Options> options,
base::Optional<ManagerOptions> options,
base::StringPiece host_mapping_rules,
bool enable_caching) {
std::unique_ptr<ContextHostResolver> resolver =
Expand All @@ -211,14 +138,15 @@ std::unique_ptr<HostResolver> HostResolver::CreateStandaloneResolver(

// static
std::unique_ptr<ContextHostResolver>
HostResolver::CreateStandaloneContextResolver(NetLog* net_log,
base::Optional<Options> options,
bool enable_caching) {
HostResolver::CreateStandaloneContextResolver(
NetLog* net_log,
base::Optional<ManagerOptions> options,
bool enable_caching) {
auto cache = enable_caching ? HostCache::CreateDefaultCache() : nullptr;

return std::make_unique<ContextHostResolver>(
std::make_unique<HostResolverManager>(
std::move(options).value_or(Options()), net_log),
std::move(options).value_or(ManagerOptions()), net_log),
std::move(cache));
}

Expand Down
41 changes: 19 additions & 22 deletions net/dns/host_resolver.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
#include "net/base/address_family.h"
#include "net/base/completion_once_callback.h"
#include "net/base/host_port_pair.h"
#include "net/base/prioritized_dispatcher.h"
#include "net/base/request_priority.h"
#include "net/dns/dns_config.h"
#include "net/dns/host_cache.h"
Expand Down Expand Up @@ -112,19 +111,24 @@ class NET_EXPORT HostResolver {
virtual void ChangeRequestPriority(RequestPriority priority) {}
};

// |max_concurrent_resolves| is how many resolve requests will be allowed to
// run in parallel. Pass HostResolver::kDefaultParallelism to choose a
// default value.
// |max_retry_attempts| is the maximum number of times we will retry for host
// resolution. Pass HostResolver::kDefaultRetryAttempts to choose a default
// value.
struct NET_EXPORT Options {
Options();
// Parameter-grouping struct for additional optional parameters for creation
// of HostResolverManagers and stand-alone HostResolvers.
struct NET_EXPORT ManagerOptions {
// Set |max_concurrent_resolves| to this to select a default level
// of concurrency.
static const size_t kDefaultParallelism = 0;

PrioritizedDispatcher::Limits GetDispatcherLimits() const;
// Set |max_system_retry_attempts| to this to select a default retry value.
static const size_t kDefaultRetryAttempts = static_cast<size_t>(-1);

size_t max_concurrent_resolves;
size_t max_retry_attempts;
// How many resolve requests will be allowed to run in parallel.
// |kDefaultParallelism| for the resolver to choose a default value.
size_t max_concurrent_resolves = kDefaultParallelism;

// The maximum number of times to retry for host resolution if using the
// system resolver. No effect when the system resolver is not used.
// |kDefaultRetryAttempts| for the resolver to choose a default value.
size_t max_system_retry_attempts = kDefaultRetryAttempts;
};

// Factory class. Useful for classes that need to inject and override resolver
Expand All @@ -142,7 +146,7 @@ class NET_EXPORT HostResolver {
// See HostResolver::CreateStandaloneResolver.
virtual std::unique_ptr<HostResolver> CreateStandaloneResolver(
NetLog* net_log,
const Options& options,
const ManagerOptions& options,
base::StringPiece host_mapping_rules,
bool enable_caching);
};
Expand Down Expand Up @@ -234,13 +238,6 @@ class NET_EXPORT HostResolver {
virtual int Start(Delegate* delegate) = 0;
};

// Set Options.max_concurrent_resolves to this to select a default level
// of concurrency.
static const size_t kDefaultParallelism = 0;

// Set Options.max_retry_attempts to this to select a default retry value.
static const size_t kDefaultRetryAttempts = static_cast<size_t>(-1);

// If any completion callbacks are pending when the resolver is destroyed,
// the host resolutions are cancelled, and the completion callbacks will not
// be called.
Expand Down Expand Up @@ -330,15 +327,15 @@ class NET_EXPORT HostResolver {
// requests. See MappedHostResolver for details.
static std::unique_ptr<HostResolver> CreateStandaloneResolver(
NetLog* net_log,
base::Optional<Options> options = base::nullopt,
base::Optional<ManagerOptions> options = base::nullopt,
base::StringPiece host_mapping_rules = "",
bool enable_caching = true);
// Same, but explicitly returns the implementing ContextHostResolver. Only
// used by tests and by StaleHostResolver in Cronet. No mapping rules can be
// applied because doing so requires wrapping the ContextHostResolver.
static std::unique_ptr<ContextHostResolver> CreateStandaloneContextResolver(
NetLog* net_log,
base::Optional<Options> options = base::nullopt,
base::Optional<ManagerOptions> options = base::nullopt,
bool enable_caching = true);

// Helpers for interacting with HostCache and ProcResolver.
Expand Down
76 changes: 72 additions & 4 deletions net/dns/host_resolver_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@
#include "base/rand_util.h"
#include "base/sequence_checker.h"
#include "base/single_thread_task_runner.h"
#include "base/strings/string_piece.h"
#include "base/strings/string_split.h"
#include "base/strings/string_util.h"
#include "base/strings/utf_string_conversions.h"
#include "base/task/post_task.h"
Expand All @@ -60,6 +62,7 @@
#include "net/base/ip_address.h"
#include "net/base/ip_endpoint.h"
#include "net/base/net_errors.h"
#include "net/base/request_priority.h"
#include "net/base/trace_constants.h"
#include "net/base/url_util.h"
#include "net/dns/address_sorter.h"
Expand Down Expand Up @@ -395,6 +398,70 @@ void LogCancelRequest(const NetLogWithSource& source_net_log) {

//-----------------------------------------------------------------------------

// Maximum of 6 concurrent resolver threads (excluding retries).
// Some routers (or resolvers) appear to start to provide host-not-found if
// too many simultaneous resolutions are pending. This number needs to be
// further optimized, but 8 is what FF currently does. We found some routers
// that limit this to 6, so we're temporarily holding it at that level.
const size_t kDefaultMaxProcTasks = 6u;

PrioritizedDispatcher::Limits GetDispatcherLimits(
const HostResolver::ManagerOptions& options) {
PrioritizedDispatcher::Limits limits(NUM_PRIORITIES,
options.max_concurrent_resolves);

// If not using default, do not use the field trial.
if (limits.total_jobs != HostResolver::ManagerOptions::kDefaultParallelism)
return limits;

// Default, without trial is no reserved slots.
limits.total_jobs = kDefaultMaxProcTasks;

// Parallelism is determined by the field trial.
std::string group =
base::FieldTrialList::FindFullName("HostResolverDispatch");

if (group.empty())
return limits;

// The format of the group name is a list of non-negative integers separated
// by ':'. Each of the elements in the list corresponds to an element in
// |reserved_slots|, except the last one which is the |total_jobs|.
std::vector<base::StringPiece> group_parts = base::SplitStringPiece(
group, ":", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
if (group_parts.size() != NUM_PRIORITIES + 1) {
NOTREACHED();
return limits;
}

std::vector<size_t> parsed(group_parts.size());
size_t total_reserved_slots = 0;

for (size_t i = 0; i < group_parts.size(); ++i) {
if (!base::StringToSizeT(group_parts[i], &parsed[i])) {
NOTREACHED();
return limits;
}
}

size_t total_jobs = parsed.back();
parsed.pop_back();
for (size_t i = 0; i < parsed.size(); ++i) {
total_reserved_slots += parsed[i];
}

// There must be some unreserved slots available for the all priorities.
if (total_reserved_slots > total_jobs ||
(total_reserved_slots == total_jobs && parsed[MINIMUM_PRIORITY] == 0)) {
NOTREACHED();
return limits;
}

limits.total_jobs = total_jobs;
limits.reserved_slots = parsed;
return limits;
}

// Keeps track of the highest priority.
class PriorityTracker {
public:
Expand Down Expand Up @@ -2241,10 +2308,11 @@ class HostResolverManager::Job : public PrioritizedDispatcher::Job,

//-----------------------------------------------------------------------------

HostResolverManager::HostResolverManager(const Options& options,
NetLog* net_log)
HostResolverManager::HostResolverManager(
const HostResolver::ManagerOptions& options,
NetLog* net_log)
: max_queued_jobs_(0),
proc_params_(nullptr, options.max_retry_attempts),
proc_params_(nullptr, options.max_system_retry_attempts),
net_log_(net_log),
received_dns_config_(false),
num_dns_failures_(0),
Expand All @@ -2258,7 +2326,7 @@ HostResolverManager::HostResolverManager(const Options& options,
invalidation_in_progress_(false),
weak_ptr_factory_(this),
probe_weak_ptr_factory_(this) {
PrioritizedDispatcher::Limits job_limits = options.GetDispatcherLimits();
PrioritizedDispatcher::Limits job_limits = GetDispatcherLimits(options);
dispatcher_.reset(new PrioritizedDispatcher(job_limits));
max_queued_jobs_ = job_limits.total_jobs * 100u;

Expand Down
5 changes: 3 additions & 2 deletions net/dns/host_resolver_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include "base/timer/timer.h"
#include "net/base/completion_once_callback.h"
#include "net/base/network_change_notifier.h"
#include "net/base/prioritized_dispatcher.h"
#include "net/dns/dns_config.h"
#include "net/dns/dns_config_overrides.h"
#include "net/dns/host_cache.h"
Expand Down Expand Up @@ -84,7 +85,6 @@ class NET_EXPORT HostResolverManager
public NetworkChangeNotifier::DNSObserver {
public:
using MdnsListener = HostResolver::MdnsListener;
using Options = HostResolver::Options;
using ResolveHostRequest = HostResolver::ResolveHostRequest;
using ResolveHostParameters = HostResolver::ResolveHostParameters;

Expand All @@ -107,7 +107,8 @@ class NET_EXPORT HostResolverManager
// outstanding DNS transactions (not counting retransmissions and retries).
//
// |net_log| must remain valid for the life of the HostResolverManager.
HostResolverManager(const Options& options, NetLog* net_log);
HostResolverManager(const HostResolver::ManagerOptions& options,
NetLog* net_log);

// If any completion callbacks are pending when the resolver is destroyed,
// the host resolutions are cancelled, and the completion callbacks will not
Expand Down
Loading

0 comments on commit 4d635c1

Please sign in to comment.