Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
632033002:
Add PLT measurement to Resource Prefetching for Mobile Web (Closed)
DescriptionAdd PLT measurement to Resource Prefetching for Mobile Web
histograms.xml is also updated. This update includes the ones for PLT measurement.
BUG=405690, 406200
Committed: https://crrev.com/3b9ba02c6439832abab9bea9163646652d81bbc5
Cr-Commit-Position: refs/heads/master@{#302505}
Patch Set 1 #
Total comments: 8
Patch Set 2 : address comments and update histograms.xml #Patch Set 3 : rebase to fix patch error #
Total comments: 14
Patch Set 4 : address review comments #Patch Set 5 : avoid using STATIC_HISTOGRAM_POINTER_BLOCK #
Total comments: 19
Patch Set 6 : review fix #
Total comments: 6
Patch Set 7 : review fix #Patch Set 8 : review fix - separate unrelated UMA to later CL #
Total comments: 2
Patch Set 9 : use nested suffix #Patch Set 10 : use unified ResourcePrefetchPredictorNetworkType #Patch Set 11 : use suffix for ResourcePrefetchPredictor.NetworkType #
Total comments: 4
Patch Set 12 : review fix #Patch Set 13 : rebase #Patch Set 14 : rebase fix #
Total comments: 2
Patch Set 15 : review fix #
Messages
Total messages: 33 (4 generated)
zhenw@chromium.org changed reviewers: + shishir@chromium.org, shishir@google.com, tburkard@chromium.org
ptal
It'd be useful to also add someone from cbentzel's team (someone who is currently working on Chrome), for example: jkarlin, mmenke, or davidben. Also, histograms.xml must be updated to reflect the new histograms, and asvitkine@ or rkaplow@ must be added to review the histograms in general and histograms.xml in particular. Thanks. https://codereview.chromium.org/632033002/diff/1/chrome/browser/predictors/re... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/632033002/diff/1/chrome/browser/predictors/re... chrome/browser/predictors/resource_prefetch_predictor.cc:108: enum PageNotPrefetchedNetworkType { can't these be combined into a single enum, or, better yet, just reuse the enum types from net? https://codereview.chromium.org/632033002/diff/1/chrome/browser/predictors/re... chrome/browser/predictors/resource_prefetch_predictor.cc:1160: REPORT_PLT("ResourcePrefetchPredictor.PLT.PagePrefetched." + histogram_type, see comment below, this shouldn't work. https://codereview.chromium.org/632033002/diff/1/chrome/browser/predictors/re... chrome/browser/predictors/resource_prefetch_predictor.cc:1162: REPORT_PLT("ResourcePrefetchPredictor.PLT.PagePrefetched." + histogram_type ditto https://codereview.chromium.org/632033002/diff/1/chrome/browser/predictors/re... chrome/browser/predictors/resource_prefetch_predictor.cc:1193: + GetNetworkTypeString(), plt); This should not work, if the user switches networks. Can you test this condition? The reason is that the UMA macros expand to a { } with a static variable, initialized with the histogram name, and the histogram name must remain constant across all calls to the macro. See prerender_histograms.cc, we have the same issue there, and it must be addressed by duplicating the UMA call for each possible value of the variable.
I just found out HistogramTester. Will add some tests to make sure the UMA recording works. By the way, should I update histograms.xml in the same CL? There are many other UMA metrics not related to PLT measurement.
As discussed, you don't need to write tests for histogram, just verify it by hand using chrome://histograms Yes, histograms.xml changes should go in the same CL, and add asvitkine or rkaplow as reviewers. Thanks. On Wed, Oct 8, 2014 at 7:56 PM, <zhenw@chromium.org> wrote: > I just found out HistogramTester. Will add some tests to make sure the UMA > recording works. > > By the way, should I update histograms.xml in the same CL? There are many > other > UMA metrics not related to PLT measurement. > > https://codereview.chromium.org/632033002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
zhenw@chromium.org changed reviewers: + asvitkine@chromium.org
Comments addressed. ptal. Alexei, can you review histograms.xml? Thanks! https://codereview.chromium.org/632033002/diff/1/chrome/browser/predictors/re... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/632033002/diff/1/chrome/browser/predictors/re... chrome/browser/predictors/resource_prefetch_predictor.cc:108: enum PageNotPrefetchedNetworkType { Reusing enum types from net now. https://codereview.chromium.org/632033002/diff/1/chrome/browser/predictors/re... chrome/browser/predictors/resource_prefetch_predictor.cc:1160: REPORT_PLT("ResourcePrefetchPredictor.PLT.PagePrefetched." + histogram_type, On 2014/10/08 07:58:29, tburkard wrote: > see comment below, this shouldn't work. Done. https://codereview.chromium.org/632033002/diff/1/chrome/browser/predictors/re... chrome/browser/predictors/resource_prefetch_predictor.cc:1162: REPORT_PLT("ResourcePrefetchPredictor.PLT.PagePrefetched." + histogram_type On 2014/10/08 07:58:29, tburkard wrote: > ditto Done. https://codereview.chromium.org/632033002/diff/1/chrome/browser/predictors/re... chrome/browser/predictors/resource_prefetch_predictor.cc:1193: + GetNetworkTypeString(), plt); I see. I duplicate the UMA calls now.
https://codereview.chromium.org/632033002/diff/40005/chrome/browser/predictor... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/632033002/diff/40005/chrome/browser/predictor... chrome/browser/predictors/resource_prefetch_predictor.cc:95: CONNECTION_CELLULAR = net::NetworkChangeNotifier::CONNECTION_LAST + 1, Nit: Indentation is wrong. https://codereview.chromium.org/632033002/diff/40005/chrome/browser/predictor... chrome/browser/predictors/resource_prefetch_predictor.cc:1025: base::TimeDelta::FromSeconds(60), \ I don't think 60s is a good upper bound - there are probably pages that take longer to load. I suggest using UMA_HISTOGRAM_MEDIUM_TIMES(), which gives you an upper bound of 3minutes. It is 50 buckets, rather than 100, but I think given that you're adding so many histograms, its wasteful to have them all be 100 buckets, in terms of memory used on the client and data used in logs. https://codereview.chromium.org/632033002/diff/40005/chrome/browser/predictor... chrome/browser/predictors/resource_prefetch_predictor.cc:1074: "ResourcePrefetchPredictor.PagePrefetchedNetworkType", Please make a helper function for this histogram which takes the enum value as a param. Since each histogram macro block expands to a lot of code, this will help reduce some bloat. https://codereview.chromium.org/632033002/diff/40005/chrome/browser/predictor... chrome/browser/predictors/resource_prefetch_predictor.cc:1162: REPORT_PLT("ResourcePrefetchPredictor.PLT.PagePrefetched.Url", plt); Honestly, I feel like it would be better to just use the underlying Histogram* API directly, rather than going through the macros. Then, you can avoid all of this duplication and just have helper functions that return suffix strings for the particular enums. https://codereview.chromium.org/632033002/diff/40005/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/632033002/diff/40005/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:28142: +<histogram name="ResourcePrefetchPredictor.PLT.PagePrefetched.4G" Please use the histogram_suffixes feature to avoid all of this duplication in the file. You can see some docs for it at the top of the file and also some examples of its use within the file. For example, you can define a list of suffixes for the connection type (e.g. "3G", "4G", etc) and set affected-histogram to ResourcePrefetchPredictor.PLT.PagePrefetched.
https://codereview.chromium.org/632033002/diff/40005/chrome/browser/predictor... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/632033002/diff/40005/chrome/browser/predictor... chrome/browser/predictors/resource_prefetch_predictor.cc:95: CONNECTION_CELLULAR = net::NetworkChangeNotifier::CONNECTION_LAST + 1, On 2014/10/14 19:27:27, Alexei Svitkine wrote: > Nit: Indentation is wrong. Done. https://codereview.chromium.org/632033002/diff/40005/chrome/browser/predictor... chrome/browser/predictors/resource_prefetch_predictor.cc:1025: base::TimeDelta::FromSeconds(60), \ The granularity of 3 minutes with 50 buckets is too coarse (3.6s). The granularity of 1 minute with 100 buckets (0.6s) seems most useful. For current stage, we do not care too much about super long PLT pages. Although there are many histograms for PLT, they are usually exclusive to each other because there is only one network type for each page load. And this is consistent with the implementation in prerender_histograms (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pre...). https://codereview.chromium.org/632033002/diff/40005/chrome/browser/predictor... chrome/browser/predictors/resource_prefetch_predictor.cc:1074: "ResourcePrefetchPredictor.PagePrefetchedNetworkType", On 2014/10/14 19:27:27, Alexei Svitkine wrote: > Please make a helper function for this histogram which takes the enum value as a > param. Since each histogram macro block expands to a lot of code, this will help > reduce some bloat. Done. https://codereview.chromium.org/632033002/diff/40005/chrome/browser/predictor... chrome/browser/predictors/resource_prefetch_predictor.cc:1162: REPORT_PLT("ResourcePrefetchPredictor.PLT.PagePrefetched.Url", plt); I originally have a more concise version (patch 1). e.g. REPORT_PLT("ResourcePrefetchPredictor.PLT.PagePrefetched." + histogram_type + "." + GetNetworkTypeString(), plt); But it turns out that each macro should records a const name. So I have to expand it to this duplication. Or is there a better histogram API for me to use? https://codereview.chromium.org/632033002/diff/40005/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/632033002/diff/40005/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:28142: +<histogram name="ResourcePrefetchPredictor.PLT.PagePrefetched.4G" On 2014/10/14 19:27:27, Alexei Svitkine wrote: > Please use the histogram_suffixes feature to avoid all of this duplication in > the file. You can see some docs for it at the top of the file and also some > examples of its use within the file. > > For example, you can define a list of suffixes for the connection type (e.g. > "3G", "4G", etc) and set affected-histogram to > ResourcePrefetchPredictor.PLT.PagePrefetched. Done.
https://codereview.chromium.org/632033002/diff/40005/chrome/browser/predictor... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/632033002/diff/40005/chrome/browser/predictor... chrome/browser/predictors/resource_prefetch_predictor.cc:1025: base::TimeDelta::FromSeconds(60), \ On 2014/10/16 18:12:22, Zhen Wang wrote: > The granularity of 3 minutes with 50 buckets is too coarse (3.6s). The > granularity of 1 minute with 100 buckets (0.6s) seems most useful. For current > stage, we do not care too much about super long PLT pages. Although there are > many histograms for PLT, they are usually exclusive to each other because there > is only one network type for each page load. > > And this is consistent with the implementation in prerender_histograms > (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pre...). Are you aware that these histograms will have bucket sizes that are exponentially increasing? You can't just divide 3 minutes by 50, since it's not a linear distribution. Instead, you'll get more granularity at the low range and coarser granularity closer to the max value. https://codereview.chromium.org/632033002/diff/40005/chrome/browser/predictor... chrome/browser/predictors/resource_prefetch_predictor.cc:1162: REPORT_PLT("ResourcePrefetchPredictor.PLT.PagePrefetched.Url", plt); On 2014/10/16 18:12:22, Zhen Wang wrote: > I originally have a more concise version (patch 1). e.g. > > REPORT_PLT("ResourcePrefetchPredictor.PLT.PagePrefetched." + histogram_type + > "." + GetNetworkTypeString(), plt); > > But it turns out that each macro should records a const name. So I have to > expand it to this duplication. Or is there a better histogram API for me to use? Yeah, you can't use the macro. You need to use the FactoryGet() API of a histogram directly. You can look at what the UMA macro expands to and use that minus the static var that's used for caching.
https://codereview.chromium.org/632033002/diff/40005/chrome/browser/predictor... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/632033002/diff/40005/chrome/browser/predictor... chrome/browser/predictors/resource_prefetch_predictor.cc:1025: base::TimeDelta::FromSeconds(60), \ Oh, thanks for pointing it out. Using medium times now. https://codereview.chromium.org/632033002/diff/40005/chrome/browser/predictor... chrome/browser/predictors/resource_prefetch_predictor.cc:1162: REPORT_PLT("ResourcePrefetchPredictor.PLT.PagePrefetched.Url", plt); Using self-defined RPP_HISTOGRAM_MEDIUM_TIMES now.
I think this is on the right track, though I have a few more suggestions. https://codereview.chromium.org/632033002/diff/170001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/632033002/diff/170001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:95: CONNECTION_CELLULAR = net::NetworkChangeNotifier::CONNECTION_LAST + 1, What happens if net::NetworkChangeNotifier::CONNECTION_LAST is updated? Now, your histogram bucket will overlap with another bucket depending on the version of Chrome. I don't think you should do this. Either use a separate histogram for these, or instead use values that can't appear in net::NetworkChangeNotifier after updates - for example use a sparse histogram and use negative placeholders -1, -2, etc. (You need to use a sparse histogram if you want to use negative values.) https://codereview.chromium.org/632033002/diff/170001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:125: "ResourcePrefetchPredictor.PagePrefetchedNetworkType", I suggest naming this one and the one below to have the same prefix, so you get grouping on the dashboard. How about: ResourcePrefetchPredictor.NetworkType.PagePrefetched ResourcePrefetchPredictor.NetworkType.PageNotPrefetched https://codereview.chromium.org/632033002/diff/170001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:1060: { \ Nit: Make this a do { } while (0) loop. Also, indent everything below the macro 4 more. https://codereview.chromium.org/632033002/diff/170001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:1119: void ResourcePrefetchPredictor::ReportPageLoadTimeNotPrefetchedStats( It seems the logic of this function mirrors pretty closely what's done in ReportPageLoadTimePrefetchedStats() except for PagePrefetched vs. PageNotPrefetched. Can you make a helper function? I imagine it would need to take this following params: const std::string& prefetched_str, (either PagePrefetched or PageNotPrefetched) base::Callback<void(int)> record_network_type_callback, (either ReportPageNotPrefetchedNetworkType or ReportPagePrefetchedNetworkType). One other suggestion: maybe in all of these histograms, make the middle string shorter - i.e. instead of PagePrefetched/PageNotPrefetched, use simply Prefetched/NotPrefetched. https://codereview.chromium.org/632033002/diff/170001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:1128: net::NetworkChangeNotifier::GetConnectionType()); Nit: Use connection_type which you have as a variable above. https://codereview.chromium.org/632033002/diff/170001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:1135: "ResourcePrefetchPredictor.PLT.PageNotPrefetched" + GetNetTypeStr(), plt); Nit: I'd find it clearer if you make the "_" be part of this string at the end, rather than returned by GetNetTypeStr(). https://codereview.chromium.org/632033002/diff/170001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/632033002/diff/170001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27695: +<histogram name="ResourcePrefetchPredictor.HistoryVisitCountForUrl"> I don't see this in your CL (as well as many of the other histogram descriptions you're adding), what's the story here?
https://codereview.chromium.org/632033002/diff/170001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/632033002/diff/170001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:95: CONNECTION_CELLULAR = net::NetworkChangeNotifier::CONNECTION_LAST + 1, Good point. Using UMA_HISTOGRAM_SPARSE_SLOWLY now. Should I worry about this "_SLOWLY" thing? How slower is it? https://codereview.chromium.org/632033002/diff/170001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:125: "ResourcePrefetchPredictor.PagePrefetchedNetworkType", We need to put pages prefetched in different network types into one group and put pages not prefetched in different network types into another group. This is to match with ResourcePrefetchPredictor.PLT.Prefetched.{NetType}. So I think current naming should be easiest to see on dashboard. https://codereview.chromium.org/632033002/diff/170001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:1060: { \ Done. By the way, why do we prefer the style of "do { } while (0) loop" for macros? https://codereview.chromium.org/632033002/diff/170001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:1119: void ResourcePrefetchPredictor::ReportPageLoadTimeNotPrefetchedStats( On 2014/10/20 18:30:35, Alexei Svitkine wrote: > It seems the logic of this function mirrors pretty closely what's done in > ReportPageLoadTimePrefetchedStats() except for PagePrefetched vs. > PageNotPrefetched. > > Can you make a helper function? > > I imagine it would need to take this following params: > const std::string& prefetched_str, (either PagePrefetched or > PageNotPrefetched) > base::Callback<void(int)> record_network_type_callback, > (either ReportPageNotPrefetchedNetworkType or > ReportPagePrefetchedNetworkType). > > One other suggestion: maybe in all of these histograms, make the middle string > shorter - i.e. instead of PagePrefetched/PageNotPrefetched, use simply > Prefetched/NotPrefetched. Done. https://codereview.chromium.org/632033002/diff/170001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:1128: net::NetworkChangeNotifier::GetConnectionType()); On 2014/10/20 18:30:35, Alexei Svitkine wrote: > Nit: Use connection_type which you have as a variable above. Done. https://codereview.chromium.org/632033002/diff/170001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:1135: "ResourcePrefetchPredictor.PLT.PageNotPrefetched" + GetNetTypeStr(), plt); On 2014/10/20 18:30:35, Alexei Svitkine wrote: > Nit: I'd find it clearer if you make the "_" be part of this string at the end, > rather than returned by GetNetTypeStr(). Done. https://codereview.chromium.org/632033002/diff/170001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/632033002/diff/170001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27695: +<histogram name="ResourcePrefetchPredictor.HistoryVisitCountForUrl"> This resource prefetch feature was originally part of Chromium, but later was removed because the original author does not continue to maintain it and gather experiment data. We found this feature may be beneficial to mobile devices and brought the code back in CL 462423004. There were UMA recording code inside the original code. But since the purpose of CL 462423004 was to make the old code compile and work with current system, we didn't update histograms.xml. Now we have verified the old UMA recording and add some new ones for PLT; and update histograms.xml all together. See: https://codereview.chromium.org/462423004/ https://codereview.chromium.org/576313002/ (fix mem leak in 462423004)
https://codereview.chromium.org/632033002/diff/170001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/632033002/diff/170001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:95: CONNECTION_CELLULAR = net::NetworkChangeNotifier::CONNECTION_LAST + 1, On 2014/10/27 14:12:47, Zhen Wang wrote: > Good point. Using UMA_HISTOGRAM_SPARSE_SLOWLY now. > > Should I worry about this "_SLOWLY" thing? How slower is it? It involves a lock on the histogram object and a std::map operation. So slower than updating an entry in an array, as is the case with regular histograms, but not generally still quite fast. https://codereview.chromium.org/632033002/diff/170001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:125: "ResourcePrefetchPredictor.PagePrefetchedNetworkType", On 2014/10/27 14:12:47, Zhen Wang wrote: > We need to put pages prefetched in different network types into one group and > put pages not prefetched in different network types into another group. This is > to match with ResourcePrefetchPredictor.PLT.Prefetched.{NetType}. > > So I think current naming should be easiest to see on dashboard. My suggestion is not contrary to that. I'm just suggesting using different names for the two histograms you're logging here. i.e. ResourcePrefetchPredictor.PagePrefetchedNetworkType -> ResourcePrefetchPredictor.NetworkType.PagePrefetched ResourcePrefetchPredictor.PageNotPrefetchedNetworkType -> ResourcePrefetchPredictor.NetworkType.PageNotPrefetched The idea is both are a specialization of the "ResourcePrefetchPredictor.NetworkType" base histogram - one specialization for "PagePrefetched" and another for "PageNotPrefetched", but otherwise logging the same type of thing. Then, you can access both histograms by typing ResourcePrefetchPredictor.NetworkType.* on the dashboard. https://codereview.chromium.org/632033002/diff/190001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/632033002/diff/190001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:1118: if (prefetched) { Nit: Make this an early return with inverse condition to avoid extra nesting below. https://codereview.chromium.org/632033002/diff/190001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:1125: + "_" + GetNetTypeStr(), Nit: I think the + "_" fits on the previous line. https://codereview.chromium.org/632033002/diff/190001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/632033002/diff/190001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:275: PrefetchKeyType key_type = PREFETCH_KEY_TYPE_URL) const; Nit: I thinks style guide discourages use of default param values.
https://codereview.chromium.org/632033002/diff/170001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/632033002/diff/170001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:125: "ResourcePrefetchPredictor.PagePrefetchedNetworkType", I see. Thanks for the clarification! Using new naming now. https://codereview.chromium.org/632033002/diff/190001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/632033002/diff/190001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:1118: if (prefetched) { On 2014/10/27 14:59:10, Alexei Svitkine wrote: > Nit: Make this an early return with inverse condition to avoid extra nesting > below. Done. https://codereview.chromium.org/632033002/diff/190001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:1125: + "_" + GetNetTypeStr(), On 2014/10/27 14:59:10, Alexei Svitkine wrote: > Nit: I think the + "_" fits on the previous line. Done. https://codereview.chromium.org/632033002/diff/190001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/632033002/diff/190001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:275: PrefetchKeyType key_type = PREFETCH_KEY_TYPE_URL) const; On 2014/10/27 14:59:10, Alexei Svitkine wrote: > Nit: I thinks style guide discourages use of default param values. Done.
ping
https://codereview.chromium.org/632033002/diff/170001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/632033002/diff/170001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27695: +<histogram name="ResourcePrefetchPredictor.HistoryVisitCountForUrl"> On 2014/10/27 14:12:47, Zhen Wang wrote: > This resource prefetch feature was originally part of Chromium, but later was > removed because the original author does not continue to maintain it and gather > experiment data. We found this feature may be beneficial to mobile devices and > brought the code back in CL 462423004. > > There were UMA recording code inside the original code. But since the purpose of > CL 462423004 was to make the old code compile and work with current system, we > didn't update histograms.xml. > > Now we have verified the old UMA recording and add some new ones for PLT; and > update histograms.xml all together. > > See: > https://codereview.chromium.org/462423004/ > https://codereview.chromium.org/576313002/ (fix mem leak in 462423004) It seems these other histograms are actually listed in the internal (non-open source) version of histograms.xml: http://cs/ResourcePrefetchPredictor.HistoryVisitCountForUrl I agree we should move them over to the open source version of the file and set owners for them. But that should be done in a separate CL, not as part of this CL which is changing other code.
I removed the unrelated UMAs in histograms.xml. I will prepare another CL to those UMAs. https://codereview.chromium.org/632033002/diff/170001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/632033002/diff/170001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27695: +<histogram name="ResourcePrefetchPredictor.HistoryVisitCountForUrl"> I see. I will prepare another CL to do the movement for the internal ones.
https://codereview.chromium.org/632033002/diff/230001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/632033002/diff/230001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27975: +<histogram name="ResourcePrefetchPredictor.PLT.Prefetched" units="milliseconds"> Can you use histogram_suffixes for these with suffixes Prefetched and NotPrefetched? Note: You can specify separator="." for them. Then, for the .Host and .Url versions below, you can have another histogram_suffixes entry that affects ResourcePrefetchPredictor.PLT.Prefetched.
https://codereview.chromium.org/632033002/diff/230001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/632033002/diff/230001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27975: +<histogram name="ResourcePrefetchPredictor.PLT.Prefetched" units="milliseconds"> Ah, I didn't know I can specify the separator. Done. By the way, is there a way to use suffix for enum? for - ResourcePrefetchPredictor.NetworkType.NotPrefetched - ResourcePrefetchPredictor.NetworkType.Prefetched I tried. But "enum" is not recognized in tag "histogram_suffixes" or "suffix".
On 2014/10/29 18:23:37, Zhen Wang wrote: > https://codereview.chromium.org/632033002/diff/230001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/632033002/diff/230001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:27975: +<histogram > name="ResourcePrefetchPredictor.PLT.Prefetched" units="milliseconds"> > Ah, I didn't know I can specify the separator. > > Done. > > By the way, is there a way to use suffix for enum? for > - ResourcePrefetchPredictor.NetworkType.NotPrefetched > - ResourcePrefetchPredictor.NetworkType.Prefetched > > I tried. But "enum" is not recognized in tag "histogram_suffixes" or "suffix". It should work. You just need to define a <histogram> entry for ResourcePrefetchPredictor.NetworkType with enum="" tag and then have suffixes that affect that one.
done.
lgtm once you address the remaining two comments below https://codereview.chromium.org/632033002/diff/290001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/632033002/diff/290001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27941: + <summary>Records the number of pages on each type of network.</summary> Please mention *when* this is recorded. After a page load? https://codereview.chromium.org/632033002/diff/290001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:51194: + <int value="8" label="CONNECTION_CELLULAR"/> This and the value below are now negative, right? Please update.
lgtm
https://codereview.chromium.org/632033002/diff/290001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/632033002/diff/290001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27941: + <summary>Records the number of pages on each type of network.</summary> On 2014/10/29 19:09:46, Alexei Svitkine wrote: > Please mention *when* this is recorded. After a page load? Done. https://codereview.chromium.org/632033002/diff/290001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:51194: + <int value="8" label="CONNECTION_CELLULAR"/> Ah, right. Thanks for catching this!
zhenw@chromium.org changed reviewers: + thestig@chromium.org
Hi Lei, Shishir seems too busy to review this. Can you take a look since you reviewed a related patch before? Thanks! Best -Zhen
lgtm since Timo already approved. https://codereview.chromium.org/632033002/diff/350001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/632033002/diff/350001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:95: enum AdditionalConnectionType { Please add a comment to explain why these values are negative.
https://codereview.chromium.org/632033002/diff/350001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/632033002/diff/350001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:95: enum AdditionalConnectionType { On 2014/11/03 18:06:27, Lei Zhang wrote: > Please add a comment to explain why these values are negative. Done.
The CQ bit was checked by zhenw@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/632033002/370001
Message was sent while issue was closed.
Committed patchset #15 (id:370001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/3b9ba02c6439832abab9bea9163646652d81bbc5 Cr-Commit-Position: refs/heads/master@{#302505} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
