Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1908543002:
DNS: Retain stale entries in HostCache and return when requested (Closed)
|
Created:
4 years, 8 months ago by Julia Tuttle Modified:
4 years, 7 months ago CC:
cbentzel+watch_chromium.org, chromium-reviews, mmenke Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDNS: Retain stale entries in HostCache and return when requested
In preparation for future connection optimizations, retain stale
entries (those with a response past the TTL or received on a different
network) in the cache, and expose a way to look up and characterize the
stale results.
To do this, HostCache moves away from using ExpiringCache to a self-
managed map of keys to entries, with extra metadata.
Also, add a bunch of histograms measuring how stale updated entries are.
BUG=605138
Committed: https://crrev.com/317860ed4f3e3788fd3c4637ee5d91afd31e39b1
Cr-Commit-Position: refs/heads/master@{#393246}
Patch Set 1 #Patch Set 2 : Add more histograms. #Patch Set 3 : Rebase, flesh out histograms, update histograms.xml. #Patch Set 4 : rebase, move FindDeltaType to dns_util, reformat #Patch Set 5 : Tweak histogram recording. #Patch Set 6 : rebase, tweak histograms a bit #
Total comments: 10
Patch Set 7 : make requested changes, and some others, and rebase #
Total comments: 6
Patch Set 8 : Tweak histograms per asvitkine's suggestions. #
Total comments: 48
Patch Set 9 : Make requested changes. #
Total comments: 8
Patch Set 10 : Make requested changes. #
Total comments: 4
Patch Set 11 : Rebase and rearrange .h slightly per sleevi's suggestion. #
Total comments: 18
Patch Set 12 : vertical whitespace tweak #Patch Set 13 : Make requested changes; notably, merge Entry{,Internal}. #
Total comments: 8
Patch Set 14 : Make a few more requested changes #Patch Set 15 : rebase #Patch Set 16 : Make a few more changes (discussed IRL) #
Total comments: 1
Patch Set 17 : rebase, make all members of Entry private #
Dependent Patchsets: Messages
Total messages: 34 (8 generated)
Description was changed from ========== WIP: DNS: Retain stale entries in HostCache and return when requested In preparation for future connection optimizations, retain stale entries (those with a response past the TTL or received on a different network) in the cache, and expose a way to look up and characterize the stale results. To do this, HostCache moves away from using ExpiringCache to a self- managed map of keys to entries, with extra metadata. TODO: 1. Currently, eviction just removes the 'first' entry. 2. Needs unittests. BUG=605138 ========== to ========== WIP: DNS: Retain stale entries in HostCache and return when requested In preparation for future connection optimizations, retain stale entries (those with a response past the TTL or received on a different network) in the cache, and expose a way to look up and characterize the stale results. To do this, HostCache moves away from using ExpiringCache to a self- managed map of keys to entries, with extra metadata. Also, add a bunch of histograms measuring how stale updated entries are. BUG=605138 ==========
juliatuttle@chromium.org changed reviewers: + rdsmith@chromium.org
Description was changed from ========== WIP: DNS: Retain stale entries in HostCache and return when requested In preparation for future connection optimizations, retain stale entries (those with a response past the TTL or received on a different network) in the cache, and expose a way to look up and characterize the stale results. To do this, HostCache moves away from using ExpiringCache to a self- managed map of keys to entries, with extra metadata. Also, add a bunch of histograms measuring how stale updated entries are. BUG=605138 ========== to ========== DNS: Retain stale entries in HostCache and return when requested In preparation for future connection optimizations, retain stale entries (those with a response past the TTL or received on a different network) in the cache, and expose a way to look up and characterize the stale results. To do this, HostCache moves away from using ExpiringCache to a self- managed map of keys to entries, with extra metadata. Also, add a bunch of histograms measuring how stale updated entries are. BUG=605138 ==========
PTAL, rdsmith.
juliatuttle@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine, PTAL at histograms.
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
Drive bys. The O(N^M) performance is bleh, but maybe it's too small to matter? Seems like you can do an O(N log N) solution if you're willing to trade memory overhead. https://codereview.chromium.org/1908543002/diff/100001/net/dns/dns_util.h File net/dns/dns_util.h (right): https://codereview.chromium.org/1908543002/diff/100001/net/dns/dns_util.h#new... net/dns/dns_util.h:45: // Used in histograms; please only insert new entries before MAX. If they're used in histograms, you should add explicit values. Inserting a value in between two entries means they would be renumbered and throw off the histograms. https://codereview.chromium.org/1908543002/diff/100001/net/dns/dns_util.h#new... net/dns/dns_util.h:45: // Used in histograms; please only insert new entries before MAX. Comment nit: While politeness is a virtue, it leaves ambiguity that it 'might be ok' to ignore that please. // Used in histograms. New values should be added at the end, before MAX https://codereview.chromium.org/1908543002/diff/100001/net/dns/dns_util.h#new... net/dns/dns_util.h:45: // Used in histograms; please only insert new entries before MAX. Comment nit: This doesn't explain the type // The result of comparing two AddressLists for the set of addresses // they contain and whether any differences exist. (Or something?) https://codereview.chromium.org/1908543002/diff/100001/net/dns/dns_util.h#new... net/dns/dns_util.h:47: // a and b contain the same addresses in the same order. nit: "a" and "b" are not defined here (they're an implementation of FindAddressListDeltaType). Perhaps "Both AddressLists contain the same addresses in the same order." (and similarly throughout) https://codereview.chromium.org/1908543002/diff/100001/net/dns/dns_util.h#new... net/dns/dns_util.h:55: MAX_DELTA_TYPE naming: Given that you adopted ENUM_PREFIX naming, shouldn't this be DELTA_MAX_VALUE ? https://codereview.chromium.org/1908543002/diff/100001/net/dns/dns_util.h#new... net/dns/dns_util.h:58: AddressListDeltaType FindAddressListDeltaType(const AddressList& a, Document? :)
https://codereview.chromium.org/1908543002/diff/100001/net/dns/dns_util.h File net/dns/dns_util.h (right): https://codereview.chromium.org/1908543002/diff/100001/net/dns/dns_util.h#new... net/dns/dns_util.h:45: // Used in histograms; please only insert new entries before MAX. On 2016/04/26 at 02:18:22, Ryan Sleevi wrote: > If they're used in histograms, you should add explicit values. Inserting a value in between two entries means they would be renumbered and throw off the histograms. Done (all three; the new rietveld UI won't let me reply to individual comments, just the "thread" attached to the line). https://codereview.chromium.org/1908543002/diff/100001/net/dns/dns_util.h#new... net/dns/dns_util.h:47: // a and b contain the same addresses in the same order. On 2016/04/26 at 02:18:22, Ryan Sleevi wrote: > nit: "a" and "b" are not defined here (they're an implementation of FindAddressListDeltaType). Perhaps "Both AddressLists contain the same addresses in the same order." (and similarly throughout) Done. https://codereview.chromium.org/1908543002/diff/100001/net/dns/dns_util.h#new... net/dns/dns_util.h:55: MAX_DELTA_TYPE On 2016/04/26 at 02:18:22, Ryan Sleevi wrote: > naming: Given that you adopted ENUM_PREFIX naming, shouldn't this be DELTA_MAX_VALUE ? I usually make an exception for the max value constant. Is that bad? https://codereview.chromium.org/1908543002/diff/100001/net/dns/dns_util.h#new... net/dns/dns_util.h:58: AddressListDeltaType FindAddressListDeltaType(const AddressList& a, On 2016/04/26 at 02:18:22, Ryan Sleevi wrote: > Document? :) Done (the new rietveld UI has no button for this :( ).
https://codereview.chromium.org/1908543002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1908543002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:8727: +<histogram name="DNS.HostCacheClearStaleExpiredBy" units="ms"> Nit: I suggest adding an intermediate . to make these more readable. e.g. DNS.HostCache.ClearStaleExpiredBy https://codereview.chromium.org/1908543002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:8857: +<histogram name="DNS.HostCacheUpdateStaleNetworkChanges"> Nit: Add units=. Same everywhere else where you don't have an enum=. https://codereview.chromium.org/1908543002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:89485: +<histogram_suffixes name="DNS.HostCacheUpdateStaleDeltaType"> Nit: Up to you, but I find using . as separator to be a bit more pleasant than the default of _.
https://codereview.chromium.org/1908543002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1908543002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:8727: +<histogram name="DNS.HostCacheClearStaleExpiredBy" units="ms"> On 2016/04/27 at 15:45:33, Alexei Svitkine wrote: > Nit: I suggest adding an intermediate . to make these more readable. > > e.g. DNS.HostCache.ClearStaleExpiredBy Done. https://codereview.chromium.org/1908543002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:8857: +<histogram name="DNS.HostCacheUpdateStaleNetworkChanges"> On 2016/04/27 at 15:45:33, Alexei Svitkine wrote: > Nit: Add units=. Same everywhere else where you don't have an enum=. Done. https://codereview.chromium.org/1908543002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:89485: +<histogram_suffixes name="DNS.HostCacheUpdateStaleDeltaType"> On 2016/04/27 at 15:45:33, Alexei Svitkine wrote: > Nit: Up to you, but I find using . as separator to be a bit more pleasant than the default of _. I prefer . for the general hierarchy but _ for the specific "recorded only in these instances" meaning.
lgtm
https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.cc File net/dns/host_cache.cc (right): https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.cc#... net/dns/host_cache.cc:305: base::ListValue* entry_list = new base::ListValue(); better to use scoped_ptr<base::ListValue> entry_list(new base::ListValue()); and then for your return return std::move(entry_list) (which will convert to the base::Value). Mostly, it keeps the memory safer if a short-circuit was ever added. https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.cc#... net/dns/host_cache.cc:307: for (auto& pair : entries_) { const auto? https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.cc#... net/dns/host_cache.cc:335: /* const HostCache::EntryMap& HostCache::entries() const { Leftover to delete? https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.cc#... net/dns/host_cache.cc:398: return oldest_it; My Kingdom for C++14, he laments! return std::min_element(entries_.begin(), entries_.end(), [](const auto& lhs, const auto& rhs) -> bool { return lhs.second.expires < rhs.second.expires; }); generic lambdas aren't until C++14; otherwise, it'd have to be grossness like [](const EntryMap::value_type& lhs, const EntryMap::value_type& rhs) -> bool { return lhs.second.expires < rhs.second.expires; }); (Assuming you don't have to specify HostCache before EntryMap because the lexical scoping of the lambda is within the HostCache itself) https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h File net/dns/host_cache.h (right): https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h#n... net/dns/host_cache.h:77: return expired_by >= base::TimeDelta() || network_changes > 0; nit: network_changes > 0 first (to short-circuit the overhead), then the construction of the object? https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h#n... net/dns/host_cache.h:82: // poke at it. At least it's not NET_EXPORTed. Is the TODO meaning that you're planning to address this? Having private member-function histogram functions leak into this header (e.g. via the "private:" section) seems way better than spraying this into the header. https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h#n... net/dns/host_cache.h:112: const Entry* LookupStale(const Key& key, Document? https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h#n... net/dns/host_cache.h:137: std::unique_ptr<base::Value> GetEntriesAsValue() const; Document? This feels a little weird because it's now exposing something explicitly for NetLog as an API contract. Lessons have been this can backfire when people 'depend' on it. So documentation here about the "here be dragons" may help https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h#n... net/dns/host_cache.h:150: EntryMap::iterator SelectEntryToEvict(); Document?
First round of comments. https://codereview.chromium.org/1908543002/diff/140001/net/dns/dns_util.cc File net/dns/dns_util.cc (right): https://codereview.chromium.org/1908543002/diff/140001/net/dns/dns_util.cc#ne... net/dns/dns_util.cc:196: AddressListDeltaType FindAddressListDeltaType(const AddressList& a, This is only used in host_cache.cc; any reason not to make it a static function in that file, at least until it's actually needed elsewhere? (Or is it needed elsewhere in one of the follow-on CLs? In which case, in an ideal world I'd suggest starting in host_cache.cc and moving in that CL, and in this world, ok. :-}) https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.cc File net/dns/host_cache.cc (right): https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.cc#... net/dns/host_cache.cc:112: void HistogramLookup(LookupOutcome outcome, nit, suggestion: I'd rather have these functions named "HistorgramRecord*" (or maybe just Record*) as what they're doing is recording info (rather than, e.g., HistogramErase erasing histograms). https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.cc#... net/dns/host_cache.cc:221: const HostCache::Entry* HostCache::LookupStale( The implicit API contract of this function (return stale entries iff stale_out is not null) seems questionable to me; I'd rather have an API contract where stale_out's nullity only controlled what was written into the structure it points to. https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.cc#... net/dns/host_cache.cc:243: stale_out->stale_hits = ++it->second.stale_hits; It strikes me as really weird that we only update this single entry in StaleEntryInfo and leave the rest untouched. I could probably understand better whether this made sense with documentation on the API contract for this function; could you provide that? (I'll say ahead of time I dislike having two separate return pathways, both returning structures, from the same function; I'd rather have a function that returns a single structure. Maybe Make this void*, filling in StaleEntryInfo, and have a const pointer to the Entry as a member of that structure? But feel free just to document answering my concern above, and I'll make suggestions or not on what I'd like to see differently at that point.) (I'm also bothered by setting different portions of a structure in different locations; I like to have structures internally consistent all the time. But I don't yet understand well enough how you're using StaleEntryInfo to suggest changes.) https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.cc#... net/dns/host_cache.cc:272: Evict(now); (Throw out all stale entries when we reach max? What about a more gradual, LRU policy?) https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.cc#... net/dns/host_cache.cc:305: base::ListValue* entry_list = new base::ListValue(); On 2016/04/28 01:44:24, Ryan Sleevi wrote: > better to use > > scoped_ptr<base::ListValue> entry_list(new base::ListValue()); > > and then for your return > return std::move(entry_list) > > > (which will convert to the base::Value). > > Mostly, it keeps the memory safer if a short-circuit was ever added. +1 except use std::unique_ptr. https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h File net/dns/host_cache.h (right): https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h#n... net/dns/host_cache.h:34: The comments and code seem crowded together enough in this class that I'm not sure I see the point of whitespace to separate constructors and destructor. But it's aesthetics; up to you. https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h#n... net/dns/host_cache.h:69: // Time since the entry's TTL has expired. Negative if not expired. If it might not be expired, it's not really StaleEntryInfo. EntryExpirationInfo? https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h#n... net/dns/host_cache.h:71: // Number of network changes since this result was cached, or 0u if none. nit, suggestion: I find the comma following phrase redundant; what else would 0u mean and how else would it be expressed? https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h#n... net/dns/host_cache.h:74: unsigned stale_hits; nit: The style guide forbids using built-in types other than int. https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h#n... net/dns/host_cache.h:89: base::TimeTicks added; nit: Document meanings of member variables? The use of network_changes is specifically slightly subtle. https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h#n... net/dns/host_cache.h:125: // (Does not actually remove them from memory.) nit, suggestion: I think the fact that the interface now exposes getters for stale entries makes the parenthesized comment redundant. https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h#n... net/dns/host_cache.h:137: std::unique_ptr<base::Value> GetEntriesAsValue() const; On 2016/04/28 01:44:24, Ryan Sleevi wrote: > Document? > > This feels a little weird because it's now exposing something explicitly for > NetLog as an API contract. Lessons have been this can backfire when people > 'depend' on it. So documentation here about the "here be dragons" may help I agree. I'm willing to accept this interface, but the old way (in which the net_log pickling routine used the public API to pull out information and put it in Value/json form) strikes me as better if there's no major down sides to it. https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h#n... net/dns/host_cache.h:148: void Evict(base::TimeTicks now); Document? When I first encountered this, I presumed it was tossing all entries that were stale, which I didn't like. A single entry is better, but it should be documented/named EvictAnEntry or something. https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h#n... net/dns/host_cache.h:150: EntryMap::iterator SelectEntryToEvict(); Why does this exist? It's only called from Evict()?
PTAL. https://codereview.chromium.org/1908543002/diff/140001/net/dns/dns_util.cc File net/dns/dns_util.cc (right): https://codereview.chromium.org/1908543002/diff/140001/net/dns/dns_util.cc#ne... net/dns/dns_util.cc:196: AddressListDeltaType FindAddressListDeltaType(const AddressList& a, On 2016/04/28 at 16:24:16, Randy Smith - Not in Fridays wrote: > This is only used in host_cache.cc; any reason not to make it a static function in that file, at least until it's actually needed elsewhere? (Or is it needed elsewhere in one of the follow-on CLs? In which case, in an ideal world I'd suggest starting in host_cache.cc and moving in that CL, and in this world, ok. :-}) Yeah, it's used in a follow-on CL. https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.cc File net/dns/host_cache.cc (right): https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.cc#... net/dns/host_cache.cc:112: void HistogramLookup(LookupOutcome outcome, On 2016/04/28 16:24:16, Randy Smith - Not in Fridays wrote: > nit, suggestion: I'd rather have these functions named "HistorgramRecord*" (or > maybe just Record*) as what they're doing is recording info (rather than, e.g., > HistogramErase erasing histograms). Done. https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.cc#... net/dns/host_cache.cc:221: const HostCache::Entry* HostCache::LookupStale( On 2016/04/28 16:24:16, Randy Smith - Not in Fridays wrote: > The implicit API contract of this function (return stale entries iff stale_out > is not null) seems questionable to me; I'd rather have an API contract where > stale_out's nullity only controlled what was written into the structure it > points to. Agreed. Changed. https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.cc#... net/dns/host_cache.cc:243: stale_out->stale_hits = ++it->second.stale_hits; On 2016/04/28 16:24:16, Randy Smith - Not in Fridays wrote: > It strikes me as really weird that we only update this single entry in > StaleEntryInfo and leave the rest untouched. I could probably understand better > whether this made sense with documentation on the API contract for this > function; could you provide that? > > (I'll say ahead of time I dislike having two separate return pathways, both > returning structures, from the same function; I'd rather have a function that > returns a single structure. Maybe Make this void*, filling in StaleEntryInfo, > and have a const pointer to the Entry as a member of that structure? But feel > free just to document answering my concern above, and I'll make suggestions or > not on what I'd like to see differently at that point.) > > (I'm also bothered by setting different portions of a structure in different > locations; I like to have structures internally consistent all the time. But I > don't yet understand well enough how you're using StaleEntryInfo to suggest > changes.) Agreed. Changed. https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.cc#... net/dns/host_cache.cc:272: Evict(now); On 2016/04/28 16:24:16, Randy Smith - Not in Fridays wrote: > (Throw out all stale entries when we reach max? What about a more gradual, LRU > policy?) Wrong, it evicts the oldest. I've renamed Evict to EvictOneEntry to clarify. We don't currently track LRU, but we could. https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.cc#... net/dns/host_cache.cc:305: base::ListValue* entry_list = new base::ListValue(); On 2016/04/28 16:24:16, Randy Smith - Not in Fridays wrote: > On 2016/04/28 01:44:24, Ryan Sleevi wrote: > > better to use > > > > scoped_ptr<base::ListValue> entry_list(new base::ListValue()); > > > > and then for your return > > return std::move(entry_list) > > > > > > (which will convert to the base::Value). > > > > Mostly, it keeps the memory safer if a short-circuit was ever added. > > +1 except use std::unique_ptr. Done. https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.cc#... net/dns/host_cache.cc:307: for (auto& pair : entries_) { On 2016/04/28 01:44:24, Ryan Sleevi wrote: > const auto? Done. https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.cc#... net/dns/host_cache.cc:335: /* const HostCache::EntryMap& HostCache::entries() const { On 2016/04/28 01:44:24, Ryan Sleevi wrote: > Leftover to delete? Indeed. Deleted. https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h File net/dns/host_cache.h (right): https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h#n... net/dns/host_cache.h:34: On 2016/04/28 at 16:24:16, Randy Smith - Not in Fridays wrote: > The comments and code seem crowded together enough in this class that I'm not sure I see the point of whitespace to separate constructors and destructor. But it's aesthetics; up to you. Fixed. https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h#n... net/dns/host_cache.h:69: // Time since the entry's TTL has expired. Negative if not expired. On 2016/04/28 at 16:24:16, Randy Smith - Not in Fridays wrote: > If it might not be expired, it's not really StaleEntryInfo. EntryExpirationInfo? EntryStaleness. https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h#n... net/dns/host_cache.h:71: // Number of network changes since this result was cached, or 0u if none. On 2016/04/28 at 16:24:16, Randy Smith - Not in Fridays wrote: > nit, suggestion: I find the comma following phrase redundant; what else would 0u mean and how else would it be expressed? Removed. https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h#n... net/dns/host_cache.h:74: unsigned stale_hits; On 2016/04/28 at 16:24:16, Randy Smith - Not in Fridays wrote: > nit: The style guide forbids using built-in types other than int. Done. (Does it not allow size_t either?) https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h#n... net/dns/host_cache.h:77: return expired_by >= base::TimeDelta() || network_changes > 0; On 2016/04/28 at 01:44:25, Ryan Sleevi wrote: > nit: network_changes > 0 first (to short-circuit the overhead), then the construction of the object? Done. https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h#n... net/dns/host_cache.h:82: // poke at it. At least it's not NET_EXPORTed. On 2016/04/28 at 01:44:25, Ryan Sleevi wrote: > Is the TODO meaning that you're planning to address this? Having private member-function histogram functions leak into this header (e.g. via the "private:" section) seems way better than spraying this into the header. I'm removing the TODO. I need the struct here because I don't think I Can declare a map with an incomplete type. I'd move the histograms here to avoid having to pass in network_changes, but then I'd need to move the enums here too. https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h#n... net/dns/host_cache.h:89: base::TimeTicks added; On 2016/04/28 at 16:24:16, Randy Smith - Not in Fridays wrote: > nit: Document meanings of member variables? The use of network_changes is specifically slightly subtle. Done; I think the others are self-evident. https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h#n... net/dns/host_cache.h:112: const Entry* LookupStale(const Key& key, On 2016/04/28 at 01:44:25, Ryan Sleevi wrote: > Document? Done. https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h#n... net/dns/host_cache.h:125: // (Does not actually remove them from memory.) On 2016/04/28 at 16:24:16, Randy Smith - Not in Fridays wrote: > nit, suggestion: I think the fact that the interface now exposes getters for stale entries makes the parenthesized comment redundant. Removed. https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h#n... net/dns/host_cache.h:137: std::unique_ptr<base::Value> GetEntriesAsValue() const; On 2016/04/28 at 16:24:16, Randy Smith - Not in Fridays wrote: > On 2016/04/28 01:44:24, Ryan Sleevi wrote: > > Document? > > > > This feels a little weird because it's now exposing something explicitly for > > NetLog as an API contract. Lessons have been this can backfire when people > > 'depend' on it. So documentation here about the "here be dragons" may help > > I agree. I'm willing to accept this interface, but the old way (in which the net_log pickling routine used the public API to pull out information and put it in Value/json form) strikes me as better if there's no major down sides to it. Would appending "ForNetLogOnly" do it? I wanted to avoid having to re-export a bunch of entry metadata. I guess I can make EntryInternal public (and rename it) and just expose the map itself if you prefer that. https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h#n... net/dns/host_cache.h:148: void Evict(base::TimeTicks now); On 2016/04/28 16:24:16, Randy Smith - Not in Fridays wrote: > Document? When I first encountered this, I presumed it was tossing all entries > that were stale, which I didn't like. A single entry is better, but it should > be documented/named EvictAnEntry or something. Renamed to EvictOneEntry. https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h#n... net/dns/host_cache.h:150: EntryMap::iterator SelectEntryToEvict(); On 2016/04/28 16:24:16, Randy Smith - Not in Fridays wrote: > Why does this exist? It's only called from Evict()? I was imagining a more complex algorithm, but there isn't one yet, so I've merged it back in.
I'll let Randy drive the rest. https://codereview.chromium.org/1908543002/diff/160001/net/dns/host_cache.cc File net/dns/host_cache.cc (right): https://codereview.chromium.org/1908543002/diff/160001/net/dns/host_cache.cc#... net/dns/host_cache.cc:280: // TODO(juliatuttle): Remember some old metadata, if it's useful? I generally push on TODOs to include a little more context as to what you were thinking at the time (e.g. now) https://codereview.chromium.org/1908543002/diff/160001/net/dns/host_cache.cc#... net/dns/host_cache.cc:299: void HostCache::clear() { Seems like this should be renamed Clear(), because it's no longer "trivial" https://codereview.chromium.org/1908543002/diff/160001/net/dns/host_cache.h File net/dns/host_cache.h (right): https://codereview.chromium.org/1908543002/diff/160001/net/dns/host_cache.h#n... net/dns/host_cache.h:80: struct EntryInternal : public Entry { Well, it's not really Internal now. Perhaps a comment update? As it stands, there's now no way to publicly get access to an EntryInternal, nor to the EntryMap. So shouldn't this be made private? (IIRC - and I may be wrong - the value type for a map can be forward declared until the point it's instantiated, but the key type must be visible. Since the ctor/dtor is private, you SHOULD be fine making this type opaque) https://codereview.chromium.org/1908543002/diff/160001/net/dns/host_cache.h#n... net/dns/host_cache.h:101: // Ditto. Ditto what? This doesn't seem to need to be public anymore, now that EntryMap isn't exposed (line 105)
Patchset #10 (id:180001) has been deleted
PTAL. https://codereview.chromium.org/1908543002/diff/160001/net/dns/host_cache.cc File net/dns/host_cache.cc (right): https://codereview.chromium.org/1908543002/diff/160001/net/dns/host_cache.cc#... net/dns/host_cache.cc:280: // TODO(juliatuttle): Remember some old metadata, if it's useful? On 2016/04/29 23:09:37, Ryan Sleevi wrote: > I generally push on TODOs to include a little more context as to what you were > thinking at the time (e.g. now) Done. https://codereview.chromium.org/1908543002/diff/160001/net/dns/host_cache.cc#... net/dns/host_cache.cc:299: void HostCache::clear() { On 2016/04/29 23:09:38, Ryan Sleevi wrote: > Seems like this should be renamed Clear(), because it's no longer "trivial" Should it? The histograms seem like an implementation detail, but the visible behavior is still trivial. https://codereview.chromium.org/1908543002/diff/160001/net/dns/host_cache.h File net/dns/host_cache.h (right): https://codereview.chromium.org/1908543002/diff/160001/net/dns/host_cache.h#n... net/dns/host_cache.h:80: struct EntryInternal : public Entry { On 2016/04/29 23:09:38, Ryan Sleevi wrote: > Well, it's not really Internal now. Perhaps a comment update? > > As it stands, there's now no way to publicly get access to an EntryInternal, nor > to the EntryMap. So shouldn't this be made private? > > (IIRC - and I may be wrong - the value type for a map can be forward declared > until the point it's instantiated, but the key type must be visible. Since the > ctor/dtor is private, you SHOULD be fine making this type opaque) Done. This is *much* better, thank you. https://codereview.chromium.org/1908543002/diff/160001/net/dns/host_cache.h#n... net/dns/host_cache.h:101: // Ditto. On 2016/04/29 23:09:38, Ryan Sleevi wrote: > Ditto what? This doesn't seem to need to be public anymore, now that EntryMap > isn't exposed (line 105) Done.
One last style nit. THanks for bearing through the style chime-in. I mostly focused on the interface/documentation, rather than the implementation, but looks good/makes sense to me [e.g. seems maintainable, edge weirdness documented]. Randy should double check the nitty gritty though :) https://codereview.chromium.org/1908543002/diff/200001/net/dns/host_cache.h File net/dns/host_cache.h (right): https://codereview.chromium.org/1908543002/diff/200001/net/dns/host_cache.h#n... net/dns/host_cache.h:131: enum EraseReason : int; style: These enums should go up with 124/125 (as typedefs/forward declarations), per https://google.github.io/styleguide/cppguide.html#Declaration_Order
Thanks for all the style help, rsleevi! PTAL, rdsmith. https://codereview.chromium.org/1908543002/diff/200001/net/dns/host_cache.h File net/dns/host_cache.h (right): https://codereview.chromium.org/1908543002/diff/200001/net/dns/host_cache.h#n... net/dns/host_cache.h:131: enum EraseReason : int; On 2016/05/04 01:12:34, Ryan Sleevi wrote: > style: These enums should go up with 124/125 (as typedefs/forward declarations), > per https://google.github.io/styleguide/cppguide.html#Declaration_Order Done.
Next round of comments. I'm going to wait on the documentation I requested to review the tests (because it's easier for me, and given the number of reviews I'm doing for you I think that means I'll spend less time blocking you if I wait :-}) and I want to put some more careful thought into the histograms you're collecting and probably spend some time comparing ExpiringCache to the new semantics, but I don't anticipate any major changes past this review. https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.cc File net/dns/host_cache.cc (right): https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.cc#... net/dns/host_cache.cc:272: Evict(now); On 2016/04/29 17:33:51, Julia Tuttle wrote: > On 2016/04/28 16:24:16, Randy Smith - Not in Fridays wrote: > > (Throw out all stale entries when we reach max? What about a more gradual, > LRU > > policy?) > > Wrong, it evicts the oldest. I've renamed Evict to EvictOneEntry to clarify. > > We don't currently track LRU, but we could. Sorry, this was my bad--I'm trying an experiment in reviewing these CLs of making my notes to myself as I go in parens to come back to, and I missed this one. In the future, if you see notes like this, feel free to call me on them or ignore them--they shouldn't make it into the published comments. https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h File net/dns/host_cache.h (right): https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h#n... net/dns/host_cache.h:74: unsigned stale_hits; On 2016/04/29 17:33:52, Julia Tuttle wrote: > On 2016/04/28 at 16:24:16, Randy Smith - Not in Fridays wrote: > > nit: The style guide forbids using built-in types other than int. > > Done. (Does it not allow size_t either?) size_t isn't a built-in type :-}. But yes, size_t is ok. Quoting from the guide: "<stdint.h> defines types like int16_t, uint32_t, int64_t, etc. You should always use those in preference to short, unsigned long long and the like, when you need a guarantee on the size of an integer. Of the C integer types, only int should be used. When appropriate, you are welcome to use standard types like size_t and ptrdiff_t." https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h#n... net/dns/host_cache.h:137: std::unique_ptr<base::Value> GetEntriesAsValue() const; On 2016/04/29 17:33:52, Julia Tuttle wrote: > On 2016/04/28 at 16:24:16, Randy Smith - Not in Fridays wrote: > > On 2016/04/28 01:44:24, Ryan Sleevi wrote: > > > Document? > > > > > > This feels a little weird because it's now exposing something explicitly for > > > NetLog as an API contract. Lessons have been this can backfire when people > > > 'depend' on it. So documentation here about the "here be dragons" may help > > > > I agree. I'm willing to accept this interface, but the old way (in which the > net_log pickling routine used the public API to pull out information and put it > in Value/json form) strikes me as better if there's no major down sides to it. > > Would appending "ForNetLogOnly" do it? > > I wanted to avoid having to re-export a bunch of entry metadata. I guess I can > make EntryInternal public (and rename it) and just expose the map itself if you > prefer that. So there's a balancing act here. I'm ok with the interface as given if there's real value in logging in the netlog stuff that's not available through the public interface (which I take as being the Entry structure), and I'm happy to take your word that there's value from a debugging perspective. Having said that, looking at what's returned by this function looks like it's all from Entry--can you clarify what private information you're wanting to share with the netlog that public consumers can't access through Entry? (I'll also note that I personally think a friend declaration is another fine way to solve this problem, since what we're trying to say is that we're giving privileges to the net log that other consumers don't have. But I'd run that by Matt before doing it :-}.) https://codereview.chromium.org/1908543002/diff/200001/net/dns/dns_util.cc File net/dns/dns_util.cc (right): https://codereview.chromium.org/1908543002/diff/200001/net/dns/dns_util.cc#ne... net/dns/dns_util.cc:200: bool any_missing = false; Suggestion: I apologize for the persnicketyness, but it took me enough puzzling through corner cases to verify this function that I'd like a bit better documentation, specifically for the above two variables (can be new variable names). Maybe any_match -> some_overlap, and any_missing -> some_disjoint ("any_missing" is in the context of searching for a specific value in the full set, i.e. it's about the algorithm used, rather than about the result, at least that's how I read it). https://codereview.chromium.org/1908543002/diff/200001/net/dns/dns_util.cc#ne... net/dns/dns_util.cc:220: return DELTA_REORDERED; Can the address lists ever have duplicates? Does that change this logic? (I'm thinking of one list having one entry duplicated, and the other having a different entry duplicated, which I think would result in a reordered, but I'm not sure it should?) Regardless, worth documenting expectations around duplicates at the interface level. https://codereview.chromium.org/1908543002/diff/220001/net/dns/host_cache.cc File net/dns/host_cache.cc (right): https://codereview.chromium.org/1908543002/diff/220001/net/dns/host_cache.cc#... net/dns/host_cache.cc:31: struct HostCache::EntryInternal : public HostCache::Entry { nit, suggestion: I'm somewhat uncomfortable with the existence of this structure and whether it's worth having a secondary structure to Entry. Did you consider storing all this information in Entry, but making it private and only accessible to the HostCache? I'm also uncomfortable with inheritance rather than inclusion, as it feels like the inheritance is being used for implementation simplicity rather than because there's a real ISA relationship between the two classes. But that's less of a big deal because this class has much smaller visibility than Entry; it doesn't matter too much playing those games within a single .cc file. https://codereview.chromium.org/1908543002/diff/220001/net/dns/host_cache.cc#... net/dns/host_cache.cc:37: base::TimeTicks expires; Why isn't this in Entry in place of ttl? It seems like it would be much more valuable to the caller than ttl, and the same information isn't really needed in both places, is it? https://codereview.chromium.org/1908543002/diff/220001/net/dns/host_cache.cc#... net/dns/host_cache.cc:111: if (entry->IsStale(now, network_changes_)) { Is this a behavior change to Lookup() with this CL? IIUC, network changes didn't use to make entries stale, did they? https://codereview.chromium.org/1908543002/diff/220001/net/dns/host_cache.cc#... net/dns/host_cache.cc:116: entry->CountHit(/* hit_is_stale= */ false); Consistency nit: Include the comment in all calls or none (my preference is all, but up to you). https://codereview.chromium.org/1908543002/diff/220001/net/dns/host_cache.cc#... net/dns/host_cache.cc:285: if (it->second.expires < oldest_it->second.expires) What is your thinking around expiring the strictly oldest as opposed to expiring stale before fresh? I could imagine fresh entries with long ttls being older than stale entries. https://codereview.chromium.org/1908543002/diff/220001/net/dns/host_cache.cc#... net/dns/host_cache.cc:300: // Nothing to log here. I don't understand why this comment only applies to SET_INSERT and not SET_UPDATE_VALID; the existence of the comment makes me feel like there's some subtle difference between the two. Is there? https://codereview.chromium.org/1908543002/diff/220001/net/dns/host_cache.cc#... net/dns/host_cache.cc:364: // Nothing to log here. Same confusion as to why this comment applies here and not to the two below. https://codereview.chromium.org/1908543002/diff/220001/net/dns/host_cache.h File net/dns/host_cache.h (left): https://codereview.chromium.org/1908543002/diff/220001/net/dns/host_cache.h#o... net/dns/host_cache.h:77: EvictionHandler> EntryMap; I don't have a sense as to the usage of ExpiringCache in the system as a whole; does this removal imply that we should queue getting rid of it in other places because it's more heavyweight than needed if the host cache isn't using it? https://codereview.chromium.org/1908543002/diff/220001/net/dns/host_cache_uni... File net/dns/host_cache_unittest.cc (right): https://codereview.chromium.org/1908543002/diff/220001/net/dns/host_cache_uni... net/dns/host_cache_unittest.cc:299: TEST(HostCacheTest, Evict) { Any chance you'd be willing to add a description of what each test does before the function? Often understanding the tests is the hardest part of making changes in an unfamiliar section of the code base. The description doesn't have to be very long--just a sentence to give the reader an understanding of what functionality the test probes.
PTAL. https://codereview.chromium.org/1908543002/diff/220001/net/dns/host_cache.cc File net/dns/host_cache.cc (right): https://codereview.chromium.org/1908543002/diff/220001/net/dns/host_cache.cc#... net/dns/host_cache.cc:31: struct HostCache::EntryInternal : public HostCache::Entry { On 2016/05/04 18:15:25, Randy Smith - Not in Fridays wrote: > nit, suggestion: I'm somewhat uncomfortable with the existence of this structure > and whether it's worth having a secondary structure to Entry. Did you consider > storing all this information in Entry, but making it private and only accessible > to the HostCache? > > I'm also uncomfortable with inheritance rather than inclusion, as it feels like > the inheritance is being used for implementation simplicity rather than because > there's a real ISA relationship between the two classes. But that's less of a > big deal because this class has much smaller visibility than Entry; it doesn't > matter too much playing those games within a single .cc file. I thought about it, but did not think about making it private. I didn't want to expose them as public, because it seemed rude to expect the caller to see and deal with them. (I grew up using Java, so friend declarations don't occur to me easily.) This is *much* nicer with the cache metadata as private members and HostCache as a friend. Thanks for the suggestion. https://codereview.chromium.org/1908543002/diff/220001/net/dns/host_cache.cc#... net/dns/host_cache.cc:37: base::TimeTicks expires; On 2016/05/04 18:15:25, Randy Smith - Not in Fridays wrote: > Why isn't this in Entry in place of ttl? It seems like it would be much more > valuable to the caller than ttl, and the same information isn't really needed in > both places, is it? I was trying not to alter the interface, for no practical reason. It looks like the TTL is histogrammed in HostResolverImpl, but that's it. The only user who'd like *expiration* is WebUI, so I don't know if it's worth changing it for that. https://codereview.chromium.org/1908543002/diff/220001/net/dns/host_cache.cc#... net/dns/host_cache.cc:111: if (entry->IsStale(now, network_changes_)) { On 2016/05/04 18:15:25, Randy Smith - Not in Fridays wrote: > Is this a behavior change to Lookup() with this CL? IIUC, network changes > didn't use to make entries stale, did they? No; network changes clear the cache right now. This is just preparing to make network changes lighter-weight -- we can alter the owner to only call OnNetworkChange, and then the entries will be stale instead of gone. https://codereview.chromium.org/1908543002/diff/220001/net/dns/host_cache.cc#... net/dns/host_cache.cc:116: entry->CountHit(/* hit_is_stale= */ false); On 2016/05/04 18:15:25, Randy Smith - Not in Fridays wrote: > Consistency nit: Include the comment in all calls or none (my preference is all, > but up to you). Oh, whoops, thanks. (I hadn't included it in the other version because the argument I was passing was "is_stale", but it's better to be clearer.) https://codereview.chromium.org/1908543002/diff/220001/net/dns/host_cache.cc#... net/dns/host_cache.cc:285: if (it->second.expires < oldest_it->second.expires) On 2016/05/04 18:15:25, Randy Smith - Not in Fridays wrote: > What is your thinking around expiring the strictly oldest as opposed to expiring > stale before fresh? I could imagine fresh entries with long ttls being older > than stale entries. Maybe, but I'd rather tinker with eviction strategies in another CL. This *should* mirror the behavior of the original cache for the most part. https://codereview.chromium.org/1908543002/diff/220001/net/dns/host_cache.cc#... net/dns/host_cache.cc:300: // Nothing to log here. On 2016/05/04 18:15:25, Randy Smith - Not in Fridays wrote: > I don't understand why this comment only applies to SET_INSERT and not > SET_UPDATE_VALID; the existence of the comment makes me feel like there's some > subtle difference between the two. Is there? The intended difference was that INSERT doesn't have an old entry to compare to. I decided to just share the comment between all of them, and also add one in the DESTRUCT case in RecordErase. https://codereview.chromium.org/1908543002/diff/220001/net/dns/host_cache.cc#... net/dns/host_cache.cc:364: // Nothing to log here. On 2016/05/04 18:15:25, Randy Smith - Not in Fridays wrote: > Same confusion as to why this comment applies here and not to the two below. Acknowledged. https://codereview.chromium.org/1908543002/diff/220001/net/dns/host_cache.h File net/dns/host_cache.h (left): https://codereview.chromium.org/1908543002/diff/220001/net/dns/host_cache.h#o... net/dns/host_cache.h:77: EvictionHandler> EntryMap; On 2016/05/04 18:15:25, Randy Smith - Not in Fridays wrote: > I don't have a sense as to the usage of ExpiringCache in the system as a whole; > does this removal imply that we should queue getting rid of it in other places > because it's more heavyweight than needed if the host cache isn't using it? The host cache isn't using it because the expiration/eviction semantics didn't make sense anymore. It still makes sense for the other two users, but whether the *abstraction* is worth the hassle is another question. I think we should leave it around for now. https://codereview.chromium.org/1908543002/diff/220001/net/dns/host_cache_uni... File net/dns/host_cache_unittest.cc (right): https://codereview.chromium.org/1908543002/diff/220001/net/dns/host_cache_uni... net/dns/host_cache_unittest.cc:299: TEST(HostCacheTest, Evict) { On 2016/05/04 18:15:25, Randy Smith - Not in Fridays wrote: > Any chance you'd be willing to add a description of what each test does before > the function? Often understanding the tests is the hardest part of making > changes in an unfamiliar section of the code base. The description doesn't have > to be very long--just a sentence to give the reader an understanding of what > functionality the test probes. Oh! Sure.
I think once we resolve these issues, I'm good. https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h File net/dns/host_cache.h (right): https://codereview.chromium.org/1908543002/diff/140001/net/dns/host_cache.h#n... net/dns/host_cache.h:137: std::unique_ptr<base::Value> GetEntriesAsValue() const; On 2016/05/04 18:15:25, Randy Smith - Not in Fridays wrote: > On 2016/04/29 17:33:52, Julia Tuttle wrote: > > On 2016/04/28 at 16:24:16, Randy Smith - Not in Fridays wrote: > > > On 2016/04/28 01:44:24, Ryan Sleevi wrote: > > > > Document? > > > > > > > > This feels a little weird because it's now exposing something explicitly > for > > > > NetLog as an API contract. Lessons have been this can backfire when people > > > > 'depend' on it. So documentation here about the "here be dragons" may help > > > > > > I agree. I'm willing to accept this interface, but the old way (in which > the > > net_log pickling routine used the public API to pull out information and put > it > > in Value/json form) strikes me as better if there's no major down sides to it. > > > > Would appending "ForNetLogOnly" do it? > > > > I wanted to avoid having to re-export a bunch of entry metadata. I guess I can > > make EntryInternal public (and rename it) and just expose the map itself if > you > > prefer that. > > So there's a balancing act here. I'm ok with the interface as given if there's > real value in logging in the netlog stuff that's not available through the > public interface (which I take as being the Entry structure), and I'm happy to > take your word that there's value from a debugging perspective. Having said > that, looking at what's returned by this function looks like it's all from > Entry--can you clarify what private information you're wanting to share with the > netlog that public consumers can't access through Entry? > > (I'll also note that I personally think a friend declaration is another fine way > to solve this problem, since what we're trying to say is that we're giving > privileges to the net log that other consumers don't have. But I'd run that by > Matt before doing it :-}.) Ping? (I asked what the private information you're trying to share with NetLog is.) https://codereview.chromium.org/1908543002/diff/260001/net/dns/host_cache.cc File net/dns/host_cache.cc (right): https://codereview.chromium.org/1908543002/diff/260001/net/dns/host_cache.cc#... net/dns/host_cache.cc:351: CACHE_HISTOGRAM_COUNT("LookupStale.Hits", entry->stale_hits); Is this going to be a useful statistic? You'll log a "1" on the first hit, a "2", on the second hit, a "3" on the third hit ... https://codereview.chromium.org/1908543002/diff/260001/net/dns/host_cache.h File net/dns/host_cache.h (right): https://codereview.chromium.org/1908543002/diff/260001/net/dns/host_cache.h#n... net/dns/host_cache.h:58: base::TimeDelta ttl; How would you feel about turning Entry into a class and just exposing these through accessors? I think the style guide encourages that: https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes https://codereview.chromium.org/1908543002/diff/260001/net/dns/host_cache.h#n... net/dns/host_cache.h:80: EntryStaleness* out) const; I believe the style guide says that within each section, methods should be before data members. https://codereview.chromium.org/1908543002/diff/260001/net/dns/host_cache.h#n... net/dns/host_cache.h:149: typedef std::map<Key, Entry> EntryMap; I believe that "using EntryMap = std::map<Key, Entry>;" is the new, preferred hotness; see http://chromium-cpp.appspot.com/.
The private data we're sharing with netlog is the actual expiration time. Callers can get the *ttl*, but without knowing when the entry was *added*, they can't know when it *expires*. https://codereview.chromium.org/1908543002/diff/260001/net/dns/host_cache.cc File net/dns/host_cache.cc (right): https://codereview.chromium.org/1908543002/diff/260001/net/dns/host_cache.cc#... net/dns/host_cache.cc:351: CACHE_HISTOGRAM_COUNT("LookupStale.Hits", entry->stale_hits); On 2016/05/05 20:41:51, Randy Smith - Not in Fridays wrote: > Is this going to be a useful statistic? You'll log a "1" on the first hit, a > "2", on the second hit, a "3" on the third hit ... Yeah, it'll be useful because we can extract the actual "how many hits are stale entries getting?" data from it. Without it we'd have to sum the on-update, on-cache-clear, and (non-existent) on-cache-destruct variants of the same histogram. https://codereview.chromium.org/1908543002/diff/260001/net/dns/host_cache.h File net/dns/host_cache.h (right): https://codereview.chromium.org/1908543002/diff/260001/net/dns/host_cache.h#n... net/dns/host_cache.h:58: base::TimeDelta ttl; On 2016/05/05 20:41:51, Randy Smith - Not in Fridays wrote: > How would you feel about turning Entry into a class and just exposing these > through accessors? I think the style guide encourages that: > https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes Done. https://codereview.chromium.org/1908543002/diff/260001/net/dns/host_cache.h#n... net/dns/host_cache.h:80: EntryStaleness* out) const; On 2016/05/05 20:41:51, Randy Smith - Not in Fridays wrote: > I believe the style guide says that within each section, methods should be > before data members. Done. https://codereview.chromium.org/1908543002/diff/260001/net/dns/host_cache.h#n... net/dns/host_cache.h:149: typedef std::map<Key, Entry> EntryMap; On 2016/05/05 20:41:51, Randy Smith - Not in Fridays wrote: > I believe that "using EntryMap = std::map<Key, Entry>;" is the new, preferred > hotness; see http://chromium-cpp.appspot.com/. Done.
PTAL, rdsmith.
LGTM modulo issue below. (I could probably be persuaded to stamp without it, but I thought we had agreed on the change.) https://codereview.chromium.org/1908543002/diff/320001/net/dns/host_cache.h File net/dns/host_cache.h (right): https://codereview.chromium.org/1908543002/diff/320001/net/dns/host_cache.h#n... net/dns/host_cache.h:86: base::TimeDelta ttl_; I thought you were going to turn these into accessors?
On 2016/05/11 19:37:54, Randy Smith - Not in Fridays wrote: > LGTM modulo issue below. (I could probably be persuaded to stamp without it, > but I thought we had agreed on the change.) > > https://codereview.chromium.org/1908543002/diff/320001/net/dns/host_cache.h > File net/dns/host_cache.h (right): > > https://codereview.chromium.org/1908543002/diff/320001/net/dns/host_cache.h#n... > net/dns/host_cache.h:86: base::TimeDelta ttl_; > I thought you were going to turn these into accessors? You're right! I did, but then I forgot to make the variables private. Fixed.
LGTM.
still lgtm
The CQ bit was checked by juliatuttle@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1908543002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1908543002/340001
Message was sent while issue was closed.
Committed patchset #17 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== DNS: Retain stale entries in HostCache and return when requested In preparation for future connection optimizations, retain stale entries (those with a response past the TTL or received on a different network) in the cache, and expose a way to look up and characterize the stale results. To do this, HostCache moves away from using ExpiringCache to a self- managed map of keys to entries, with extra metadata. Also, add a bunch of histograms measuring how stale updated entries are. BUG=605138 ========== to ========== DNS: Retain stale entries in HostCache and return when requested In preparation for future connection optimizations, retain stale entries (those with a response past the TTL or received on a different network) in the cache, and expose a way to look up and characterize the stale results. To do this, HostCache moves away from using ExpiringCache to a self- managed map of keys to entries, with extra metadata. Also, add a bunch of histograms measuring how stale updated entries are. BUG=605138 Committed: https://crrev.com/317860ed4f3e3788fd3c4637ee5d91afd31e39b1 Cr-Commit-Position: refs/heads/master@{#393246} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/317860ed4f3e3788fd3c4637ee5d91afd31e39b1 Cr-Commit-Position: refs/heads/master@{#393246} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
