Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16514008:
Add metrics for calculating precision/recall of link navigation pre-connect triggers. (Closed)
|
Created:
7 years, 6 months ago by kouhei (in TOK) Modified:
7 years, 4 months ago Reviewers:
cbentzel, jochen (gone - plz use gerrit), jar (doing other things), Ilya Sherman, mmenke, sky CC:
chromium-reviews, cbentzel+watch_chromium.org, haraken, tburkard Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd metrics for calculating precision/recall of link navigation pre-connect triggers.
This patch adds the UMA "Net.PreconnectTriggerUsed", and add the UMA "Net.PreconnectedLinkNavigation".
PredictorTabHelper collects requests information for user link navigation. This information is sent to class Predictor::PreconnectUsage to see if the navigation has possibly used a preconnect session from {mouse,touch}-event triggers.
The UMA "Net.PreconnectTriggerUsed" records whether if a preconnect trigger is followed by a resource request (from link navigations) to the host or not. This is to measure precision of link-based preconnect triggers.
The UMA "Net.PreconnectedLinkNavigation" records whether if a resource request from link navigations is preceded by a preconnect trigger. This is to measure recall of link-based preconnect triggers.
BUG=235361
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215858
Patch Set 1 #
Total comments: 4
Patch Set 2 : WIP #Patch Set 3 : #Patch Set 4 : fix comments / remove leftover pritnfs #
Total comments: 10
Patch Set 5 : nits #
Total comments: 6
Patch Set 6 : PostTask Browser thr / add histograms.xml #
Total comments: 2
Patch Set 7 : fix comments / add ptr check #Patch Set 8 : reverted change to ResourceRequestDetails, and use WebContentsObserver instead of Notifications. #Patch Set 9 : rebase / comments update only #Patch Set 10 : styling only (no logic change) #
Total comments: 25
Patch Set 11 : style fix #Patch Set 12 : PostTask explicitly from caller #Patch Set 13 : fix BrowserCommandTests. #
Total comments: 4
Patch Set 14 : #Patch Set 15 : Uppercase used #
Total comments: 2
Patch Set 16 : use UrlInfo::MOUSE_OVER_MOTIVATED #Patch Set 17 : styles #Patch Set 18 : remove PRERENDER flag change #Messages
Total messages: 41 (0 generated)
jar: Would you take a look?
If this does proceed, please also include the change to src/tools/metrics/histograms.xml https://codereview.chromium.org/16514008/diff/1/chrome/browser/net/predictor.cc File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/16514008/diff/1/chrome/browser/net/predictor.... chrome/browser/net/predictor.cc:847: const GURL& url, const GURL& first_party_for_cookies, nit: one arg per line for definitions and declarations. If you clean up this line, you might want to do the same to other instances in this file. I think the coding style evolved (in a positive direction), and at a minimum you should clean up any method/function that you piddle with. https://codereview.chromium.org/16514008/diff/1/chrome/browser/net/predictor.... chrome/browser/net/predictor.cc:853: recent_preconnects_.WasRecentlySeen(canonical_url)); Why is this an interesting stat? I expect that connection pooling will disregard any extraneous or repeated preconnect requests, so why do we care if we request preconnects more than once (in a short(?) period of time?
Sorry, I will rework this patch. https://codereview.chromium.org/16514008/diff/1/chrome/browser/net/predictor.cc File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/16514008/diff/1/chrome/browser/net/predictor.... chrome/browser/net/predictor.cc:847: const GURL& url, const GURL& first_party_for_cookies, On 2013/06/10 03:17:16, jar wrote: > nit: one arg per line for definitions and declarations. > > If you clean up this line, you might want to do the same to other instances in > this file. > > I think the coding style evolved (in a positive direction), and at a minimum you > should clean up any method/function that you piddle with. Ack.
Updated patch. jar, mmenke: Would you review chrome/browser/net/predictor_*.*? I will split the other parts of the patch if it looks ok in general. https://codereview.chromium.org/16514008/diff/1/chrome/browser/net/predictor.cc File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/16514008/diff/1/chrome/browser/net/predictor.... chrome/browser/net/predictor.cc:847: const GURL& url, const GURL& first_party_for_cookies, On 2013/06/10 23:24:05, kouhei wrote: > On 2013/06/10 03:17:16, jar wrote: > > nit: one arg per line for definitions and declarations. > > > > If you clean up this line, you might want to do the same to other instances in > > this file. > > > > I think the coding style evolved (in a positive direction), and at a minimum > you > > should clean up any method/function that you piddle with. > > Ack. I would like to address this in other CL.
ping. jar, mmenke: Would you review chrome/browser/net/predictor*.*? On 2013/06/14 16:49:42, kouhei wrote: > Updated patch. > > jar, mmenke: Would you review chrome/browser/net/predictor_*.*? > > I will split the other parts of the patch if it looks ok in general. > > https://codereview.chromium.org/16514008/diff/1/chrome/browser/net/predictor.cc > File chrome/browser/net/predictor.cc (right): > > https://codereview.chromium.org/16514008/diff/1/chrome/browser/net/predictor.... > chrome/browser/net/predictor.cc:847: const GURL& url, const GURL& > first_party_for_cookies, > On 2013/06/10 23:24:05, kouhei wrote: > > On 2013/06/10 03:17:16, jar wrote: > > > nit: one arg per line for definitions and declarations. > > > > > > If you clean up this line, you might want to do the same to other instances > in > > > this file. > > > > > > I think the coding style evolved (in a positive direction), and at a minimum > > you > > > should clean up any method/function that you piddle with. > > > > Ack. > > I would like to address this in other CL.
I'm not sure which part you wanted reviewed by me... so this mostly became a drive-by review. https://codereview.chromium.org/16514008/diff/12001/chrome/browser/net/predic... File chrome/browser/net/predictor_tab_helper.cc (right): https://codereview.chromium.org/16514008/diff/12001/chrome/browser/net/predic... chrome/browser/net/predictor_tab_helper.cc:66: break; nit: break inside the curlie. Same on line 75 https://codereview.chromium.org/16514008/diff/12001/chrome/browser/prerender/... File chrome/browser/prerender/prerender_contents.cc (right): https://codereview.chromium.org/16514008/diff/12001/chrome/browser/prerender/... chrome/browser/prerender/prerender_contents.cc:332: content::PAGE_TRANSITION_PRERENDER); nit: please use a temp variable for this argument to make this readable. There is some justification for using the ?: to assign an initial value... but by the time you pass it into a function... this gets to be far less readable. https://codereview.chromium.org/16514008/diff/12001/content/browser/loader/re... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/16514008/diff/12001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_impl.cc:748: GetCertID(loader->request(), info->GetChildID()), nit: one arg per line when you can't even wrap at the paren. https://codereview.chromium.org/16514008/diff/12001/content/public/browser/re... File content/public/browser/resource_request_details.cc (right): https://codereview.chromium.org/16514008/diff/12001/content/public/browser/re... content/public/browser/resource_request_details.cc:13: ResourceRequestDetails::ResourceRequestDetails(const net::URLRequest* request, Question: Why isn't this a const ref? generally we pass either pointers, or const refs.... and the latter makes sense here. https://codereview.chromium.org/16514008/diff/12001/content/public/browser/re... File content/public/browser/resource_request_details.h (right): https://codereview.chromium.org/16514008/diff/12001/content/public/browser/re... content/public/browser/resource_request_details.h:51: struct ResourceRedirectDetails : public ResourceRequestDetails { nit: These should probably be classes. By the time you have virtual methods, a class seems much more appropriate. Any justification I'm missing? Alternatively... why do we need this as a derived class (er...um... stuct)?
I'm unfamiliar with the code this touches, and just don't have time to learn it - I'm pretty heavily loaded with reviews. On 2013/06/17 23:14:13, kouhei wrote: > ping. > > jar, mmenke: Would you review chrome/browser/net/predictor*.*?
jar: Thanks for review! cbentzel: Would you take a look or redirect to appropriate reviewer? https://codereview.chromium.org/16514008/diff/12001/chrome/browser/net/predic... File chrome/browser/net/predictor_tab_helper.cc (right): https://codereview.chromium.org/16514008/diff/12001/chrome/browser/net/predic... chrome/browser/net/predictor_tab_helper.cc:66: break; On 2013/06/18 01:33:49, jar wrote: > nit: break inside the curlie. Same on line 75 Done. https://codereview.chromium.org/16514008/diff/12001/chrome/browser/prerender/... File chrome/browser/prerender/prerender_contents.cc (right): https://codereview.chromium.org/16514008/diff/12001/chrome/browser/prerender/... chrome/browser/prerender/prerender_contents.cc:332: content::PAGE_TRANSITION_PRERENDER); On 2013/06/18 01:33:49, jar wrote: > nit: please use a temp variable for this argument to make this readable. There > is some justification for using the ?: to assign an initial value... but by the > time you pass it into a function... this gets to be far less readable. Done. https://codereview.chromium.org/16514008/diff/12001/content/browser/loader/re... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/16514008/diff/12001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_impl.cc:748: GetCertID(loader->request(), info->GetChildID()), On 2013/06/18 01:33:49, jar wrote: > nit: one arg per line when you can't even wrap at the paren. Done. https://codereview.chromium.org/16514008/diff/12001/content/public/browser/re... File content/public/browser/resource_request_details.cc (right): https://codereview.chromium.org/16514008/diff/12001/content/public/browser/re... content/public/browser/resource_request_details.cc:13: ResourceRequestDetails::ResourceRequestDetails(const net::URLRequest* request, On 2013/06/18 01:33:49, jar wrote: > Question: Why isn't this a const ref? generally we pass either pointers, or > const refs.... and the latter makes sense here. I'd like to address this in separate CL. https://codereview.chromium.org/16514008/diff/12001/content/public/browser/re... File content/public/browser/resource_request_details.h (right): https://codereview.chromium.org/16514008/diff/12001/content/public/browser/re... content/public/browser/resource_request_details.h:51: struct ResourceRedirectDetails : public ResourceRequestDetails { On 2013/06/18 01:33:49, jar wrote: > nit: These should probably be classes. By the time you have virtual methods, a > class seems much more appropriate. > > Any justification I'm missing? > > Alternatively... why do we need this as a derived class (er...um... stuct)? I'd like to address this in separate CL.
Comments addressed. mmenke, cbentzel: Would you take a look or recommend another reviewer?
On 2013/06/21 02:51:32, kouhei wrote: > Comments addressed. > > mmenke, cbentzel: Would you take a look or recommend another reviewer? I will look tonight. Sorry for the super long delays.
Can you update histograms.xml with the new UMA? Also, it would help to describe the new data being recorded in the changelist description. I spent quite some time even trying to understand what this CL was trying to accomplish - it would be better to make it clear in the CL description itself.
I can dig some more into the code, but I am a bit confused about why we need to record these stats in the first place. In my mind the major thing we want to record for preconnects are whether the sockets created are actually used by a request, and the time saved from that. I'm not sure why we need to look specifically at whether it is used for a navigation. https://codereview.chromium.org/16514008/diff/30001/chrome/browser/net/predic... File chrome/browser/net/predictor.h (right): https://codereview.chromium.org/16514008/diff/30001/chrome/browser/net/predic... chrome/browser/net/predictor.h:245: void RecordPreconnectTriggerOnIOThread(const GURL& url); This should be in private - not a part of the interface. https://codereview.chromium.org/16514008/diff/30001/chrome/browser/net/predic... File chrome/browser/net/predictor_tab_helper.cc (right): https://codereview.chromium.org/16514008/diff/30001/chrome/browser/net/predic... chrome/browser/net/predictor_tab_helper.cc:88: profile->GetNetworkPredictor()->RecordPreconnectNavigationStat( This will be called on the UI thread but RecordPreconnectNavigationStat expects to be called on the IO thread. https://codereview.chromium.org/16514008/diff/30001/chrome/browser/renderer_h... File chrome/browser/renderer_host/chrome_render_message_filter.cc (right): https://codereview.chromium.org/16514008/diff/30001/chrome/browser/renderer_h... chrome/browser/renderer_host/chrome_render_message_filter.cc:339: profile_->GetNetworkPredictor()->RecordPreconnectTrigger(url); Could this move into PreconnectUrlAndSubresources? Why is it not being called in the ChromeRenderViewHostObserver::Navigate case?
On 2013/06/24 01:46:46, cbentzel wrote: > I can dig some more into the code, but I am a bit confused about why we need to > record these stats in the first place. > > In my mind the major thing we want to record for preconnects are whether the > sockets created are actually used by a request, and the time saved from that. > > I'm not sure why we need to look specifically at whether it is used for a > navigation. > > https://codereview.chromium.org/16514008/diff/30001/chrome/browser/net/predic... > File chrome/browser/net/predictor.h (right): > > https://codereview.chromium.org/16514008/diff/30001/chrome/browser/net/predic... > chrome/browser/net/predictor.h:245: void RecordPreconnectTriggerOnIOThread(const > GURL& url); > This should be in private - not a part of the interface. > > https://codereview.chromium.org/16514008/diff/30001/chrome/browser/net/predic... > File chrome/browser/net/predictor_tab_helper.cc (right): > > https://codereview.chromium.org/16514008/diff/30001/chrome/browser/net/predic... > chrome/browser/net/predictor_tab_helper.cc:88: > profile->GetNetworkPredictor()->RecordPreconnectNavigationStat( > This will be called on the UI thread but RecordPreconnectNavigationStat expects > to be called on the IO thread. > > https://codereview.chromium.org/16514008/diff/30001/chrome/browser/renderer_h... > File chrome/browser/renderer_host/chrome_render_message_filter.cc (right): > > https://codereview.chromium.org/16514008/diff/30001/chrome/browser/renderer_h... > chrome/browser/renderer_host/chrome_render_message_filter.cc:339: > profile_->GetNetworkPredictor()->RecordPreconnectTrigger(url); > Could this move into PreconnectUrlAndSubresources? Why is it not being called in > the ChromeRenderViewHostObserver::Navigate case? The preconnect socket usage is already available as Net.PreconnectUtilization2, so I will also check that between control/experiment groups. I would like to use these new navigation metrics to evaluate triggers. I wanted the numbers directly related to the mouse-event triggers as much as possible and not affected by preconnect internals. Maybe their names should be changed to MouseEventPredictionPrecision/Recall. At first, I tried to use mouseclick rate to measure those, but as I mentioned in other emails, it was biased when click loggers were used in a web page. I first tried to look at preconnect socket level, but it seemed too low-level to use it to evaluate mouse-event triggers. What makes it difficult is that the method PreconnectUrlAndSubresources creates multiple sessions. Even if only one of the sessions is used, we would like to treat that as a "hit" for a mouse-event trigger.
Thank you for your comments. > Can you update histograms.xml with the new UMA? Done. > Also, it would help to describe the new data being recorded in the changelist > description. I spent quite some time even trying to understand what this CL was > trying to accomplish - it would be better to make it clear in the CL description > itself. Done. https://codereview.chromium.org/16514008/diff/30001/chrome/browser/net/predic... File chrome/browser/net/predictor.h (right): https://codereview.chromium.org/16514008/diff/30001/chrome/browser/net/predic... chrome/browser/net/predictor.h:245: void RecordPreconnectTriggerOnIOThread(const GURL& url); On 2013/06/24 01:46:46, cbentzel wrote: > This should be in private - not a part of the interface. Done. https://codereview.chromium.org/16514008/diff/30001/chrome/browser/net/predic... File chrome/browser/net/predictor_tab_helper.cc (right): https://codereview.chromium.org/16514008/diff/30001/chrome/browser/net/predic... chrome/browser/net/predictor_tab_helper.cc:88: profile->GetNetworkPredictor()->RecordPreconnectNavigationStat( On 2013/06/24 01:46:46, cbentzel wrote: > This will be called on the UI thread but RecordPreconnectNavigationStat expects > to be called on the IO thread. Done. https://codereview.chromium.org/16514008/diff/30001/chrome/browser/renderer_h... File chrome/browser/renderer_host/chrome_render_message_filter.cc (right): https://codereview.chromium.org/16514008/diff/30001/chrome/browser/renderer_h... chrome/browser/renderer_host/chrome_render_message_filter.cc:339: profile_->GetNetworkPredictor()->RecordPreconnectTrigger(url); On 2013/06/24 01:46:46, cbentzel wrote: > Could this move into PreconnectUrlAndSubresources? Why is it not being called in > the ChromeRenderViewHostObserver::Navigate case? ChromeRenderViewHostObserver::Navigate seems to be called when a user types on address bar. Our target is preconnect signals for link navigations.
Thanks for the additional discussion and code review comments. This makes it more clear why you want the metrics. I am still not fully convinced that gathering them is worth the additional mechanics, and will try to think of alternate approaches that are not quite as complex. It's quite possible that your approach is the simplest solution. https://codereview.chromium.org/16514008/diff/35002/chrome/browser/net/predic... File chrome/browser/net/predictor.h (right): https://codereview.chromium.org/16514008/diff/35002/chrome/browser/net/predic... chrome/browser/net/predictor.h:236: // May be called from either the IO or UI thread and will PostTask Is this comment correct? The name of it seems to indicate that it must be called on the IO thread.
https://codereview.chromium.org/16514008/diff/35002/chrome/browser/net/predic... File chrome/browser/net/predictor.h (right): https://codereview.chromium.org/16514008/diff/35002/chrome/browser/net/predic... chrome/browser/net/predictor.h:236: // May be called from either the IO or UI thread and will PostTask On 2013/06/24 10:48:39, cbentzel wrote: > Is this comment correct? The name of it seems to indicate that it must be called > on the IO thread. Sorry I put this by mistake. Thank you for pointing it out.
On 2013/06/24 10:48:39, cbentzel wrote: > Thanks for the additional discussion and code review comments. This makes it > more clear why you want the metrics. I am still not fully convinced that > gathering them is worth the additional mechanics, and will try to think of > alternate approaches that are not quite as complex. It's quite possible that > your approach is the simplest solution. cbentzel: Hi. Have you come up with a good idea? :)
On 2013/07/01 01:21:46, kouhei wrote: > On 2013/06/24 10:48:39, cbentzel wrote: > > Thanks for the additional discussion and code review comments. This makes it > > more clear why you want the metrics. I am still not fully convinced that > > gathering them is worth the additional mechanics, and will try to think of > > alternate approaches that are not quite as complex. It's quite possible that > > your approach is the simplest solution. > > cbentzel: Hi. Have you come up with a good idea? :) My apologies - I am on a two week vacation. If I do not get back to you over the weekend than let's proceed with this.
On 2013/07/05 15:43:47, cbentzel wrote: > On 2013/07/01 01:21:46, kouhei wrote: > > On 2013/06/24 10:48:39, cbentzel wrote: > > > Thanks for the additional discussion and code review comments. This makes it > > > more clear why you want the metrics. I am still not fully convinced that > > > gathering them is worth the additional mechanics, and will try to think of > > > alternate approaches that are not quite as complex. It's quite possible that > > > your approach is the simplest solution. > > > > cbentzel: Hi. Have you come up with a good idea? :) > > My apologies - I am on a two week vacation. If I do not get back to you over the > weekend than let's proceed with this. OK, finally back on this. I think jar should be primary reviewer as he knows the Predictor best. Let's add another UMA stat rather than change the meaning of Net.PreconnectedNavigation. Also - rather than change ResourceRequestDetails I think it would be better to have a WebContentsObserver which overrides DidNavigateMainFrame or possible DidCommitProvisionalLoad. Originally I was considering just having this detect main frame navigations in the ConnectInterceptor by identifying main frame URLRequests which have a referrer from a page which triggers mouse-based preconnects, but I think the WebContentsObserver will be better and not as drastic a change as what you suggest.
Thanks for your comments! > OK, finally back on this. > > I think jar should be primary reviewer as he knows the Predictor best. > > Let's add another UMA stat rather than change the meaning of > Net.PreconnectedNavigation. Ack. > Also - rather than change ResourceRequestDetails I think it would be better to > have a WebContentsObserver which overrides DidNavigateMainFrame or possible > DidCommitProvisionalLoad. Originally I was considering just having this detect > main frame navigations in the ConnectInterceptor by identifying main frame > URLRequests which have a referrer from a page which triggers mouse-based > preconnects, but I think the WebContentsObserver will be better and not as > drastic a change as what you suggest. Yes, using WebContentsObserver simplifies the code, but I found it unable to track redirects. The reason I used the notification observer and ResourceRequestDetails is that I wanted to track redirect chain. If a preconnected domain was somewhere in the redirect chain, the navigation should be marked as "used preconnect". e.g.) Suppose if user mousedown-ed and clicked on <a href="www.example.com">. This causes preconnect to be triggered against "www.example.com". The actual navigation is redirected from 30x redirected from "www.example.com" to "example.iana.org", so the preconnect session is used. However, DidNavigateMainFrame/DidCommitProvisionalLoad is only triggered for "example.iana.org" I tried to use WebContentsObserver first, and then found ProvisionalChangeToMainFrameUrl which can track redirect is a deprecated API. I agree that current patch is complex and involves too much change, so I'm currently trying to find other ways around, minimizing change to existing code.
Ok. I re-worked the patch. I will think this over again this weekend if this can be simpler. Before submit, I will split some part of this patch as separate CL. I think the PRERENDER load flag part can be split at least. > > Let's add another UMA stat rather than change the meaning of > > Net.PreconnectedNavigation. > Ack. Done. > > Also - rather than change ResourceRequestDetails I think it would be better to > > have a WebContentsObserver which overrides DidNavigateMainFrame or possible > > DidCommitProvisionalLoad. Originally I was considering just having this detect > > main frame navigations in the ConnectInterceptor by identifying main frame > > URLRequests which have a referrer from a page which triggers mouse-based > > preconnects, but I think the WebContentsObserver will be better and not as > > drastic a change as what you suggest. > Yes, using WebContentsObserver simplifies the code, but I found it unable to > track redirects. > > The reason I used the notification observer and ResourceRequestDetails is that I > wanted to track redirect chain. If a preconnected domain was somewhere in the > redirect chain, the navigation should be marked as "used preconnect". > e.g.) Suppose if user mousedown-ed and clicked on <a href="www.example.com">. > This causes preconnect to be triggered against "www.example.com". The actual > navigation is redirected from 30x redirected from "www.example.com" to > "example.iana.org", so the preconnect session is used. However, > DidNavigateMainFrame/DidCommitProvisionalLoad is only triggered for > "example.iana.org" > > I tried to use WebContentsObserver first, and then found > ProvisionalChangeToMainFrameUrl which can track redirect is a deprecated API. > > I agree that current patch is complex and involves too much change, so I'm > currently trying to find other ways around, minimizing change to existing code. In this patch, I used WebContentsObserver::DidNavigateMainFrame to identify link navigations, and used ConnectInterceptor for tracking redirects. Those information are joined in Predictor::PreconnectUsage. I still feel this is too complex, but at least this should be better than the last patch.
On 2013/07/26 10:45:21, kouhei wrote: > Ok. I re-worked the patch. I will think this over again this weekend if this can > be simpler. > Before submit, I will split some part of this patch as separate CL. > I think the PRERENDER load flag part can be split at least. > > > > Let's add another UMA stat rather than change the meaning of > > > Net.PreconnectedNavigation. > > Ack. > Done. > > > > Also - rather than change ResourceRequestDetails I think it would be better > to > > > have a WebContentsObserver which overrides DidNavigateMainFrame or possible > > > DidCommitProvisionalLoad. Originally I was considering just having this > detect > > > main frame navigations in the ConnectInterceptor by identifying main frame > > > URLRequests which have a referrer from a page which triggers mouse-based > > > preconnects, but I think the WebContentsObserver will be better and not as > > > drastic a change as what you suggest. > > Yes, using WebContentsObserver simplifies the code, but I found it unable to > > track redirects. > > > > The reason I used the notification observer and ResourceRequestDetails is that > I > > wanted to track redirect chain. If a preconnected domain was somewhere in the > > redirect chain, the navigation should be marked as "used preconnect". > > e.g.) Suppose if user mousedown-ed and clicked on <a href="www.example.com">. > > This causes preconnect to be triggered against "www.example.com". The actual > > navigation is redirected from 30x redirected from "www.example.com" to > > "example.iana.org", so the preconnect session is used. However, > > DidNavigateMainFrame/DidCommitProvisionalLoad is only triggered for > > "example.iana.org" > > > > I tried to use WebContentsObserver first, and then found > > ProvisionalChangeToMainFrameUrl which can track redirect is a deprecated API. > > > > I agree that current patch is complex and involves too much change, so I'm > > currently trying to find other ways around, minimizing change to existing > code. > In this patch, I used WebContentsObserver::DidNavigateMainFrame to identify link > navigations, and used ConnectInterceptor for tracking redirects. Those > information are joined in Predictor::PreconnectUsage. I still feel this is too > complex, but at least this should be better than the last patch. We use ProvisionalChangeToMainFrameUrl in the prerender code to track redirects. I'll look at why this is being deprecated and what it's being replaced with.
On 2013/07/26 13:19:45, cbentzel wrote: > On 2013/07/26 10:45:21, kouhei wrote: > > Ok. I re-worked the patch. I will think this over again this weekend if this > can > > be simpler. > > Before submit, I will split some part of this patch as separate CL. > > I think the PRERENDER load flag part can be split at least. > > > > > > Let's add another UMA stat rather than change the meaning of > > > > Net.PreconnectedNavigation. > > > Ack. > > Done. > > > > > > Also - rather than change ResourceRequestDetails I think it would be > better > > to > > > > have a WebContentsObserver which overrides DidNavigateMainFrame or > possible > > > > DidCommitProvisionalLoad. Originally I was considering just having this > > detect > > > > main frame navigations in the ConnectInterceptor by identifying main frame > > > > URLRequests which have a referrer from a page which triggers mouse-based > > > > preconnects, but I think the WebContentsObserver will be better and not as > > > > drastic a change as what you suggest. > > > Yes, using WebContentsObserver simplifies the code, but I found it unable to > > > track redirects. > > > > > > The reason I used the notification observer and ResourceRequestDetails is > that > > I > > > wanted to track redirect chain. If a preconnected domain was somewhere in > the > > > redirect chain, the navigation should be marked as "used preconnect". > > > e.g.) Suppose if user mousedown-ed and clicked on <a > href="www.example.com">. > > > This causes preconnect to be triggered against "www.example.com". The actual > > > navigation is redirected from 30x redirected from "www.example.com" to > > > "example.iana.org", so the preconnect session is used. However, > > > DidNavigateMainFrame/DidCommitProvisionalLoad is only triggered for > > > "example.iana.org" > > > > > > I tried to use WebContentsObserver first, and then found > > > ProvisionalChangeToMainFrameUrl which can track redirect is a deprecated > API. > > > > > > I agree that current patch is complex and involves too much change, so I'm > > > currently trying to find other ways around, minimizing change to existing > > code. > > In this patch, I used WebContentsObserver::DidNavigateMainFrame to identify > link > > navigations, and used ConnectInterceptor for tracking redirects. Those > > information are joined in Predictor::PreconnectUsage. I still feel this is too > > complex, but at least this should be better than the last patch. > > We use ProvisionalChangeToMainFrameUrl in the prerender code to track redirects. > I'll look at why this is being deprecated and what it's being replaced with. In chromium/src/content/public/browser/web_contents_observer.h, there is a comment saying using this for tracking redirects is deprecated. https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...
jar: Would you take a look again? The CL has updated since your last review. Thanks in advance. Particularly, 1) PredictorTabHelper is now a content::WebContentsObserver instead of NotificationObserver, and 2) Predictor::PreconnectUsage doesn't manage LRU for recent navigations, but use URLRequest::url_chain() from ConnectInterceptor to track redirects. > Ok. I re-worked the patch. I will think this over again this weekend if this can > be simpler. I didn't come up with a better idea, so I would like to proceed with this patch. > Before submit, I will split some part of this patch as separate CL. > I think the PRERENDER load flag part can be split at least. The PRERENDER transition_type change was split as http://crrev.com/20989002
> jar: Would you take a look again? The CL has updated since your last review. > Thanks in advance. > Particularly, 1) PredictorTabHelper is now a content::WebContentsObserver > instead of NotificationObserver, and 2) Predictor::PreconnectUsage doesn't > manage LRU for recent navigations, but use URLRequest::url_chain() from > ConnectInterceptor to track redirects. jar: I would appreciate it if you would take a look. Thanks!
On 2013/08/01 01:37:57, kouhei wrote: > > jar: Would you take a look again? The CL has updated since your last review. > > Thanks in advance. > > Particularly, 1) PredictorTabHelper is now a content::WebContentsObserver > > instead of NotificationObserver, and 2) Predictor::PreconnectUsage doesn't > > manage LRU for recent navigations, but use URLRequest::url_chain() from > > ConnectInterceptor to track redirects. > > jar: I would appreciate it if you would take a look. Thanks! As this UMA analysis need 6 weeks to gather reliable data in dev/beta, so I want this patch to be merged within a week to get the results by the end of this quarter. I would really appreciate it if you would review this patch before your OOO. Thanks! :)
More stats, and understanding of how well this works (or doesn't work) is very good. So LGTM with nits. That said, please consider the first comment below, as it IMO represents the real path to better preconnection (and better stats for how well we do to help things). https://chromiumcodereview.appspot.com/16514008/diff/70005/chrome/browser/net... File chrome/browser/net/predictor.cc (right): https://chromiumcodereview.appspot.com/16514008/diff/70005/chrome/browser/net... chrome/browser/net/predictor.cc:130: // This records UMAs for preconnect usage based on navigation URLs to FWIW: The long standing bug in this algorithm is that we monitor navigations, but we don't monitor socket (connection) utilization count. As a result, if a top level resource sequentially(!) needs 6 subresources, then it is incorrectly tagged as as needing 6 preconnects. This comments suggests that we're really not addressing that issue... which may greatly improve preconnection hit rate, and diminish the count of wasteful connections (that never get used). https://chromiumcodereview.appspot.com/16514008/diff/70005/chrome/browser/net... chrome/browser/net/predictor.cc:132: // Stats are gathered via a LRU cach that remembers all preconnect within the nit: cache-->cache https://chromiumcodereview.appspot.com/16514008/diff/70005/chrome/browser/net... chrome/browser/net/predictor.cc:135: // access to the preconnected host occurs within a time period specified by Per comment: We count access (still), and not simultaneous connection use :-/ https://chromiumcodereview.appspot.com/16514008/diff/70005/chrome/browser/net... chrome/browser/net/predictor.cc:193: // Evict any overly old entries and record stats nit: Period at end of sentences. https://chromiumcodereview.appspot.com/16514008/diff/70005/chrome/browser/net... chrome/browser/net/predictor.cc:208: // Add new entry nit: period https://chromiumcodereview.appspot.com/16514008/diff/70005/chrome/browser/net... chrome/browser/net/predictor.cc:214: const std::vector<GURL>& url_chain, bool is_subresource) { nit: one arg per line. https://chromiumcodereview.appspot.com/16514008/diff/70005/chrome/browser/net... chrome/browser/net/predictor.cc:1011: const std::vector<GURL>& url_chain, bool is_subresource) { nit: one arg per line. https://chromiumcodereview.appspot.com/16514008/diff/70005/chrome/browser/net... File chrome/browser/net/predictor.h (right): https://chromiumcodereview.appspot.com/16514008/diff/70005/chrome/browser/net... chrome/browser/net/predictor.h:241: // May be called from either the IO or UI thread and will PostTask nit: (personal?) I think it is a bad pattern to protect methods with a callback to the right thread if you happen to be on the wrong thread when calling. I guess you could argue that you compact some amount of code (the post-task call), but I think that folks should know which thread they are on, and not be blind to the implications of such calls. I'll ask others... but my personal preference is to avoid this pattern. If you really have a bunch of folks calling Foo() from both the IO thread and from the UI thread (and want to save code?), you *could* have a FooFromUIThread() vs a FooFromIOThread(). That would at least hint to the reader that you might be on the wrong thread.. and this call might need to be posted etc. https://chromiumcodereview.appspot.com/16514008/diff/70005/chrome/browser/net... chrome/browser/net/predictor.h:248: // May be called from either the IO or UI thread and will PostTask nit: same comment on pattern as above. https://chromiumcodereview.appspot.com/16514008/diff/70005/chrome/browser/net... chrome/browser/net/predictor.h:467: void RecordLinkNavigationOnIOThread(const GURL& url); So you have this method... it is just IMO nicer to expose it, let callers use their knowledge to "call the right flavor," and then DCHECK that it was done correctly. This also means we don't waste time doing the check in production code needlessly. https://chromiumcodereview.appspot.com/16514008/diff/70005/chrome/browser/net... File chrome/browser/net/predictor_tab_helper.cc (right): https://chromiumcodereview.appspot.com/16514008/diff/70005/chrome/browser/net... chrome/browser/net/predictor_tab_helper.cc:23: // is currently not supported nit: Start with upper case letter... and with period. https://chromiumcodereview.appspot.com/16514008/diff/70005/chrome/browser/net... chrome/browser/net/predictor_tab_helper.cc:34: content::WebContents* web_contents) nit: no need to wrap https://chromiumcodereview.appspot.com/16514008/diff/70005/chrome/browser/pre... File chrome/browser/prerender/prerender_contents.cc (right): https://chromiumcodereview.appspot.com/16514008/diff/70005/chrome/browser/pre... chrome/browser/prerender/prerender_contents.cc:342: content::PAGE_TRANSITION_TYPED : content::PAGE_TRANSITION_LINK; nit: The old format was bad... your format is better... more readable may be: content::PageTransition transition_type_no_qualifier = origin_ == ORIGIN_OMNIBOX ? content::PAGE_TRANSITION_TYPED : content::PAGE_TRANSITION_LINK;
Thanks for review! Comments inline. https://codereview.chromium.org/16514008/diff/70005/chrome/browser/net/predic... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/16514008/diff/70005/chrome/browser/net/predic... chrome/browser/net/predictor.cc:130: // This records UMAs for preconnect usage based on navigation URLs to On 2013/08/01 22:56:47, jar (OOO 8-3...8-14-2013) wrote: > FWIW: The long standing bug in this algorithm is that we monitor navigations, > but we don't monitor socket (connection) utilization count. > > As a result, if a top level resource sequentially(!) needs 6 subresources, then > it is incorrectly tagged as as needing 6 preconnects. This comments suggests > that we're really not addressing that issue... which may greatly improve > preconnection hit rate, and diminish the count of wasteful connections (that > never get used). My understanding is that per-socket utilization count is measured at Net.PreconnectUtilization2. https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/stream_... I wanted to specifically measure navigations-oriented precision/recall here, as my intent is to study and improve accuracy of the navigation based preconnect triggers. The problem of sequentially accessing subresources is very interesting, may be we can improve Predictor::LearnFromNavigation? Although I would do it in separate CL. https://codereview.chromium.org/16514008/diff/70005/chrome/browser/net/predic... chrome/browser/net/predictor.cc:132: // Stats are gathered via a LRU cach that remembers all preconnect within the On 2013/08/01 22:56:47, jar (OOO 8-3...8-14-2013) wrote: > nit: cache-->cache Done. https://codereview.chromium.org/16514008/diff/70005/chrome/browser/net/predic... chrome/browser/net/predictor.cc:193: // Evict any overly old entries and record stats On 2013/08/01 22:56:47, jar (OOO 8-3...8-14-2013) wrote: > nit: Period at end of sentences. Done. https://codereview.chromium.org/16514008/diff/70005/chrome/browser/net/predic... chrome/browser/net/predictor.cc:208: // Add new entry On 2013/08/01 22:56:47, jar (OOO 8-3...8-14-2013) wrote: > nit: period Done. https://codereview.chromium.org/16514008/diff/70005/chrome/browser/net/predic... chrome/browser/net/predictor.cc:214: const std::vector<GURL>& url_chain, bool is_subresource) { On 2013/08/01 22:56:47, jar (OOO 8-3...8-14-2013) wrote: > nit: one arg per line. Done. https://codereview.chromium.org/16514008/diff/70005/chrome/browser/net/predic... chrome/browser/net/predictor.cc:1011: const std::vector<GURL>& url_chain, bool is_subresource) { On 2013/08/01 22:56:47, jar (OOO 8-3...8-14-2013) wrote: > nit: one arg per line. Done. https://codereview.chromium.org/16514008/diff/70005/chrome/browser/net/predic... File chrome/browser/net/predictor.h (right): https://codereview.chromium.org/16514008/diff/70005/chrome/browser/net/predic... chrome/browser/net/predictor.h:241: // May be called from either the IO or UI thread and will PostTask On 2013/08/01 22:56:47, jar (OOO 8-3...8-14-2013) wrote: > nit: (personal?) I think it is a bad pattern to protect methods with a callback > to the right thread if you happen to be on the wrong thread when calling. > > I guess you could argue that you compact some amount of code (the post-task > call), but I think that folks should know which thread they are on, and not be > blind to the implications of such calls. > > I'll ask others... but my personal preference is to avoid this pattern. > > If you really have a bunch of folks calling Foo() from both the IO thread and > from the UI thread (and want to save code?), you *could* have a > FooFromUIThread() vs a FooFromIOThread(). That would at least hint to the > reader that you might be on the wrong thread.. and this call might need to be > posted etc. I changed so that callers PostTask to IOThread. https://codereview.chromium.org/16514008/diff/70005/chrome/browser/net/predic... chrome/browser/net/predictor.h:248: // May be called from either the IO or UI thread and will PostTask On 2013/08/01 22:56:47, jar (OOO 8-3...8-14-2013) wrote: > nit: same comment on pattern as above. Done. https://codereview.chromium.org/16514008/diff/70005/chrome/browser/net/predic... chrome/browser/net/predictor.h:467: void RecordLinkNavigationOnIOThread(const GURL& url); On 2013/08/01 22:56:47, jar (OOO 8-3...8-14-2013) wrote: > So you have this method... it is just IMO nicer to expose it, let callers use > their knowledge to "call the right flavor," and then DCHECK that it was done > correctly. This also means we don't waste time doing the check in production > code needlessly. Done. https://codereview.chromium.org/16514008/diff/70005/chrome/browser/net/predic... File chrome/browser/net/predictor_tab_helper.cc (right): https://codereview.chromium.org/16514008/diff/70005/chrome/browser/net/predic... chrome/browser/net/predictor_tab_helper.cc:23: // is currently not supported On 2013/08/01 22:56:47, jar (OOO 8-3...8-14-2013) wrote: > nit: Start with upper case letter... and with period. Done. https://codereview.chromium.org/16514008/diff/70005/chrome/browser/net/predic... chrome/browser/net/predictor_tab_helper.cc:34: content::WebContents* web_contents) On 2013/08/01 22:56:47, jar (OOO 8-3...8-14-2013) wrote: > nit: no need to wrap Done. https://codereview.chromium.org/16514008/diff/70005/chrome/browser/prerender/... File chrome/browser/prerender/prerender_contents.cc (right): https://codereview.chromium.org/16514008/diff/70005/chrome/browser/prerender/... chrome/browser/prerender/prerender_contents.cc:342: content::PAGE_TRANSITION_TYPED : content::PAGE_TRANSITION_LINK; On 2013/08/01 22:56:47, jar (OOO 8-3...8-14-2013) wrote: > nit: The old format was bad... your format is better... more readable may be: > > content::PageTransition transition_type_no_qualifier = > origin_ == ORIGIN_OMNIBOX ? content::PAGE_TRANSITION_TYPED > : content::PAGE_TRANSITION_LINK; Done.
sky: Would you take a look at chrome/browser/renderer_host/chrome_render_message_filter.cc and chrome_browser/ui/browser_tab_contents.cc? isherman: Would you take a look at histograms.xml?
https://codereview.chromium.org/16514008/diff/92001/tools/metrics/histograms/... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/16514008/diff/92001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:6581: <histogram name="Net.PreconnectTriggerused" enum="PreconnectTriggerUsed"> nit: "used" -> "Used" https://codereview.chromium.org/16514008/diff/92001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:20113: <int value="1" label="Page nav. preceded by a pre-connect"/> Hmm, are you sure that this enum was previously incorrect?
Thanks for your comments! sky: Would you take a look at chrome/browser/renderer_host/chrome_render_message_filter.cc and chrome_browser/ui/browser_tab_contents.cc? isherman: Would you take a look at histograms.xml? https://codereview.chromium.org/16514008/diff/92001/tools/metrics/histograms/... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/16514008/diff/92001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:6581: <histogram name="Net.PreconnectTriggerused" enum="PreconnectTriggerUsed"> On 2013/08/02 23:11:58, Ilya Sherman wrote: > nit: "used" -> "Used" Done. https://codereview.chromium.org/16514008/diff/92001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:20113: <int value="1" label="Page nav. preceded by a pre-connect"/> On 2013/08/02 23:11:58, Ilya Sherman wrote: > Hmm, are you sure that this enum was previously incorrect? Yes, this was opposite. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/net...
browser_tab_contents.cc LGTM I'm not a good owner for chrome_render_message_filter.cc though.
Thanks for review! jochen: Would you take a look at chrome_render_message_filter.cc? isherman: Would you take a look at histograms.xml again?
https://codereview.chromium.org/16514008/diff/105001/chrome/browser/renderer_... File chrome/browser/renderer_host/chrome_render_message_filter.cc (right): https://codereview.chromium.org/16514008/diff/105001/chrome/browser/renderer_... chrome/browser/renderer_host/chrome_render_message_filter.cc:202: BrowserThread::PostTask( wouldn't it be more stable if you did called RecordPreconnectTrigger inside PreconnectUrlOnIOThread depending on the motivation parameter?
jochen: Updated patch. Would you take a look? https://codereview.chromium.org/16514008/diff/105001/chrome/browser/renderer_... File chrome/browser/renderer_host/chrome_render_message_filter.cc (right): https://codereview.chromium.org/16514008/diff/105001/chrome/browser/renderer_... chrome/browser/renderer_host/chrome_render_message_filter.cc:202: BrowserThread::PostTask( On 2013/08/06 01:23:09, jochen wrote: > wouldn't it be more stable if you did called RecordPreconnectTrigger inside > PreconnectUrlOnIOThread depending on the motivation parameter? Done. Pass UrlInfo::MOUSE_OVER_MOTIVATED as motivation.
lgtm
Thanks for review! isherman: Would you take a look at histograms.xml again?
histograms lgtm
On 2013/08/06 06:44:03, Ilya Sherman (Away Aug. 9-25) wrote: > histograms lgtm Thanks for review!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kouhei@chromium.org/16514008/122001
Message was sent while issue was closed.
Change committed as 215858 |
