DNS: Let requests specify a callback for future cache hits

DNS: Let requests specify a callback for future cache hits

To measure the effectiveness of various speculative DNS measures, allow
DNS requests to contain a cache hit callback that will be called every
time another request returns synchronously with cached data from the
same entry that the original request wrote or read.

To clarify, here's an example sequence of operations (assuming all
requests have callbacks set):

1. Request A resolves a name.
2. Request B resolves the same name while the result is still valid;
   the resolver calls request A's cache hit callback.
3. Request C resolves the same name again; the resolver calls the
   callbacks for both request A and B.
4. The cache is cleared on a network change.
5. Requests D and E both resolve the name. No callbacks are called,
   since there was no result cached for either request.
6. Request F resolves the name; the resolver calls the callbacks for
   requests D and E.

Notably, if multiple speculative resolutions occur, the cache hit
callbacks pile up, and future cache hits will call *all* of them.

BUG=621554
Committed: https://crrev.com/7e8bf1c42475e7bf0cf31a0cbda952cb6cae5f99
Cr-Commit-Position: refs/heads/master@{#409262}

[Julia Tuttle, 2016-06-20 16:32:47]

Description was changed from

==========
DNS: Let requests specify a callback for future cache hits

To measure the effectiveness of various speculative DNS measures, allow
DNS requests to contain a cache hit callback that will be called every
time another request returns synchronously with cached data from the
same entry that the original request wrote or read.

To clarify, here's an example sequence of operations (assuming all
requests have callbacks set):

1. Request A resolves a name.
2. Request B resolves the same name while the result is still valid;
   the resolver calls request A's cache hit callback.
3. Request C resolves the same name again; the resolver calls the
   callbacks for both request A and B.
4. The cache is cleared on a network change.
5. Requests D and E both resolve the name. No callbacks are called,
   since there was no result cached for either request.
6. Request F resolves the name; the resolver calls the callbacks for
   requests D and E.

Notably, if multiple speculative resolutions occur, the cache hit
callbacks pile up, and future cache hits will call *all* of them.

BUG=
==========

to

==========
DNS: Let requests specify a callback for future cache hits

To measure the effectiveness of various speculative DNS measures, allow
DNS requests to contain a cache hit callback that will be called every
time another request returns synchronously with cached data from the
same entry that the original request wrote or read.

To clarify, here's an example sequence of operations (assuming all
requests have callbacks set):

1. Request A resolves a name.
2. Request B resolves the same name while the result is still valid;
   the resolver calls request A's cache hit callback.
3. Request C resolves the same name again; the resolver calls the
   callbacks for both request A and B.
4. The cache is cleared on a network change.
5. Requests D and E both resolve the name. No callbacks are called,
   since there was no result cached for either request.
6. Request F resolves the name; the resolver calls the callbacks for
   requests D and E.

Notably, if multiple speculative resolutions occur, the cache hit
callbacks pile up, and future cache hits will call *all* of them.

BUG=621554
==========

[Julia Tuttle, 2016-07-22 20:42:00]

The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 20:42:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 22:06:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-22 22:06:56]

Dry run: This issue passed the CQ dry run.

[Julia Tuttle, 2016-07-25 19:31:17]

The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-25 19:32:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Julia Tuttle, 2016-07-25 19:32:27]

juliatuttle@chromium.org changed reviewers:
+ csharrison@chromium.org

[Julia Tuttle, 2016-07-25 19:32:28]

PTAL, csharrison.

[commit-bot: I haz the power, 2016-07-25 20:42:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-25 20:42:27]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-07-26 13:41:13]

Looks good!

https://codereview.chromium.org/2083643003/diff/40001/net/dns/host_resolver.cc
File net/dns/host_resolver.cc (right):

https://codereview.chromium.org/2083643003/diff/40001/net/dns/host_resolver.c...
net/dns/host_resolver.cc:93: HostResolver::RequestInfo::RequestInfo(const
HostPortPair& host_port_pair)
Can this constructor call into the no argument constructor to reduce the
boilerplate of default member initialization?

https://codereview.chromium.org/2083643003/diff/40001/net/dns/host_resolver_i...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/2083643003/diff/40001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:2309: ClearCacheHitCallbacks(key);
It is not quite clear why we clear the cache hit callback here. Could you add a
comment? It also might be clear if this call was hoisted into CompleteRequests.

https://codereview.chromium.org/2083643003/diff/40001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:2554: void
HostResolverImpl::ClearCacheHitCallbacks(const HostCache::Key& key) {
I don't think this method is really necessary.

https://codereview.chromium.org/2083643003/diff/40001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:2568: if (cache_hit_callbacks_.count(key) == 0u)
I think using find(key) is clearer here, and only requires looking up the key in
the map once because you can use the iterator to traverse the callback vector.

[Julia Tuttle, 2016-07-26 17:48:11]

The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-26 17:48:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Julia Tuttle, 2016-07-26 17:51:24]

PTAL, csharrison.

https://codereview.chromium.org/2083643003/diff/40001/net/dns/host_resolver.cc
File net/dns/host_resolver.cc (right):

https://codereview.chromium.org/2083643003/diff/40001/net/dns/host_resolver.c...
net/dns/host_resolver.cc:93: HostResolver::RequestInfo::RequestInfo(const
HostPortPair& host_port_pair)
On 2016/07/26 13:41:13, csharrison wrote:
> Can this constructor call into the no argument constructor to reduce the
> boilerplate of default member initialization?

Done.

https://codereview.chromium.org/2083643003/diff/40001/net/dns/host_resolver_i...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/2083643003/diff/40001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:2309: ClearCacheHitCallbacks(key);
On 2016/07/26 13:41:13, csharrison wrote:
> It is not quite clear why we clear the cache hit callback here. Could you add
a
> comment? It also might be clear if this call was hoisted into
CompleteRequests.

Done.

https://codereview.chromium.org/2083643003/diff/40001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:2554: void
HostResolverImpl::ClearCacheHitCallbacks(const HostCache::Key& key) {
On 2016/07/26 13:41:13, csharrison wrote:
> I don't think this method is really necessary.

Done.

https://codereview.chromium.org/2083643003/diff/40001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:2568: if (cache_hit_callbacks_.count(key) == 0u)
On 2016/07/26 13:41:13, csharrison wrote:
> I think using find(key) is clearer here, and only requires looking up the key
in
> the map once because you can use the iterator to traverse the callback vector.

Done.

[Charlie Harrison, 2016-07-26 18:52:15]

Thanks! Can you add a test for the cache hit case?

https://codereview.chromium.org/2083643003/diff/60001/net/dns/host_resolver.cc
File net/dns/host_resolver.cc (right):

https://codereview.chromium.org/2083643003/diff/60001/net/dns/host_resolver.c...
net/dns/host_resolver.cc:95: host_port_pair_ = host_port_pair;
nit: I think this can still be in the initializer list, after RequestInfo().

[commit-bot: I haz the power, 2016-07-26 19:06:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-26 19:06:15]

Dry run: This issue passed the CQ dry run.

[Julia Tuttle, 2016-08-01 18:08:08]

The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-01 18:08:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-01 19:35:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-01 19:35:21]

Dry run: This issue passed the CQ dry run.

[Julia Tuttle, 2016-08-02 16:56:04]

PTAL, csharrison.

https://codereview.chromium.org/2083643003/diff/60001/net/dns/host_resolver.cc
File net/dns/host_resolver.cc (right):

https://codereview.chromium.org/2083643003/diff/60001/net/dns/host_resolver.c...
net/dns/host_resolver.cc:95: host_port_pair_ = host_port_pair;
On 2016/07/26 18:52:15, csharrison wrote:
> nit: I think this can still be in the initializer list, after RequestInfo().

net/dns/host_resolver.cc:94:7: error: an initializer for a delegating
constructor must appear alone

[Charlie Harrison, 2016-08-02 17:15:41]

Few questions:

https://codereview.chromium.org/2083643003/diff/60001/net/dns/host_resolver.cc
File net/dns/host_resolver.cc (right):

https://codereview.chromium.org/2083643003/diff/60001/net/dns/host_resolver.c...
net/dns/host_resolver.cc:95: host_port_pair_ = host_port_pair;
On 2016/08/02 16:56:04, Julia Tuttle wrote:
> On 2016/07/26 18:52:15, csharrison wrote:
> > nit: I think this can still be in the initializer list, after RequestInfo().
> 
> net/dns/host_resolver.cc:94:7: error: an initializer for a delegating
> constructor must appear alone

Welp, proven wrong. My C++ foo is weak!

https://codereview.chromium.org/2083643003/diff/80001/net/dns/host_resolver_i...
File net/dns/host_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/2083643003/diff/80001/net/dns/host_resolver_i...
net/dns/host_resolver_impl_unittest.cc:2441: // Make an uncached request to
clear the cache hit callbacks.
Is this because it evicts the entries?

https://codereview.chromium.org/2083643003/diff/80001/net/dns/host_resolver_i...
net/dns/host_resolver_impl_unittest.cc:2451: // Make another request to make
sure neither callback is cleared.
s/neither callback is/both callbacks are/?

[Julia Tuttle, 2016-08-02 17:22:06]

The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-02 17:22:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Julia Tuttle, 2016-08-02 17:22:50]

PTAL, csharrison.

https://codereview.chromium.org/2083643003/diff/80001/net/dns/host_resolver_i...
File net/dns/host_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/2083643003/diff/80001/net/dns/host_resolver_i...
net/dns/host_resolver_impl_unittest.cc:2441: // Make an uncached request to
clear the cache hit callbacks.
On 2016/08/02 17:15:41, csharrison wrote:
> Is this because it evicts the entries?

Yes. Not necessarily *evicts*, really, but certainly overwrites. I added a
clarifying comment.

https://codereview.chromium.org/2083643003/diff/80001/net/dns/host_resolver_i...
net/dns/host_resolver_impl_unittest.cc:2451: // Make another request to make
sure neither callback is cleared.
On 2016/08/02 17:15:41, csharrison wrote:
> s/neither callback is/both callbacks are/?

Done.

[Charlie Harrison, 2016-08-02 17:23:45]

lgtm!!!

[commit-bot: I haz the power, 2016-08-02 18:27:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-02 18:27:25]

Dry run: This issue passed the CQ dry run.

[Julia Tuttle, 2016-08-02 19:02:35]

The CQ bit was checked by juliatuttle@chromium.org

[commit-bot: I haz the power, 2016-08-02 19:03:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-02 19:09:23]

Description was changed from

==========
DNS: Let requests specify a callback for future cache hits

To measure the effectiveness of various speculative DNS measures, allow
DNS requests to contain a cache hit callback that will be called every
time another request returns synchronously with cached data from the
same entry that the original request wrote or read.

To clarify, here's an example sequence of operations (assuming all
requests have callbacks set):

1. Request A resolves a name.
2. Request B resolves the same name while the result is still valid;
   the resolver calls request A's cache hit callback.
3. Request C resolves the same name again; the resolver calls the
   callbacks for both request A and B.
4. The cache is cleared on a network change.
5. Requests D and E both resolve the name. No callbacks are called,
   since there was no result cached for either request.
6. Request F resolves the name; the resolver calls the callbacks for
   requests D and E.

Notably, if multiple speculative resolutions occur, the cache hit
callbacks pile up, and future cache hits will call *all* of them.

BUG=621554
==========

to

==========
DNS: Let requests specify a callback for future cache hits

To measure the effectiveness of various speculative DNS measures, allow
DNS requests to contain a cache hit callback that will be called every
time another request returns synchronously with cached data from the
same entry that the original request wrote or read.

To clarify, here's an example sequence of operations (assuming all
requests have callbacks set):

1. Request A resolves a name.
2. Request B resolves the same name while the result is still valid;
   the resolver calls request A's cache hit callback.
3. Request C resolves the same name again; the resolver calls the
   callbacks for both request A and B.
4. The cache is cleared on a network change.
5. Requests D and E both resolve the name. No callbacks are called,
   since there was no result cached for either request.
6. Request F resolves the name; the resolver calls the callbacks for
   requests D and E.

Notably, if multiple speculative resolutions occur, the cache hit
callbacks pile up, and future cache hits will call *all* of them.

BUG=621554
==========

[commit-bot: I haz the power, 2016-08-02 19:09:24]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-02 19:10:38]

Description was changed from

==========
DNS: Let requests specify a callback for future cache hits

To measure the effectiveness of various speculative DNS measures, allow
DNS requests to contain a cache hit callback that will be called every
time another request returns synchronously with cached data from the
same entry that the original request wrote or read.

To clarify, here's an example sequence of operations (assuming all
requests have callbacks set):

1. Request A resolves a name.
2. Request B resolves the same name while the result is still valid;
   the resolver calls request A's cache hit callback.
3. Request C resolves the same name again; the resolver calls the
   callbacks for both request A and B.
4. The cache is cleared on a network change.
5. Requests D and E both resolve the name. No callbacks are called,
   since there was no result cached for either request.
6. Request F resolves the name; the resolver calls the callbacks for
   requests D and E.

Notably, if multiple speculative resolutions occur, the cache hit
callbacks pile up, and future cache hits will call *all* of them.

BUG=621554
==========

to

==========
DNS: Let requests specify a callback for future cache hits

To measure the effectiveness of various speculative DNS measures, allow
DNS requests to contain a cache hit callback that will be called every
time another request returns synchronously with cached data from the
same entry that the original request wrote or read.

To clarify, here's an example sequence of operations (assuming all
requests have callbacks set):

1. Request A resolves a name.
2. Request B resolves the same name while the result is still valid;
   the resolver calls request A's cache hit callback.
3. Request C resolves the same name again; the resolver calls the
   callbacks for both request A and B.
4. The cache is cleared on a network change.
5. Requests D and E both resolve the name. No callbacks are called,
   since there was no result cached for either request.
6. Request F resolves the name; the resolver calls the callbacks for
   requests D and E.

Notably, if multiple speculative resolutions occur, the cache hit
callbacks pile up, and future cache hits will call *all* of them.

BUG=621554

Committed: https://crrev.com/7e8bf1c42475e7bf0cf31a0cbda952cb6cae5f99
Cr-Commit-Position: refs/heads/master@{#409262}
==========

[commit-bot: I haz the power, 2016-08-02 19:10:40]

Patchset 6 (id:??) landed as
https://crrev.com/7e8bf1c42475e7bf0cf31a0cbda952cb6cae5f99
Cr-Commit-Position: refs/heads/master@{#409262}