Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10900037:
Adding more histograms to ResourcePrefetchPredictor. (Closed)
|
Created:
8 years, 3 months ago by Shishir Modified:
8 years, 3 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdding more histograms to ResourcePrefetchPredictor.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154855
Patch Set 1 #
Total comments: 16
Patch Set 2 : Addressing Dominich's comments. #Patch Set 3 : Changes based on renaming recommendations. #
Total comments: 4
Patch Set 4 : Addressing Dominich's comments. #Patch Set 5 : ResourcePrefetchPredictor can only be enabled at startup. #
Total comments: 2
Patch Set 6 : Disabling resource prefetch predictor on OffTheRecord profile. #Patch Set 7 : Rebasing and minor fix. #Patch Set 8 : Fixing issue with histograms not initializing correctly. #
Total comments: 2
Patch Set 9 : Adding macros to avoid having to use STATIC_HISTOGRAM_POINTER_BLOCK. #
Total comments: 2
Patch Set 10 : Addressing Matt's comments. #
Total comments: 3
Messages
Total messages: 34 (0 generated)
PTAL. mmenke for chrome/browser/net/... changes.
https://chromiumcodereview.appspot.com/10900037/diff/1/chrome/browser/net/res... File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): https://chromiumcodereview.appspot.com/10900037/diff/1/chrome/browser/net/res... chrome/browser/net/resource_prefetch_predictor_observer.cc:40: UMA_HISTOGRAM_BOOLEAN("ResourcePrefetchPredictor.NoResourceRequestInfo", you can also use HISTOGRAM_ENUMERATION with the parameters 1, 2. consider this instead: UMA_HISTOGRAM_BOOLEAN("ResourcePrefetchPredictor.ResourceRequestInfo", info); https://chromiumcodereview.appspot.com/10900037/diff/1/chrome/browser/net/res... chrome/browser/net/resource_prefetch_predictor_observer.cc:47: UMA_HISTOGRAM_BOOLEAN( as above https://chromiumcodereview.appspot.com/10900037/diff/1/chrome/browser/net/res... chrome/browser/net/resource_prefetch_predictor_observer.cc:48: "ResourcePrefetchPredictor.NoRenderViewIdFromrResourceRequestInfo", typo: FromrResource ? https://chromiumcodereview.appspot.com/10900037/diff/1/chrome/browser/predict... File chrome/browser/predictors/autocomplete_action_predictor_table.cc (right): https://chromiumcodereview.appspot.com/10900037/diff/1/chrome/browser/predict... chrome/browser/predictors/autocomplete_action_predictor_table.cc:246: void AutocompleteActionPredictorTable::LogDatabaseStats(Profile* profile) { why are you passing this in? https://chromiumcodereview.appspot.com/10900037/diff/1/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://chromiumcodereview.appspot.com/10900037/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:798: ++from_network; if it was cached, it's not from the network, no? https://chromiumcodereview.appspot.com/10900037/diff/1/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right): https://chromiumcodereview.appspot.com/10900037/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:251: if (!ResourcePrefetchPredictor::IsEnabled(profile)) you could have it always enabled for data tracking, but not acting on that data. that would mean you wouldn't need this and you'd get more data .
https://chromiumcodereview.appspot.com/10900037/diff/1/chrome/browser/net/res... File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): https://chromiumcodereview.appspot.com/10900037/diff/1/chrome/browser/net/res... chrome/browser/net/resource_prefetch_predictor_observer.cc:40: UMA_HISTOGRAM_BOOLEAN("ResourcePrefetchPredictor.NoResourceRequestInfo", On 2012/08/29 22:29:29, dominich wrote: > you can also use HISTOGRAM_ENUMERATION with the parameters 1, 2. > > consider this instead: > > UMA_HISTOGRAM_BOOLEAN("ResourcePrefetchPredictor.ResourceRequestInfo", info); Since this corner case happens rarely (I assume), I would like to report only the failed case. There is no point reporting the other, just seems wasteful. https://chromiumcodereview.appspot.com/10900037/diff/1/chrome/browser/net/res... chrome/browser/net/resource_prefetch_predictor_observer.cc:47: UMA_HISTOGRAM_BOOLEAN( On 2012/08/29 22:29:29, dominich wrote: > as above as above :) https://chromiumcodereview.appspot.com/10900037/diff/1/chrome/browser/net/res... chrome/browser/net/resource_prefetch_predictor_observer.cc:48: "ResourcePrefetchPredictor.NoRenderViewIdFromrResourceRequestInfo", On 2012/08/29 22:29:29, dominich wrote: > typo: FromrResource ? Fixed. https://chromiumcodereview.appspot.com/10900037/diff/1/chrome/browser/predict... File chrome/browser/predictors/autocomplete_action_predictor_table.cc (right): https://chromiumcodereview.appspot.com/10900037/diff/1/chrome/browser/predict... chrome/browser/predictors/autocomplete_action_predictor_table.cc:246: void AutocompleteActionPredictorTable::LogDatabaseStats(Profile* profile) { On 2012/08/29 22:29:29, dominich wrote: > why are you passing this in? Used to determine if prefreshing is enabled. https://chromiumcodereview.appspot.com/10900037/diff/1/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://chromiumcodereview.appspot.com/10900037/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:798: ++from_network; On 2012/08/29 22:29:29, dominich wrote: > if it was cached, it's not from the network, no? eeps! Thanks for catching this. https://chromiumcodereview.appspot.com/10900037/diff/1/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right): https://chromiumcodereview.appspot.com/10900037/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:251: if (!ResourcePrefetchPredictor::IsEnabled(profile)) On 2012/08/29 22:29:29, dominich wrote: > you could have it always enabled for data tracking, but not acting on that data. > that would mean you wouldn't need this and you'd get more data . The problem is that I get a lot of empty databases, and I am not sure why. This will remove some of the cases. If you meant have it enabled for learning only mode, this change does that.
https://chromiumcodereview.appspot.com/10900037/diff/1/chrome/browser/net/res... File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): https://chromiumcodereview.appspot.com/10900037/diff/1/chrome/browser/net/res... chrome/browser/net/resource_prefetch_predictor_observer.cc:40: UMA_HISTOGRAM_BOOLEAN("ResourcePrefetchPredictor.NoResourceRequestInfo", On 2012/08/29 22:51:06, Shishir wrote: > On 2012/08/29 22:29:29, dominich wrote: > > you can also use HISTOGRAM_ENUMERATION with the parameters 1, 2. > > > > consider this instead: > > > > UMA_HISTOGRAM_BOOLEAN("ResourcePrefetchPredictor.ResourceRequestInfo", info); > > Since this corner case happens rarely (I assume), I would like to report only > the failed case. There is no point reporting the other, just seems wasteful. but without that how do you know if it's a significant %age or a small %age? I think you need both to know how often it occurs. https://chromiumcodereview.appspot.com/10900037/diff/1/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right): https://chromiumcodereview.appspot.com/10900037/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:251: if (!ResourcePrefetchPredictor::IsEnabled(profile)) On 2012/08/29 22:51:06, Shishir wrote: > On 2012/08/29 22:29:29, dominich wrote: > > you could have it always enabled for data tracking, but not acting on that > data. > > that would mean you wouldn't need this and you'd get more data . > > The problem is that I get a lot of empty databases, and I am not sure why. This > will remove some of the cases. > > If you meant have it enabled for learning only mode, this change does that. Instead of passing down the profile, can you call this method only if it's enabled?
lgtm See my comments re: histograms.xml, this may require renaming some of the histograms here (adding underscores).
PTAL. http://chromiumcodereview.appspot.com/10900037/diff/1/chrome/browser/net/reso... File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): http://chromiumcodereview.appspot.com/10900037/diff/1/chrome/browser/net/reso... chrome/browser/net/resource_prefetch_predictor_observer.cc:40: UMA_HISTOGRAM_BOOLEAN("ResourcePrefetchPredictor.NoResourceRequestInfo", On 2012/08/29 23:16:49, dominich wrote: > On 2012/08/29 22:51:06, Shishir wrote: > > On 2012/08/29 22:29:29, dominich wrote: > > > you can also use HISTOGRAM_ENUMERATION with the parameters 1, 2. > > > > > > consider this instead: > > > > > > UMA_HISTOGRAM_BOOLEAN("ResourcePrefetchPredictor.ResourceRequestInfo", > info); > > > > Since this corner case happens rarely (I assume), I would like to report only > > the failed case. There is no point reporting the other, just seems wasteful. > > but without that how do you know if it's a significant %age or a small %age? I > think you need both to know how often it occurs. Redid this so we have base. http://chromiumcodereview.appspot.com/10900037/diff/1/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right): http://chromiumcodereview.appspot.com/10900037/diff/1/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:251: if (!ResourcePrefetchPredictor::IsEnabled(profile)) On 2012/08/29 23:16:49, dominich wrote: > On 2012/08/29 22:51:06, Shishir wrote: > > On 2012/08/29 22:29:29, dominich wrote: > > > you could have it always enabled for data tracking, but not acting on that > > data. > > > that would mean you wouldn't need this and you'd get more data . > > > > The problem is that I get a lot of empty databases, and I am not sure why. > This > > will remove some of the cases. > > > > If you meant have it enabled for learning only mode, this change does that. > > Instead of passing down the profile, can you call this method only if it's > enabled? I could do that, but the PredictorDatabase know about specific features seemed like a bad idea. Let me know if you still want me to move this.
I'm not familiar with this code, and have been pretty busy with reviews this week, so I defer to Dominic and Timo.
http://codereview.chromium.org/10900037/diff/12001/chrome/browser/predictors/... File chrome/browser/predictors/predictor_database.cc (right): http://codereview.chromium.org/10900037/diff/12001/chrome/browser/predictors/... chrome/browser/predictors/predictor_database.cc:50: Profile* const profile_; caching the profile pointer is rarely the right thing to do. http://codereview.chromium.org/10900037/diff/12001/chrome/browser/predictors/... chrome/browser/predictors/predictor_database.cc:104: resource_prefetch_tables_->LogDatabaseStats(profile_); given you know what table you're calling this on, i don't see why it shouldn't check for the feature to be enabled before calling it. It's certainly better than caching and passing down the profile pointer (which might be null/shutting down/etc)
http://chromiumcodereview.appspot.com/10900037/diff/12001/chrome/browser/pred... File chrome/browser/predictors/predictor_database.cc (right): http://chromiumcodereview.appspot.com/10900037/diff/12001/chrome/browser/pred... chrome/browser/predictors/predictor_database.cc:50: Profile* const profile_; On 2012/08/30 16:07:31, dominich wrote: > caching the profile pointer is rarely the right thing to do. Doesn't this make sense for a profile keyed service though? The service should be shut down before the profile is destroyed. http://chromiumcodereview.appspot.com/10900037/diff/12001/chrome/browser/pred... chrome/browser/predictors/predictor_database.cc:104: resource_prefetch_tables_->LogDatabaseStats(profile_); On 2012/08/30 16:07:31, dominich wrote: > given you know what table you're calling this on, i don't see why it shouldn't > check for the feature to be enabled before calling it. It's certainly better > than caching and passing down the profile pointer (which might be null/shutting > down/etc) Done.
PTAL
lgtm https://chromiumcodereview.appspot.com/10900037/diff/15007/chrome/browser/pre... File chrome/browser/prerender/prerender_field_trial.cc (left): https://chromiumcodereview.appspot.com/10900037/diff/15007/chrome/browser/pre... chrome/browser/prerender/prerender_field_trial.cc:278: if (!profile) this might mean that tests with NULL profiles will be entered into a field trial. I'm not sure if the FieldTrial code already checks that for tests, or if you're manually setting the commandline switch for all tests to disabled, but it's something to be aware of.
https://chromiumcodereview.appspot.com/10900037/diff/15007/chrome/browser/pre... File chrome/browser/prerender/prerender_field_trial.cc (left): https://chromiumcodereview.appspot.com/10900037/diff/15007/chrome/browser/pre... chrome/browser/prerender/prerender_field_trial.cc:278: if (!profile) On 2012/08/30 18:25:23, dominich wrote: > this might mean that tests with NULL profiles will be entered into a field > trial. I'm not sure if the FieldTrial code already checks that for tests, or if > you're manually setting the commandline switch for all tests to disabled, but > it's something to be aware of. Digging a little deeper, I need to disable this when the profile is off the record. This could be the reason I am seeing so many empty databases. So I think I will have to put this profile check back in.
PTAL.
lgtm
lgtm
lgtm
PTAL.
https://chromiumcodereview.appspot.com/10900037/diff/15009/chrome/browser/pre... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://chromiumcodereview.appspot.com/10900037/diff/15009/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:820: UMA_HISTOGRAM_COUNTS( if you define a macro that takes 25 or 50 as a parameter you should be able to simplify all of this. See the prerender_histograms for how to call histogram macros with various parameters.
https://chromiumcodereview.appspot.com/10900037/diff/15009/chrome/browser/pre... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://chromiumcodereview.appspot.com/10900037/diff/15009/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:820: UMA_HISTOGRAM_COUNTS( On 2012/08/31 20:40:45, dominich wrote: > if you define a macro that takes 25 or 50 as a parameter you should be able to > simplify all of this. See the prerender_histograms for how to call histogram > macros with various parameters. Wrote macros not to use the static initializer which also works. It is a bit more inefficient, so if you think I should writes ones that take the number as a param instead, let me know.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shishir@chromium.org/10900037/27003
Presubmit check for 10900037-27003 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome/browser/net
Presubmit checks took 1.1s to calculate.
Matt, can you look at this? Needs approval for chrome/browser/net still
browser/net LGTM. http://codereview.chromium.org/10900037/diff/27003/chrome/browser/net/resourc... File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): http://codereview.chromium.org/10900037/diff/27003/chrome/browser/net/resourc... chrome/browser/net/resource_prefetch_predictor_observer.cc:54: UMA_HISTOGRAM_ENUMERATION("ResourcePrefetchPredictor.RequestStats", Rather than duplicate these two strings all over the place, I'd suggest one wrapper function for each.
http://codereview.chromium.org/10900037/diff/27003/chrome/browser/net/resourc... File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): http://codereview.chromium.org/10900037/diff/27003/chrome/browser/net/resourc... chrome/browser/net/resource_prefetch_predictor_observer.cc:54: UMA_HISTOGRAM_ENUMERATION("ResourcePrefetchPredictor.RequestStats", On 2012/09/04 17:19:46, Matt Menke wrote: > Rather than duplicate these two strings all over the place, I'd suggest one > wrapper function for each. This would be hard to do efficiently. The histogram macro uses a static pointer which is initialized only once, so to create a function with a single macro call is not possible. I can do this by not using the macro and not using a static pointer, but that would be pretty inefficient since this counter gets called for every resource request, response etc.
On 2012/09/04 17:30:55, Shishir wrote: > http://codereview.chromium.org/10900037/diff/27003/chrome/browser/net/resourc... > File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): > > http://codereview.chromium.org/10900037/diff/27003/chrome/browser/net/resourc... > chrome/browser/net/resource_prefetch_predictor_observer.cc:54: > UMA_HISTOGRAM_ENUMERATION("ResourcePrefetchPredictor.RequestStats", > On 2012/09/04 17:19:46, Matt Menke wrote: > > Rather than duplicate these two strings all over the place, I'd suggest one > > wrapper function for each. > > This would be hard to do efficiently. The histogram macro uses a static pointer > which is initialized only once, so to create a function with a single macro > call is not possible. I can do this by not using the macro and not using a > static pointer, but that would be pretty inefficient since this counter gets > called for every resource request, response etc. "One wrapper function for each" != "A single wrapper function for both"
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shishir@chromium.org/10900037/27003
On 2012/09/04 17:32:32, Matt Menke wrote: > On 2012/09/04 17:30:55, Shishir wrote: > > > http://codereview.chromium.org/10900037/diff/27003/chrome/browser/net/resourc... > > File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): > > > > > http://codereview.chromium.org/10900037/diff/27003/chrome/browser/net/resourc... > > chrome/browser/net/resource_prefetch_predictor_observer.cc:54: > > UMA_HISTOGRAM_ENUMERATION("ResourcePrefetchPredictor.RequestStats", > > On 2012/09/04 17:19:46, Matt Menke wrote: > > > Rather than duplicate these two strings all over the place, I'd suggest one > > > wrapper function for each. > > > > This would be hard to do efficiently. The histogram macro uses a static > pointer > > which is initialized only once, so to create a function with a single macro > > call is not possible. I can do this by not using the macro and not using a > > static pointer, but that would be pretty inefficient since this counter gets > > called for every resource request, response etc. > > "One wrapper function for each" != "A single wrapper function for both" I read that wrong and somehow I am not getting mails for you replies. Checked the commit bot by mistake and I dont think I can revert this. Let me send you separate CL for that.
Apparently the commit bot can be stopped. PTAL.
Just what I had had in mind, LGTM. With copying strings all over the file, there's a potential issue of typos or using the wrong combination of values (Logging a main frame result for the other histogram, for instance), and we also have to go through the static variable initialization a bunch of times - the CPU overhead is minimal, but not sure about the overhead in terms of code length. http://codereview.chromium.org/10900037/diff/20004/chrome/browser/net/resourc... File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): http://codereview.chromium.org/10900037/diff/20004/chrome/browser/net/resourc... chrome/browser/net/resource_prefetch_predictor_observer.cc:41: void ReportRequestStats(RequestStats stat) { These should be in an anonymous namespace.
http://codereview.chromium.org/10900037/diff/20004/chrome/browser/net/resourc... File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): http://codereview.chromium.org/10900037/diff/20004/chrome/browser/net/resourc... chrome/browser/net/resource_prefetch_predictor_observer.cc:41: void ReportRequestStats(RequestStats stat) { On 2012/09/04 18:14:01, Matt Menke wrote: > These should be in an anonymous namespace. It already is :)
http://codereview.chromium.org/10900037/diff/20004/chrome/browser/net/resourc... File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): http://codereview.chromium.org/10900037/diff/20004/chrome/browser/net/resourc... chrome/browser/net/resource_prefetch_predictor_observer.cc:41: void ReportRequestStats(RequestStats stat) { On 2012/09/04 19:15:19, Shishir wrote: > On 2012/09/04 18:14:01, Matt Menke wrote: > > These should be in an anonymous namespace. > > It already is :) Umm...Erm... Of course I knew that! I was just making sure you did, too. :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shishir@chromium.org/10900037/20004
Change committed as 154855 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
