Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10909136:
Http Cache: Add code for simulating an infinite HTTP cache. (Closed)
|
Created:
8 years, 3 months ago by rvargas (doing something else) Modified:
8 years, 3 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org, gavinp+disk_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionHttp Cache: Add code for simulating an infinite HTTP cache.
This is not completely wired yet.
BUg=147383
TEST=net_unittests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=156846
Patch Set 1 #
Total comments: 63
Patch Set 2 : rebase after update #Patch Set 3 : #
Total comments: 4
Patch Set 4 : #Patch Set 5 : Add HttpCache getter #
Total comments: 1
Patch Set 6 : #
Messages
Total messages: 17 (0 generated)
http://codereview.chromium.org/10909136/diff/1/net/http/http_cache_transactio... File net/http/http_cache_transaction.cc (right): http://codereview.chromium.org/10909136/diff/1/net/http/http_cache_transactio... net/http/http_cache_transaction.cc:255: if ((effective_load_flags_ & LOAD_BYPASS_CACHE) || I'm not really sure how to evaluate this section of the code. Are you sure you want to send this to me? :) http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache_unitte... File net/http/infinite_cache_unittest.cc (right): http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache_unitte... net/http/infinite_cache_unittest.cc:11: #include "net/http/infinite_cache.h" Move this to the top
Yeah, despite my interest in this, I think deferring the review to Gavin makes more sense. On Mon, Sep 10, 2012 at 1:47 PM, <willchan@chromium.org> wrote: > > http://codereview.chromium.**org/10909136/diff/1/net/http/** > http_cache_transaction.cc<http://codereview.chromium.org/10909136/diff/1/net/http/http_cache_transaction.cc> > File net/http/http_cache_**transaction.cc (right): > > http://codereview.chromium.**org/10909136/diff/1/net/http/** > http_cache_transaction.cc#**newcode255<http://codereview.chromium.org/10909136/diff/1/net/http/http_cache_transaction.cc#newcode255> > net/http/http_cache_**transaction.cc:255: if ((effective_load_flags_ & > LOAD_BYPASS_CACHE) || > I'm not really sure how to evaluate this section of the code. Are you > sure you want to send this to me? :) > > http://codereview.chromium.**org/10909136/diff/1/net/http/** > infinite_cache_unittest.cc<http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache_unittest.cc> > File net/http/infinite_cache_**unittest.cc (right): > > http://codereview.chromium.**org/10909136/diff/1/net/http/** > infinite_cache_unittest.cc#**newcode11<http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache_unittest.cc#newcode11> > net/http/infinite_cache_**unittest.cc:11: #include > "net/http/infinite_cache.h" > Move this to the top > > http://codereview.chromium.**org/10909136/<http://codereview.chromium.org/109... >
On 2012/09/10 20:48:20, willchan wrote: > Yeah, despite my interest in this, I think deferring the review to Gavin > makes more sense. > > > On Mon, Sep 10, 2012 at 1:47 PM, <mailto:willchan@chromium.org> wrote: > > > > > http://codereview.chromium.**org/10909136/diff/1/net/http/** > > > http_cache_transaction.cc<http://codereview.chromium.org/10909136/diff/1/net/http/http_cache_transaction.cc> > > File net/http/http_cache_**transaction.cc (right): > > > > http://codereview.chromium.**org/10909136/diff/1/net/http/** > > > http_cache_transaction.cc#**newcode255<http://codereview.chromium.org/10909136/diff/1/net/http/http_cache_transaction.cc#newcode255> > > net/http/http_cache_**transaction.cc:255: if ((effective_load_flags_ & > > LOAD_BYPASS_CACHE) || > > I'm not really sure how to evaluate this section of the code. Are you > > sure you want to send this to me? :) > > > > http://codereview.chromium.**org/10909136/diff/1/net/http/** > > > infinite_cache_unittest.cc<http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache_unittest.cc> > > File net/http/infinite_cache_**unittest.cc (right): > > > > http://codereview.chromium.**org/10909136/diff/1/net/http/** > > > infinite_cache_unittest.cc#**newcode11<http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache_unittest.cc#newcode11> > > net/http/infinite_cache_**unittest.cc:11: #include > > "net/http/infinite_cache.h" > > Move this to the top > > > > > http://codereview.chromium.**org/10909136/%3Chttp://codereview.chromium.org/1...> > >
My first set of comments. Overall, looks pretty good! I guess there are still a few things that need to be hooked up (such as deletion). http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc File net/http/infinite_cache.cc (right): http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:530: What if the object was in the map_, but now has NO_STORE set. It seems like you still update the map to the new entry. Whereas, if no prior entry existed, you don't insert it into the map. http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:636: How about a checksum on the header + file contents to detect corruption? http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:787: header_->num_hits++; If reused was false, this wouldn't have been a hit, correct? Also, I guess when it's a bad hit, you increment num_hits twice? I find that counter-intuitive and would only increment num_hits once in that case? http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:829: UMA_HISTOGRAM_COUNTS_10000("InfiniteCache.HitRatioByUseTime", use_hours); These two histograms seem to be identical to the two above, except for the name? I guess you intended to emit hit ratios here? http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:842: UMA_HISTOGRAM_ENUMERATION("InfiniteCache.HitRatioBySize", Seems like you are just duplicating the previous histograms again, no actual hit ratios?
Also, Timo's comment about NO_STORE touches on an assumption that this code makes. There's at most one entry stored per key (url). That's certainly a limitation of our current cache, but the HTTP RFC enforces no such rule. If two distinct entries for the same URL have different vary headers, for instance, wouldn't an infinite cache want to keep both? And if we get a no_store response in reply to a conditional-get, why does invalidate the existing entry? I think the infinite cache would store as many entries for a single URL as makes sense, although perhaps that's not practical to code up for our deadline. http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc File net/http/infinite_cache.cc (right): http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:46: REVALIDABLE = 1 << 5, REVALIDATEABLE? http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:49: }; Nit: enums without prefixes in a default/global namespace scare me a bit: they can override other meanings of the same value, making code pretty confusing to read. http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:55: }; Nit: Would a typedef suit us better than a struct? http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:132: Time GetBaseline() { Why not drop this function, and instead use Time::UnixEpoch() ? http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:140: // 1 second resolution, +- 17 years from the baseline. I'm confused: won't this code be fine +- 68 years? What am I missing? http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:143: if (seconds > kint32max) Do we have a coding standard on kint32max vs std::numeric_limits<int32>::max() etc...? http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:158: // Avoid overflow when adding to current time. Won't TimeToInt function deal gracefully with overflow? If that behaviour is undesirable, why? And why is an interval of 10 years - (1 to 3) days a better overflow value? http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:165: uint32 cacheability = 0; So a cacheability of 0 means that an item can be cached? http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:177: if (headers->GetMaxAgeValue(&max_age) && max_age.InSeconds() <= 0) This if is valid, since there's a sequence point in && and || evaluation, but it took me a second to convince myself I'm right. http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:208: return adler32(hash, reinterpret_cast<const Bytef*>(pickle.data()), I think MurmurHash is faster than adler32, and it's already in our codebase. http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:237: struct InfiniteCacheTransaction::ResourceData { Nit: Isn't this rather than information about a resource, information about a transaction? http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:266: resource_data_.reset(new ResourceData); Since ResourceData is non-POD, this is fine, but I wonder if we have a standard here and if the () would be good to add? http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:284: TimeToInt(response->response_time) == details.expiration) { Not <= ? http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:337: cache_->ProcessResource(resource_data_.Pass()); Could we cache_.reset() here, and get rid of done_ ? http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:351: class InfiniteCache::Worker : public base::RefCountedThreadSafe<Worker> { Why isn't this in the anon namespace? http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:381: typedef BASE_HASH_NAMESPACE::hash_map< Does our need to occasionally iterate over a range of this structure justify using std::map, which can do that iteration in time O(log(n) + range_n) vs O(n) ? http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:471: for (KeyMap::iterator i = map_.begin(); i != map_.end();) { Deletion mid iteration is kind of scary. It looks like you got it right here, but why not store the list of to-delete items in a std::stack or something, and delete them all after? http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:487: UMA_HISTOGRAM_ENUMERATION("InfiniteCache.DeleteAll", 1, 2); UMA_HISTOGRAM_BOOLEAN? http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.h File net/http/infinite_cache.h (right): http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.h#newc... net/http/infinite_cache.h:18: #include "net/base/completion_callback.h" Can't we drop this include? http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.h#newc... net/http/infinite_cache.h:38: class NET_EXPORT_PRIVATE InfiniteCacheTransaction { A question: Is the NET_EXPORT_PRIVATE required when the unit test is also in net/ ? Why? Is it because unit_tests use the same libnet.a/.so/.dll that is built for chrome and content_browser etc...? http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.h#newc... net/http/infinite_cache.h:67: bool doom_method_; Nit: I don't think this is well named. It stores if the transaction will doom the entry or not, presuming that our infinite cache doesn't store multiple entries for the same URL, right? Maybe something like method_will_doom_entry_ ? http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.h#newc... net/http/infinite_cache.h:97: // Flush the data to disk, preventing flushing when the destructor runs. Document what happens if we add more data after calling this, then call the destructor.
On 2012/09/10 20:48:20, willchan wrote: > Yeah, despite my interest in this, I think deferring the review to Gavin > makes more sense. Sure, I was not expecting you to be the only reviewer, but you said you wanted to look at the code. Thanks.
First round of comments. Code to follow. http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc File net/http/infinite_cache.cc (right): http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:49: }; On 2012/09/13 04:45:43, gavinp wrote: > Nit: enums without prefixes in a default/global namespace scare me a bit: they > can override other meanings of the same value, making code pretty confusing to > read. This is an anonymous namespace, and nothing should be visible in this file without a prefix/namespace/class_name, right? http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:132: Time GetBaseline() { On 2012/09/13 04:45:43, gavinp wrote: > Why not drop this function, and instead use Time::UnixEpoch() ? see below http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:140: // 1 second resolution, +- 17 years from the baseline. On 2012/09/13 04:45:43, gavinp wrote: > I'm confused: won't this code be fine +- 68 years? What am I missing? Looks like I missed a four (or apply 2x in the wrong direction). With this being 68, there's no need to start in 2010 (unless we want to improve the resolution). http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:143: if (seconds > kint32max) On 2012/09/13 04:45:43, gavinp wrote: > Do we have a coding standard on kint32max vs std::numeric_limits<int32>::max() > etc...? My personal position is that as long as kint32max is available i'd rather use it and not std::numeric_limits<int32>::max() http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:158: // Avoid overflow when adding to current time. On 2012/09/13 04:45:43, gavinp wrote: > Won't TimeToInt function deal gracefully with overflow? If that behaviour is > undesirable, why? And why is an interval of 10 years - (1 to 3) days a better > overflow value? freshness can be kint64max, which overflows to a negative time before reaching TimeToInt() http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:165: uint32 cacheability = 0; On 2012/09/13 04:45:43, gavinp wrote: > So a cacheability of 0 means that an item can be cached? It means that there are no cache related headers. http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:237: struct InfiniteCacheTransaction::ResourceData { On 2012/09/13 04:45:43, gavinp wrote: > Nit: Isn't this rather than information about a resource, information about a > transaction? No in that details (and actually also key) refer to the particular resource that a transaction is trying to fetch. Or at least that's the bulk of the info (with a small overloading of a couple of flags). http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:266: resource_data_.reset(new ResourceData); On 2012/09/13 04:45:43, gavinp wrote: > Since ResourceData is non-POD, this is fine, but I wonder if we have a standard > here and if the () would be good to add? I think the standard leans towards avoiding decoration unless it is needed. http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:284: TimeToInt(response->response_time) == details.expiration) { On 2012/09/13 04:45:43, gavinp wrote: > Not <= ? expiration cannot be lower than response_time. On the other hand, response_time < expiration is not expired. http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:337: cache_->ProcessResource(resource_data_.Pass()); On 2012/09/13 04:45:43, gavinp wrote: > Could we cache_.reset() here, and get rid of done_ ? ok http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:351: class InfiniteCache::Worker : public base::RefCountedThreadSafe<Worker> { On 2012/09/13 04:45:43, gavinp wrote: > Why isn't this in the anon namespace? Because the cache has a pointer to it. http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:381: typedef BASE_HASH_NAMESPACE::hash_map< On 2012/09/13 04:45:43, gavinp wrote: > Does our need to occasionally iterate over a range of this structure justify > using std::map, which can do that iteration in time O(log(n) + range_n) vs O(n) > ? I think we should optimize for the common case. Iterations are not it (unless we are serializing, in which case we have to go through the whole set). http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:471: for (KeyMap::iterator i = map_.begin(); i != map_.end();) { On 2012/09/13 04:45:43, gavinp wrote: > Deletion mid iteration is kind of scary. It looks like you got it right here, > but why not store the list of to-delete items in a std::stack or something, and > delete them all after? We may be deleting basically the whole list... a lot of objects. Do we want to take the memory hit? http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:530: On 2012/09/11 21:02:00, tburkard wrote: > What if the object was in the map_, but now has NO_STORE set. It seems like you > still update the map to the new entry. Whereas, if no prior entry existed, you > don't insert it into the map. Right. It looks like I have not made up my mind about what to do: I was thinking about tracking NO_STORE and report that stat too... if we never store NO_STORE requests, there's no point in forwarding them to the worker thread. Opinions? http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:636: On 2012/09/11 21:02:00, tburkard wrote: > How about a checksum on the header + file contents to detect corruption? There is a checksum on the record set. It is just slightly simpler to not keep a single checksum. http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:787: header_->num_hits++; On 2012/09/11 21:02:00, tburkard wrote: > If reused was false, this wouldn't have been a hit, correct? > > Also, I guess when it's a bad hit, you increment num_hits twice? I find that > counter-intuitive and would only increment num_hits once in that case? yeah, this is not right. http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:829: UMA_HISTOGRAM_COUNTS_10000("InfiniteCache.HitRatioByUseTime", use_hours); On 2012/09/11 21:02:00, tburkard wrote: > These two histograms seem to be identical to the two above, except for the name? > I guess you intended to emit hit ratios here? No, this is Gavin's trick of dividing two histograms on the dashboard to get a rolling hit ratio (or whatever modulates report_second_stat). I omitted the comment because I expect the pattern to be common... I'll add something. http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.h File net/http/infinite_cache.h (right): http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.h#newc... net/http/infinite_cache.h:38: class NET_EXPORT_PRIVATE InfiniteCacheTransaction { On 2012/09/13 04:45:43, gavinp wrote: > A question: Is the NET_EXPORT_PRIVATE required when the unit test is also in > net/ ? Why? Is it because unit_tests use the same libnet.a/.so/.dll that is > built for chrome and content_browser etc...? net_unittests doesn't link in net on the component build, so if it wants to access this class it has to be exported. http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.h#newc... net/http/infinite_cache.h:67: bool doom_method_; On 2012/09/13 04:45:43, gavinp wrote: > Nit: I don't think this is well named. It stores if the transaction will doom > the entry or not, presuming that our infinite cache doesn't store multiple > entries for the same URL, right? Maybe something like method_will_doom_entry_ ? I don't get the part about multiple entries, but yes, it says that the method used for the request should result in the entry being doomed. must_doom_entry_ ?
http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc File net/http/infinite_cache.cc (right): http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:208: return adler32(hash, reinterpret_cast<const Bytef*>(pickle.data()), On 2012/09/13 04:45:43, gavinp wrote: > I think MurmurHash is faster than adler32, and it's already in our codebase. That's not currently a dependency of net (or any module below net, afaik), and I don't want to add another dependency, just for this.
New version up. http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc File net/http/infinite_cache.cc (right): http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:55: }; On 2012/09/13 04:45:43, gavinp wrote: > Nit: Would a typedef suit us better than a struct? I just tried to do that and run into issues reading and writing to/from disk. (array initialization errors). It looks like I would need explicit memcpies into the key, so for the time being I'm leaving it this way. http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:530: We'd still need forwarding just to delete the old entry... and that's what I'm doing now. http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.h File net/http/infinite_cache.h (right): http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.h#newc... net/http/infinite_cache.h:18: #include "net/base/completion_callback.h" On 2012/09/13 04:45:43, gavinp wrote: > Can't we drop this include? I cannot forward declare it, because it is a typedef.
http://codereview.chromium.org/10909136/diff/14001/net/http/infinite_cache.cc File net/http/infinite_cache.cc (right): http://codereview.chromium.org/10909136/diff/14001/net/http/infinite_cache.cc... net/http/infinite_cache.cc:513: RecordHit(i->second, &data->details); Shouldn't we record a hit even if the data has changed? In any cache, this will be a problem (that the cache might incorrectly reuse content, potentially due to incorrect caching directives). An actual cache of the respective size would make the same mistake, so I think we should count it here, too.
http://codereview.chromium.org/10909136/diff/14001/net/http/infinite_cache.cc File net/http/infinite_cache.cc (right): http://codereview.chromium.org/10909136/diff/14001/net/http/infinite_cache.cc... net/http/infinite_cache.cc:513: RecordHit(i->second, &data->details); On 2012/09/14 01:01:49, tburkard wrote: > Shouldn't we record a hit even if the data has changed? > In any cache, this will be a problem (that the cache might incorrectly reuse > content, potentially due to incorrect caching directives). An actual cache of > the respective size would make the same mistake, so I think we should count it > here, too. After your comment, I thought is was better to have the numbers apart, so that bad_hits + hits = real hits. I'm fine either way, but I just saw that the bad_hits histogram is not perXX, so I'll just go back to the previous condition here (just |reused|)
http://codereview.chromium.org/10909136/diff/14001/net/http/infinite_cache.cc File net/http/infinite_cache.cc (right): http://codereview.chromium.org/10909136/diff/14001/net/http/infinite_cache.cc... net/http/infinite_cache.cc:513: RecordHit(i->second, &data->details); Sorry, I just meant double counting didn't make a lot of sense (once for used and once for update -- you originally incremented the hit count in both places). I suggest counted_hits - bad_hits = real_hits, with counted hits including bad hits, since that's what a cache would effectively do. real hits is a less important theoretical value.
http://codereview.chromium.org/10909136/diff/14001/net/http/infinite_cache.cc File net/http/infinite_cache.cc (right): http://codereview.chromium.org/10909136/diff/14001/net/http/infinite_cache.cc... net/http/infinite_cache.cc:513: RecordHit(i->second, &data->details); Agreed. done.
lgtm. None of my nits are major enough to block this. I will take a second pass through saturday am (friday pm for you) to be sure. http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc File net/http/infinite_cache.cc (right): http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:49: }; On 2012/09/13 22:44:07, rvargas wrote: > On 2012/09/13 04:45:43, gavinp wrote: > > Nit: enums without prefixes in a default/global namespace scare me a bit: they > > can override other meanings of the same value, making code pretty confusing to > > read. > > This is an anonymous namespace, and nothing should be visible in this file > without a prefix/namespace/class_name, right? If anyone has an enum in namespace net with an entry named EXPIRED, then you're aliasing that here. Probably fine, unless someone expects to be using that... But yes, probably fine. http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:55: }; On 2012/09/13 23:50:28, rvargas wrote: > On 2012/09/13 04:45:43, gavinp wrote: > > Nit: Would a typedef suit us better than a struct? > > I just tried to do that and run into issues reading and writing to/from disk. > (array initialization errors). It looks like I would need explicit memcpies into > the key, so for the time being I'm leaving it this way. sgtm. http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:158: // Avoid overflow when adding to current time. On 2012/09/13 22:44:07, rvargas wrote: > On 2012/09/13 04:45:43, gavinp wrote: > > Won't TimeToInt function deal gracefully with overflow? If that behaviour is > > undesirable, why? And why is an interval of 10 years - (1 to 3) days a better > > overflow value? > > freshness can be kint64max, which overflows to a negative time before reaching > TimeToInt() This is getting into crazy Nit territory, since 10 years (minus between 1 and 3 days) is probably fine for this short lived experiment. But I'll share my nit for posterity: OK... Wow. I just saw how http_response_headers.cc uses TimeDelta::FromMicroseconds(kint64max). Interesting. That probably should switch to using TimeDelta::Max(), which just happens to be the same thing... So max ints can be scary, but 10 years is a weird magic number. How about fixing it like so: if (TimeDelta::Max() - response->response_time > freshness) return TimeToInt(TimeDelta::Max()); return TimeToInt(response->response_time + freshness); http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:165: uint32 cacheability = 0; On 2012/09/13 22:44:07, rvargas wrote: > On 2012/09/13 04:45:43, gavinp wrote: > > So a cacheability of 0 means that an item can be cached? > > It means that there are no cache related headers. Right. And each of those headers limit cacheability... So 0 is the most cacheable of all. http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:237: struct InfiniteCacheTransaction::ResourceData { On 2012/09/13 22:44:07, rvargas wrote: > On 2012/09/13 04:45:43, gavinp wrote: > > Nit: Isn't this rather than information about a resource, information about a > > transaction? > > No in that details (and actually also key) refer to the particular resource that > a transaction is trying to fetch. Or at least that's the bulk of the info (with > a small overloading of a couple of flags). It's constructed once per transaction, and sent to the other thread from ...Transaction::Finish(), and in Worker::Process() we update the stats from the information we learned in the transaction. I saw it as a record of a transaction, since ResourceData only gains information about prior resources after it's already been filled in in ...Transaction methods, and sent down.. http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:381: typedef BASE_HASH_NAMESPACE::hash_map< On 2012/09/13 22:44:07, rvargas wrote: > On 2012/09/13 04:45:43, gavinp wrote: > > Does our need to occasionally iterate over a range of this structure justify > > using std::map, which can do that iteration in time O(log(n) + range_n) vs > O(n) > > ? > > I think we should optimize for the common case. Iterations are not it (unless we > are serializing, in which case we have to go through the whole set). sgtm. http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:471: for (KeyMap::iterator i = map_.begin(); i != map_.end();) { On 2012/09/13 22:44:07, rvargas wrote: > On 2012/09/13 04:45:43, gavinp wrote: > > Deletion mid iteration is kind of scary. It looks like you got it right here, > > but why not store the list of to-delete items in a std::stack or something, > and > > delete them all after? > > We may be deleting basically the whole list... a lot of objects. Do we want to > take the memory hit? 40 megabytes/sizeof(Details) * sizeof(void*) ~= 5 megabytes, allocated and then deallocated in less than 1ms? http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.h File net/http/infinite_cache.h (right): http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.h#newc... net/http/infinite_cache.h:67: bool doom_method_; On 2012/09/13 22:44:07, rvargas wrote: > On 2012/09/13 04:45:43, gavinp wrote: > > Nit: I don't think this is well named. It stores if the transaction will doom > > the entry or not, presuming that our infinite cache doesn't store multiple > > entries for the same URL, right? Maybe something like method_will_doom_entry_ > ? > > I don't get the part about multiple entries, but yes, it says that the method > used for the request should result in the entry being doomed. It's just that GETs replace anything with the same URL, even if the previously stored GET had different Vary headers. The hit rate of the cache would be higher if we could keep each version, since a future request might use the same Vary headers as earlier. I don't think we need to do this for our first stats gathering. > > must_doom_entry_ ? must_doom_entry_! http://codereview.chromium.org/10909136/diff/14004/net/http/infinite_cache.cc File net/http/infinite_cache.cc (right): http://codereview.chromium.org/10909136/diff/14004/net/http/infinite_cache.cc... net/http/infinite_cache.cc:46: REVALIDATEABLE = 1 << 5, Boy, that's an ugly word.
lgtm
thanks http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc File net/http/infinite_cache.cc (right): http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:158: // Avoid overflow when adding to current time. On 2012/09/14 03:27:28, gavinp wrote: > On 2012/09/13 22:44:07, rvargas wrote: > > On 2012/09/13 04:45:43, gavinp wrote: > > > Won't TimeToInt function deal gracefully with overflow? If that behaviour is > > > undesirable, why? And why is an interval of 10 years - (1 to 3) days a > better > > > overflow value? > > > > freshness can be kint64max, which overflows to a negative time before reaching > > TimeToInt() > > This is getting into crazy Nit territory, since 10 years (minus between 1 and 3 > days) is probably fine for this short lived experiment. But I'll share my nit > for posterity: > > OK... Wow. I just saw how http_response_headers.cc uses > TimeDelta::FromMicroseconds(kint64max). Interesting. That probably should switch > to using TimeDelta::Max(), which just happens to be the same thing... > > So max ints can be scary, but 10 years is a weird magic number. How about fixing > it like so: > > if (TimeDelta::Max() - response->response_time > freshness) > return TimeToInt(TimeDelta::Max()); > return TimeToInt(response->response_time + freshness); That seems like a more convoluted way to deal with it... what is exaclty a TimeDelta - Time ?. I think that the current version (while using a totally arbitrary time limit) is simpler and more direct. http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:237: struct InfiniteCacheTransaction::ResourceData { On 2012/09/14 03:27:28, gavinp wrote: > On 2012/09/13 22:44:07, rvargas wrote: > > On 2012/09/13 04:45:43, gavinp wrote: > > > Nit: Isn't this rather than information about a resource, information about > a > > > transaction? > > > > No in that details (and actually also key) refer to the particular resource > that > > a transaction is trying to fetch. Or at least that's the bulk of the info > (with > > a small overloading of a couple of flags). > > It's constructed once per transaction, and sent to the other thread from > ...Transaction::Finish(), and in Worker::Process() we update the stats from the > information we learned in the transaction. I saw it as a record of a > transaction, since ResourceData only gains information about prior resources > after it's already been filled in in ...Transaction methods, and sent down.. Sure... the way to get information about resources is through transactions. Still, this is meant to represent the information stored by the "infinite cache", AKA, stored resources. http://codereview.chromium.org/10909136/diff/1/net/http/infinite_cache.cc#new... net/http/infinite_cache.cc:471: for (KeyMap::iterator i = map_.begin(); i != map_.end();) { On 2012/09/14 03:27:28, gavinp wrote: > On 2012/09/13 22:44:07, rvargas wrote: > > On 2012/09/13 04:45:43, gavinp wrote: > > > Deletion mid iteration is kind of scary. It looks like you got it right > here, > > > but why not store the list of to-delete items in a std::stack or something, > > and > > > delete them all after? > > > > We may be deleting basically the whole list... a lot of objects. Do we want to > > take the memory hit? > > 40 megabytes/sizeof(Details) * sizeof(void*) ~= 5 megabytes, allocated and then > deallocated in less than 1ms? So you mean a set of iterators... That's scary. I still prefer the current version better. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
