Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10808047:
Add HttpCache histograms focussed on blocking and transaction type. (Closed)
|
Created:
8 years, 5 months ago by gavinp Modified:
8 years, 4 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. |
DescriptionAdd HttpCache histograms focussed on blocking and transaction type.
Most of our existing disk cache histograms focus on the speed of operations on the cache itself. This CL adds a bunch that break down http transactions by type, and track both time spent reading (either from disk cache or network), and time spent waiting for the disk cache to allow us to send the network request.
Note that these patches are based on http://codereview.chromium.org/10797042/ , which must land first.
R=rvargas@chromium.org, tburkhard@chromium.org
BUG=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150902
Patch Set 1 #Patch Set 2 : rebase onto http://codereview.chromium.org/10797042/ #Patch Set 3 : catch more special cases #
Total comments: 49
Patch Set 4 : remediate to rvargas review #Patch Set 5 : ... bonus braces ... #
Total comments: 9
Patch Set 6 : remediate to rvargas review #Patch Set 7 : add GetCacheType() #Patch Set 8 : lose unreferenced variable #Patch Set 9 : rename parameter #
Total comments: 2
Patch Set 10 : remediate to review #
Total comments: 2
Patch Set 11 : merge to trunk #Patch Set 12 : fix merge... #Patch Set 13 : move UpdateTransactionPattern #Patch Set 14 : style fixes #Patch Set 15 : fix for short time spans #Patch Set 16 : ... how about a fix that compiles. #
Messages
Total messages: 29 (0 generated)
WDYT?
http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... File net/http/http_cache_transaction.cc (right): http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... net/http/http_cache_transaction.cc:2181: DCHECK_NE(DISPOSITION_UNDEFINED, disposition_); Should these DCHECKs be changed into early returns? Or should we have both?
Timo's email is tburkard. tburkhard is some other guy.
I was just discussing this with Timo, and he suggested this CL should serialize on http://codereview.chromium.org/10736066/ . I'll investigate rebasing it on that.
I may be missing something with the actual logic (setting of each state), because I didn't review that in detail. https://chromiumcodereview.appspot.com/10808047/diff/2004/net/http/http_cache... File net/http/http_cache_transaction.cc (right): https://chromiumcodereview.appspot.com/10808047/diff/2004/net/http/http_cache... net/http/http_cache_transaction.cc:44: namespace { Move this outside of net https://chromiumcodereview.appspot.com/10808047/diff/2004/net/http/http_cache... net/http/http_cache_transaction.cc:687: // Don't record a load that isn't really testing the cache. This goes through the cache. https://chromiumcodereview.appspot.com/10808047/diff/2004/net/http/http_cache... net/http/http_cache_transaction.cc:737: started_reading_since_ = TimeTicks::Now(); The name is weird for what it is measuring. https://chromiumcodereview.appspot.com/10808047/diff/2004/net/http/http_cache... net/http/http_cache_transaction.cc:821: MaybeUpdateDisposition(DISPOSITION_ENTRY_VALIDATED_304); This is more than just 304. Why not drop the 304 part? https://chromiumcodereview.appspot.com/10808047/diff/2004/net/http/http_cache... net/http/http_cache_transaction.cc:825: MaybeUpdateDisposition(DISPOSITION_ENTRY_VALIDATED_200); What about names that match the histogram? used, validated, updated. https://chromiumcodereview.appspot.com/10808047/diff/2004/net/http/http_cache... net/http/http_cache_transaction.cc:1268: started_reading_since_ = TimeTicks::Now(); Now I'm confused about what do you want to measure with started_reading_since_... there are 3 different values used. https://chromiumcodereview.appspot.com/10808047/diff/2004/net/http/http_cache... net/http/http_cache_transaction.cc:2064: DVLOG(1) << "Recorded: " << request_->method << request_->url unrelated: we should just delete this log. https://chromiumcodereview.appspot.com/10808047/diff/2004/net/http/http_cache... net/http/http_cache_transaction.cc:2161: void HttpCache::Transaction::MaybeUpdateDisposition( Remove the "maybe" part from the name. https://chromiumcodereview.appspot.com/10808047/diff/2004/net/http/http_cache... net/http/http_cache_transaction.cc:2162: const Disposition disposition) { Drop the const https://chromiumcodereview.appspot.com/10808047/diff/2004/net/http/http_cache... net/http/http_cache_transaction.cc:2165: if (disposition_ == DISPOSITION_DO_NOT_RECORD) is this really worth the extra work?. We could just have a single value for don't track and uninitialized, and avoid a bunch of calls. https://chromiumcodereview.appspot.com/10808047/diff/2004/net/http/http_cache... net/http/http_cache_transaction.cc:2168: disposition == DISPOSITION_DO_NOT_RECORD); cannot reach do_not_record https://chromiumcodereview.appspot.com/10808047/diff/2004/net/http/http_cache... net/http/http_cache_transaction.cc:2172: void HttpCache::Transaction::RecordHistograms(const int resource_size) { Drop the const https://chromiumcodereview.appspot.com/10808047/diff/2004/net/http/http_cache... net/http/http_cache_transaction.cc:2181: DCHECK_NE(DISPOSITION_UNDEFINED, disposition_); On 2012/07/20 17:19:24, gavinp wrote: > Should these DCHECKs be changed into early returns? Or should we have both? DCHECKS seem file to me. https://chromiumcodereview.appspot.com/10808047/diff/2004/net/http/http_cache... net/http/http_cache_transaction.cc:2185: const TimeDelta before_reading_time = I also think we should drop the const on these variables. It's not that useful and add noise/distraction. https://chromiumcodereview.appspot.com/10808047/diff/2004/net/http/http_cache... net/http/http_cache_transaction.cc:2190: UMA_HISTOGRAM_TIMES("HttpCache.TimeBlocked", before_reading_time); This puts together all cache types under the same histogram, and the expected behavior is not the same. On the other hand, these don't really look that useful to me. We'll end up with two wide curves with slightly different parameters without a real way to extract data about them. I'd argue that what we want is to know the percentage of time that a request stays on any given "state". https://chromiumcodereview.appspot.com/10808047/diff/2004/net/http/http_cache... net/http/http_cache_transaction.cc:2193: const bool is_small_resource = resource_size < kSmallResourceMaxBytes; This is not what the caller is passing... I mean, to compare against a value of a single network read you'd have to add the http headers too. https://chromiumcodereview.appspot.com/10808047/diff/2004/net/http/http_cache... net/http/http_cache_transaction.cc:2196: "HttpCache.TimeBlocked.SmallResource", before_reading_time); nit: TimeBlocked? https://chromiumcodereview.appspot.com/10808047/diff/2004/net/http/http_cache... net/http/http_cache_transaction.cc:2200: // The more detailed histograms are only interesting for requests that are not why? https://chromiumcodereview.appspot.com/10808047/diff/2004/net/http/http_cache... net/http/http_cache_transaction.cc:2209: UMA_HISTOGRAM_PERCENTAGE("HttpCache.PercentBlocked", before_reading_percent); nit: Not really blocked https://chromiumcodereview.appspot.com/10808047/diff/2004/net/http/http_cache... net/http/http_cache_transaction.cc:2216: switch (disposition_) { nit: I'm not really happy with the term "disposition". It covers "do not track" plus whether the response came from the cache or not (with revalidation or not). How about a name that conveys a tracking/histogram, type/status? track_status, cached_status, transaction_type (too vague)... something like that. https://chromiumcodereview.appspot.com/10808047/diff/2004/net/http/http_cache... net/http/http_cache_transaction.cc:2221: "HttpCache.PercentBlocked.NotCached", before_reading_percent); Id suggest just dropping the absolute times.
Here's what I hacked up on my flight... I haven't done a full test run (just on a laptop), but I think I improved the readability in the ways you asked. What do you think? http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... File net/http/http_cache_transaction.cc (right): http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... net/http/http_cache_transaction.cc:44: namespace { On 2012/07/21 02:24:21, rvargas wrote: > Move this outside of net Done. You didn't say why: is it because it doesn't reference anything in net? http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... net/http/http_cache_transaction.cc:687: // Don't record a load that isn't really testing the cache. On 2012/07/21 02:24:21, rvargas wrote: > This goes through the cache. Good point. Comment fixed. Srsly fixed. http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... net/http/http_cache_transaction.cc:737: started_reading_since_ = TimeTicks::Now(); On 2012/07/21 02:24:21, rvargas wrote: > The name is weird for what it is measuring. Yup. Do you like the new name and use? http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... net/http/http_cache_transaction.cc:821: MaybeUpdateDisposition(DISPOSITION_ENTRY_VALIDATED_304); On 2012/07/21 02:24:21, rvargas wrote: > This is more than just 304. Why not drop the 304 part? Done, the new more consistant names are good. Thanks for suggesting it. But since this code doesn't report on partial requests which could return 206, what's the other case here? http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... net/http/http_cache_transaction.cc:825: MaybeUpdateDisposition(DISPOSITION_ENTRY_VALIDATED_200); On 2012/07/21 02:24:21, rvargas wrote: > What about names that match the histogram? used, validated, updated. > Done. http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... net/http/http_cache_transaction.cc:1268: started_reading_since_ = TimeTicks::Now(); On 2012/07/21 02:24:21, rvargas wrote: > Now I'm confused about what do you want to measure with > started_reading_since_... there are 3 different values used. You're right, it was too confusing. I came into the CL with a concern about conditional GETs, and forced that view on all of the transactions. The new code talks about send_request_since_, and it's happy with transactions that lack that. http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... net/http/http_cache_transaction.cc:2064: DVLOG(1) << "Recorded: " << request_->method << request_->url On 2012/07/21 02:24:21, rvargas wrote: > unrelated: we should just delete this log. Done. http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... net/http/http_cache_transaction.cc:2161: void HttpCache::Transaction::MaybeUpdateDisposition( On 2012/07/21 02:24:21, rvargas wrote: > Remove the "maybe" part from the name. Done. http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... net/http/http_cache_transaction.cc:2162: const Disposition disposition) { On 2012/07/21 02:24:21, rvargas wrote: > Drop the const Done. http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... net/http/http_cache_transaction.cc:2165: if (disposition_ == DISPOSITION_DO_NOT_RECORD) On 2012/07/21 02:24:21, rvargas wrote: > is this really worth the extra work?. We could just have a single value for > don't track and uninitialized, and avoid a bunch of calls. Done. Having both and the DCHECK was very good for me to go through the unit tests, and some live runs, to make sure I had at least the beginning of an idea of the paths through this state machine. http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... net/http/http_cache_transaction.cc:2168: disposition == DISPOSITION_DO_NOT_RECORD); On 2012/07/21 02:24:21, rvargas wrote: > cannot reach do_not_record Hrm, you are wrong. Does this mean I'm naming my parameter badly? http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... net/http/http_cache_transaction.cc:2172: void HttpCache::Transaction::RecordHistograms(const int resource_size) { On 2012/07/21 02:24:21, rvargas wrote: > Drop the const Done. http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... net/http/http_cache_transaction.cc:2181: DCHECK_NE(DISPOSITION_UNDEFINED, disposition_); On 2012/07/21 02:24:21, rvargas wrote: > On 2012/07/20 17:19:24, gavinp wrote: > > Should these DCHECKs be changed into early returns? Or should we have both? > > DCHECKS seem file to me. What I found very interesting was how many different ways there were through the state machine when considering all the potential validation paths for resources. A few browsing sessions and the unit tests hit these DCHECKs many times. I'm a bit concerned that I missed some corner case, but on the other hand, I found the HttpCache test suite very complete in this area, it's as if someone else has spent a long time going over these cases as they came up one by one... http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... net/http/http_cache_transaction.cc:2185: const TimeDelta before_reading_time = On 2012/07/21 02:24:21, rvargas wrote: > I also think we should drop the const on these variables. It's not that useful > and add noise/distraction. Done. http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... net/http/http_cache_transaction.cc:2193: const bool is_small_resource = resource_size < kSmallResourceMaxBytes; On 2012/07/21 02:24:21, rvargas wrote: > This is not what the caller is passing... I mean, to compare against a value of > a single network read you'd have to add the http headers too. Good point. It's also undefined since I don't really know the initial CWND, the actual MTU, if there was a drop, etc...., the idea was to BS my way into something that halfway works. But headers can vary a lot in size, might be good to include them. The new upload makes a first pass at just summing the reads from each of the cache and network. Is this crazy? http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... net/http/http_cache_transaction.cc:2200: // The more detailed histograms are only interesting for requests that are not On 2012/07/21 02:24:21, rvargas wrote: > why? Good point I had no reason whatsoever to actually know this. I'm tracking both absolute time per request and the percents. If I become suspicious that short requests have funny behaviour, I can add another test. http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... net/http/http_cache_transaction.cc:2209: UMA_HISTOGRAM_PERCENTAGE("HttpCache.PercentBlocked", before_reading_percent); On 2012/07/21 02:24:21, rvargas wrote: > nit: Not really blocked Now named how long before send. The request isn't blocked, however, for many requests, an unmodified request cannot be sent until the disk performs the ritual reads. http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... net/http/http_cache_transaction.cc:2216: switch (disposition_) { On 2012/07/21 02:24:21, rvargas wrote: > nit: I'm not really happy with the term "disposition". It covers "do not track" > plus whether the response came from the cache or not (with revalidation or not). > > How about a name that conveys a tracking/histogram, type/status? > track_status, cached_status, transaction_type (too vague)... something like > that. Now that you point it out, I hate the name too. The new one is "TransactionPattern". Does that manage the right combination of specific and general? http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... net/http/http_cache_transaction.cc:2221: "HttpCache.PercentBlocked.NotCached", before_reading_percent); On 2012/07/21 02:24:21, rvargas wrote: > Id suggest just dropping the absolute times. You might be right. I'd like to wait for results. I promise I'll aggressively narrow these if we don't find them useful.
http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... File net/http/http_cache_transaction.cc (right): http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... net/http/http_cache_transaction.cc:2165: if (disposition_ == DISPOSITION_DO_NOT_RECORD) On 2012/07/25 01:07:01, gavinp wrote: > On 2012/07/21 02:24:21, rvargas wrote: > > is this really worth the extra work?. We could just have a single value for > > don't track and uninitialized, and avoid a bunch of calls. > > Done. Having both and the DCHECK was very good for me to go through the unit > tests, and some live runs, to make sure I had at least the beginning of an idea > of the paths through this state machine. I should have updated this comment before uploading; my apologies. I tried updating it in the way you asked, but the problem was that some transactions get identified as one or the other simple patterns, but then later become DO_NOT_RECORD. Then they might get updated again... I could tighten up all my logic for assigning patterns, or have the check here for DO_NOT_RECORD. So I went with this.
http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... File net/http/http_cache_transaction.cc (right): http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... net/http/http_cache_transaction.cc:44: namespace { On 2012/07/25 01:07:01, gavinp wrote: > On 2012/07/21 02:24:21, rvargas wrote: > > Move this outside of net > > Done. You didn't say why: is it because it doesn't reference anything in net? > In part because it's the common pattern everywhere (even when it references stuff from the file namespace) In part because I dislike nested namespaces :) http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... net/http/http_cache_transaction.cc:802: Do you mind keeping this line? http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... net/http/http_cache_transaction.cc:821: MaybeUpdateDisposition(DISPOSITION_ENTRY_VALIDATED_304); On 2012/07/25 01:07:01, gavinp wrote: > On 2012/07/21 02:24:21, rvargas wrote: > > This is more than just 304. Why not drop the 304 part? > > Done, the new more consistant names are good. Thanks for suggesting it. > > But since this code doesn't report on partial requests which could return 206, > what's the other case here? No other case, but to know that 206 doesn't matter requires reading something a thousand lines apart :) http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... net/http/http_cache_transaction.cc:1333: do you mind keeping the line? http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... net/http/http_cache_transaction.cc:2168: disposition == DISPOSITION_DO_NOT_RECORD); On 2012/07/25 01:07:01, gavinp wrote: > On 2012/07/21 02:24:21, rvargas wrote: > > cannot reach do_not_record > > Hrm, you are wrong. Does this mean I'm naming my parameter badly? Why am I wrong? the previous line returns if p_not_covered. http://codereview.chromium.org/10808047/diff/18002/net/http/http_cache_transa... File net/http/http_cache_transaction.cc (right): http://codereview.chromium.org/10808047/diff/18002/net/http/http_cache_transa... net/http/http_cache_transaction.cc:830: next_state_ = STATE_OVERWRITE_CACHED_RESPONSE; Shouldn't we care about WRITE here? If it's not already not_cached, it may mean that it is cached but we are overwriting the entry anyway... shouldn't that go into the not_cached bucket, or is that case completely handled somewhere else. http://codereview.chromium.org/10808047/diff/18002/net/http/http_cache_transa... net/http/http_cache_transaction.cc:2171: if (cache_->mode() != NORMAL || request_->method != "GET") How about ignoring other cache types here. Otherwise we have to generate multiple histograms (merging them is not a good idea). http://codereview.chromium.org/10808047/diff/18002/net/http/http_cache_transa... net/http/http_cache_transaction.cc:2182: UMA_HISTOGRAM_TIMES("HttpCache.TotalTime", total_time); HttpCache.TotalTime suggests from Start to finish. http://codereview.chromium.org/10808047/diff/18002/net/http/http_cache_transa... net/http/http_cache_transaction.cc:2192: switch (transaction_pattern_) { Did you consider a simple if/else here?
Another few uploads later, and I think this is much better. Adding Backend::GetCacheType() added a lot of files to the review. http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... File net/http/http_cache_transaction.cc (right): http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... net/http/http_cache_transaction.cc:802: On 2012/07/25 19:41:04, rvargas wrote: > Do you mind keeping this line? Done. http://codereview.chromium.org/10808047/diff/2004/net/http/http_cache_transac... net/http/http_cache_transaction.cc:2168: disposition == DISPOSITION_DO_NOT_RECORD); On 2012/07/25 19:41:04, rvargas wrote: > On 2012/07/25 01:07:01, gavinp wrote: > > On 2012/07/21 02:24:21, rvargas wrote: > > > cannot reach do_not_record > > > > Hrm, you are wrong. Does this mean I'm naming my parameter badly? > > Why am I wrong? the previous line returns if p_not_covered. The previous line compares the member variable |disposition_| to DISPOSITION_..., the new line compares |disposition|, the function parameter. Names changed. http://codereview.chromium.org/10808047/diff/18002/net/http/http_cache_transa... File net/http/http_cache_transaction.cc (right): http://codereview.chromium.org/10808047/diff/18002/net/http/http_cache_transa... net/http/http_cache_transaction.cc:830: next_state_ = STATE_OVERWRITE_CACHED_RESPONSE; On 2012/07/25 19:41:04, rvargas wrote: > Shouldn't we care about WRITE here? If it's not already not_cached, it may mean > that it is cached but we are overwriting the entry anyway... shouldn't that go > into the not_cached bucket, or is that case completely handled somewhere else. I think we're good, if it's just WRITE, we'll either have found it was not cached in DoOpenEntryComplete, and marked it NOT_CACHED, and it's really the same as a read write request, or it's still UNDEFINED because there was a cache entry. I guess I could consider that NOT_CACHED? But it is enough different that I feel like I want to just let it stay undefined... http://codereview.chromium.org/10808047/diff/18002/net/http/http_cache_transa... net/http/http_cache_transaction.cc:2171: if (cache_->mode() != NORMAL || request_->method != "GET") On 2012/07/25 19:41:04, rvargas wrote: > How about ignoring other cache types here. Otherwise we have to generate > multiple histograms (merging them is not a good idea). Done. http://codereview.chromium.org/10808047/diff/18002/net/http/http_cache_transa... net/http/http_cache_transaction.cc:2192: switch (transaction_pattern_) { On 2012/07/25 19:41:04, rvargas wrote: > Did you consider a simple if/else here? Done.
http://codereview.chromium.org/10808047/diff/18002/net/http/http_cache_transa... File net/http/http_cache_transaction.cc (right): http://codereview.chromium.org/10808047/diff/18002/net/http/http_cache_transa... net/http/http_cache_transaction.cc:830: next_state_ = STATE_OVERWRITE_CACHED_RESPONSE; On 2012/07/28 01:36:06, gavinp wrote: > On 2012/07/25 19:41:04, rvargas wrote: > > Shouldn't we care about WRITE here? If it's not already not_cached, it may > mean > > that it is cached but we are overwriting the entry anyway... shouldn't that go > > into the not_cached bucket, or is that case completely handled somewhere else. > > I think we're good, if it's just WRITE, we'll either have found it was not > cached in DoOpenEntryComplete, and marked it NOT_CACHED, and it's really the > same as a read write request, or it's still UNDEFINED because there was a cache > entry. I guess I could consider that NOT_CACHED? But it is enough different that > I feel like I want to just let it stay undefined... I think the case is common enough to either be part of NOT_CACHED (we go through the logic of opening the entry and then update/write the response, so in therms of work it is basically the same), or it should be made into a group. UNDEFINED is sort of for rare error cases or complicated stuff that we don't want to deal with. http://codereview.chromium.org/10808047/diff/12003/net/http/http_cache_transa... File net/http/http_cache_transaction.cc (right): http://codereview.chromium.org/10808047/diff/12003/net/http/http_cache_transa... net/http/http_cache_transaction.cc:2195: } else if (transaction_pattern_ == PATTERN_ENTRY_VALIDATED || nit: simple else and DCHECK the pattern.
One thing I would add: You are differentiating the different pattern types, and e.g. for NOT_COVERED, you simply do not record any histogram. I would recommend recording how frequently each pattern type occurs. That way, if the patterns you are excluding make up a significant amount of overall requests, you can at least see that, and be aware of that when drawing conclusions based on histogram data. Otherwise, it may be that you think you can make a great improvement, not realizing that it represents a much smaller fraction of cases that you thought it would.
On 2012/07/30 21:19:01, tburkard wrote: > One thing I would add: > You are differentiating the different pattern types, and e.g. for > NOT_COVERED, you simply do not record any histogram. I would recommend > recording how frequently each pattern type occurs. That way, if the > patterns you are excluding make up a significant amount of overall > requests, you can at least see that, and be aware of that when drawing > conclusions based on histogram data. Otherwise, it may be that you think > you can make a great improvement, not realizing that it represents a much > smaller fraction of cases that you thought it would. Take a peek at the histogram HttpCache.HasPattern. Doesn't it serve this purpose?
Now I'm catching the DOOM case properly, and I renamed the histograms and cleaned up their organization for readability. How's this looking now? http://codereview.chromium.org/10808047/diff/18002/net/http/http_cache_transa... File net/http/http_cache_transaction.cc (right): http://codereview.chromium.org/10808047/diff/18002/net/http/http_cache_transa... net/http/http_cache_transaction.cc:830: next_state_ = STATE_OVERWRITE_CACHED_RESPONSE; On 2012/07/30 21:09:31, rvargas wrote: > On 2012/07/28 01:36:06, gavinp wrote: > > On 2012/07/25 19:41:04, rvargas wrote: > > > Shouldn't we care about WRITE here? If it's not already not_cached, it may > > mean > > > that it is cached but we are overwriting the entry anyway... shouldn't that > go > > > into the not_cached bucket, or is that case completely handled somewhere > else. > > > > I think we're good, if it's just WRITE, we'll either have found it was not > > cached in DoOpenEntryComplete, and marked it NOT_CACHED, and it's really the > > same as a read write request, or it's still UNDEFINED because there was a > cache > > entry. I guess I could consider that NOT_CACHED? But it is enough different > that > > I feel like I want to just let it stay undefined... > > I think the case is common enough to either be part of NOT_CACHED (we go through > the logic of opening the entry and then update/write the response, so in therms > of work it is basically the same), or it should be made into a group. UNDEFINED > is sort of for rare error cases or complicated stuff that we don't want to deal > with. Done. I was a bit concerned that dooming would be an extra cost, but I suppose it's asynchronous and off thread, so we can include it in PATTERN_ENTRY_NOT_CACHED. It comes up in unit tests in the *BypassCache tests, plus a few range requests. I have removed the PATTERN_ENTRY_NOT_COVERED from the LOAD_BYPASS_CACHE case, which is now mode_ gets to WRITE. And over in OnDoomEntryComplete() I added an update to PATTERN_ENTRY_NOT_CACHED. http://codereview.chromium.org/10808047/diff/12003/net/http/http_cache_transa... File net/http/http_cache_transaction.cc (right): http://codereview.chromium.org/10808047/diff/12003/net/http/http_cache_transa... net/http/http_cache_transaction.cc:2195: } else if (transaction_pattern_ == PATTERN_ENTRY_VALIDATED || On 2012/07/30 21:09:31, rvargas wrote: > nit: simple else and DCHECK the pattern. Done.
http://codereview.chromium.org/10808047/diff/21005/net/http/http_cache_transa... File net/http/http_cache_transaction.cc (right): http://codereview.chromium.org/10808047/diff/21005/net/http/http_cache_transa... net/http/http_cache_transaction.cc:807: transaction_pattern_ == PATTERN_NOT_COVERED); What I really don't like about not handling WRITE here is that, at least in my mind, the mode_ is an axis of the transaction state that is allowed to change at any given point in the state machine. This DCHECK (and the previous assumption that the interesting case was READ_WRITE and everything else at this point was not interesting), force every step of the state machine to work in a given way... if some code in this state thinks that the mode_ should be something, it should not rely on other states to make that so... in other words, I would not want to be doing something and trigger this DCHECK in a test, because then I would have to think about this logging, and it is not a fundamental invariant of the state machine (or really, this state). (probably I'm not making too much sense, but that's the idea). In other words, between sets 9 and 10 I actually prefer 9. Nothing changed in this state (the one I cared about), and now the code is a little more complex, and the histogram names are again suggesting Start to finish :( In other words, at this point I'm fine with just checking in set 9 (plus the fix for simplifying that if clause). Before looking at this code I was thinking that I should actually have give you an lgtm for the last version, if you wanted to check that in :( Sorry about that.
lgtm On Tue, Jul 31, 2012 at 10:43 AM, <rvargas@chromium.org> wrote: > > http://codereview.chromium.**org/10808047/diff/21005/net/** > http/http_cache_transaction.cc<http://codereview.chromium.org/10808047/diff/21005/net/http/http_cache_transaction.cc> > File net/http/http_cache_**transaction.cc (right): > > http://codereview.chromium.**org/10808047/diff/21005/net/** > http/http_cache_transaction.**cc#newcode807<http://codereview.chromium.org/10808047/diff/21005/net/http/http_cache_transaction.cc#newcode807> > net/http/http_cache_**transaction.cc:807: transaction_pattern_ == > PATTERN_NOT_COVERED); > What I really don't like about not handling WRITE here is that, at least > in my mind, the mode_ is an axis of the transaction state that is > allowed to change at any given point in the state machine. > > This DCHECK (and the previous assumption that the interesting case was > READ_WRITE and everything else at this point was not interesting), force > every step of the state machine to work in a given way... if some code > in this state thinks that the mode_ should be something, it should not > rely on other states to make that so... in other words, I would not want > to be doing something and trigger this DCHECK in a test, because then I > would have to think about this logging, and it is not a fundamental > invariant of the state machine (or really, this state). > > (probably I'm not making too much sense, but that's the idea). > > In other words, between sets 9 and 10 I actually prefer 9. Nothing > changed in this state (the one I cared about), and now the code is a > little more complex, and the histogram names are again suggesting Start > to finish :( > > In other words, at this point I'm fine with just checking in set 9 (plus > the fix for simplifying that if clause). Before looking at this code I > was thinking that I should actually have give you an lgtm for the last > version, if you wanted to check that in :( > > Sorry about that. > > http://codereview.chromium.**org/10808047/<http://codereview.chromium.org/108... >
How's this? I can go back to the pattern from UL 9 if you don't like it. https://chromiumcodereview.appspot.com/10808047/diff/21005/net/http/http_cach... File net/http/http_cache_transaction.cc (right): https://chromiumcodereview.appspot.com/10808047/diff/21005/net/http/http_cach... net/http/http_cache_transaction.cc:807: transaction_pattern_ == PATTERN_NOT_COVERED); On 2012/07/31 17:43:57, rvargas wrote: > What I really don't like about not handling WRITE here is that, at least in my > mind, the mode_ is an axis of the transaction state that is allowed to change at > any given point in the state machine. > > This DCHECK (and the previous assumption that the interesting case was > READ_WRITE and everything else at this point was not interesting), force every > step of the state machine to work in a given way... if some code in this state > thinks that the mode_ should be something, it should not rely on other states to > make that so... in other words, I would not want to be doing something and > trigger this DCHECK in a test, because then I would have to think about this > logging, and it is not a fundamental invariant of the state machine (or really, > this state). > > (probably I'm not making too much sense, but that's the idea). > > In other words, between sets 9 and 10 I actually prefer 9. Nothing changed in > this state (the one I cared about), and now the code is a little more complex, > and the histogram names are again suggesting Start to finish :( > > In other words, at this point I'm fine with just checking in set 9 (plus the fix > for simplifying that if clause). Before looking at this code I was thinking that > I should actually have give you an lgtm for the last version, if you wanted to > check that in :( > > Sorry about that. You're right about the state machine. My new upload moves the pattern setter to this function, and as a consequence I caught more PATTERN_NOT_COVERED cases.
LGTM (I wish we were not sprinkling the code with PATTERN_NOT_COVERED)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gavinp@chromium.org/10808047/25011
Try job failure for 10808047-25011 (retry) on win_rel for steps "nacl_integration, net_unittests". It's a second try, previously, steps "nacl_integration, net_unittests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gavinp@chromium.org/10808047/25011
Try job failure for 10808047-25011 (retry) on win_rel for steps "nacl_integration, net_unittests". It's a second try, previously, steps "nacl_integration, net_unittests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gavinp@chromium.org/10808047/25011
Try job failure for 10808047-25011 (retry) on win_rel for steps "nacl_integration, net_unittests". It's a second try, previously, steps "nacl_integration, net_unittests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gavinp@chromium.org/10808047/25011
Try job failure for 10808047-25011 (retry) on win_rel for steps "nacl_integration, net_unittests". It's a second try, previously, steps "nacl_integration, net_unittests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gavinp@chromium.org/10808047/16035
Try job failure for 10808047-16035 (retry) on mac_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gavinp@chromium.org/10808047/16038
Change committed as 150902 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
