Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6028012:
Fixed RLZTracker::GetAccessPointRlz to route IO calls to the FILE thread. (Closed)
|
Created:
9 years, 11 months ago by pastarmovj Modified:
9 years, 7 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionThe function is now only used with the CHROME_OMNIBOX point and the value is cached and returned from the cache on subsequent calls. All other access points will be always freshly fetched and returned.
Future calls to this function with another access point should either be done on the FILE thread or implement caching the way it is done for Omnibox. Moreover if they implement the caching mechanism they should be prepared for using this function in asynchronous manner if called on another thread.
BUG=62337
TEST=A search from branded browser through the omnibox-the rlz parameter should be appended to the query string.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72337
Patch Set 1 #Patch Set 2 : Cleand up debug code and added a DCHECK for wrong async call. #Patch Set 3 : s/2010/2011/ #
Total comments: 2
Patch Set 4 : Added {} around multi-line block. #
Total comments: 6
Patch Set 5 : Moved the DelayedInitTask to the FILE thread. #
Total comments: 4
Patch Set 6 : Ensured the cached value is there when the flag for that is set. #Patch Set 7 : Added thread safety to the RLZ code. #
Messages
Total messages: 19 (0 generated)
Please review.
adding cpu since I'm no RLZ expert myself http://codereview.chromium.org/6028012/diff/4001/chrome/browser/search_engine... File chrome/browser/search_engines/search_terms_data.cc (right): http://codereview.chromium.org/6028012/diff/4001/chrome/browser/search_engine... chrome/browser/search_engines/search_terms_data.cc:84: !GoogleUpdateSettings::IsOrganic(brand)) nit { ... }
Fixed the nit. Waiting for bot happiness (win*) http://codereview.chromium.org/6028012/diff/4001/chrome/browser/search_engine... File chrome/browser/search_engines/search_terms_data.cc (right): http://codereview.chromium.org/6028012/diff/4001/chrome/browser/search_engine... chrome/browser/search_engines/search_terms_data.cc:84: !GoogleUpdateSettings::IsOrganic(brand)) On 2011/01/07 14:00:32, jochen wrote: > nit { ... } Done.
Please include rogerta@chromium.org, he refactored these. I'll probably miss something. On 2011/01/07 14:06:04, pastarmovj wrote: > Fixed the nit. Waiting for bot happiness (win*) > > http://codereview.chromium.org/6028012/diff/4001/chrome/browser/search_engine... > File chrome/browser/search_engines/search_terms_data.cc (right): > > http://codereview.chromium.org/6028012/diff/4001/chrome/browser/search_engine... > chrome/browser/search_engines/search_terms_data.cc:84: > !GoogleUpdateSettings::IsOrganic(brand)) > On 2011/01/07 14:00:32, jochen wrote: > > nit { ... } > > Done.
@rogerta: Hi, can you please review this CL Carlos suggested you knew yourself best there. Thanks!
lgtm as long as the PSO folks are OK. http://codereview.chromium.org/6028012/diff/10001/chrome/browser/rlz/rlz.cc File chrome/browser/rlz/rlz.cc (right): http://codereview.chromium.org/6028012/diff/10001/chrome/browser/rlz/rlz.cc#n... chrome/browser/rlz/rlz.cc:191: RLZTracker::GetAccessPointRlz(rlz_lib::CHROME_OMNIBOX, &omnibox_rlz); This call may return an empty string, since we're not on the file thread, if this is the first time its called. If the user does not perform at least one omnibox search before the init delay, the code below will try record rlz events. I think its OK though since install events are stateful. http://codereview.chromium.org/6028012/diff/10001/chrome/browser/search_engin... File chrome/browser/search_engines/search_terms_data.cc (right): http://codereview.chromium.org/6028012/diff/10001/chrome/browser/search_engin... chrome/browser/search_engines/search_terms_data.cc:87: // search might not send the RLZ data but this is not really a problem. There will be a new "first time" each time chrome is restarted. But since DelayedInitTask calls GetAccessPointRlz(), that helps reduce the chance of getting an empty string here. Do we have any metrics that tell us how many searches are performed for each invocation of chrome, or time to first search after launch? It would be interesting to know what percentage of searches will not have an rlz parameter due to this change. I would also discuss this with the PSO folks to determine if this is not really a problem.
http://codereview.chromium.org/6028012/diff/10001/chrome/browser/rlz/rlz.cc File chrome/browser/rlz/rlz.cc (right): http://codereview.chromium.org/6028012/diff/10001/chrome/browser/rlz/rlz.cc#n... chrome/browser/rlz/rlz.cc:191: RLZTracker::GetAccessPointRlz(rlz_lib::CHROME_OMNIBOX, &omnibox_rlz); On 2011/01/12 15:46:52, Roger Tawa wrote: > This call may return an empty string, since we're not on the file thread, if > this is the first time its called. If the user does not perform at least one > omnibox search before the init delay, the code below will try record rlz events. > > I think its OK though since install events are stateful. an alternative would be to add a callback parameter to GetAccessPointRlz. If the callback is non-NULL it will be invoked with the correct result. Otherwise, an empty string is returned if no access point is cached.
http://codereview.chromium.org/6028012/diff/10001/chrome/browser/rlz/rlz.cc File chrome/browser/rlz/rlz.cc (right): http://codereview.chromium.org/6028012/diff/10001/chrome/browser/rlz/rlz.cc#n... chrome/browser/rlz/rlz.cc:191: RLZTracker::GetAccessPointRlz(rlz_lib::CHROME_OMNIBOX, &omnibox_rlz); On 2011/01/13 09:25:25, jochen wrote: > On 2011/01/12 15:46:52, Roger Tawa wrote: > > This call may return an empty string, since we're not on the file thread, if > > this is the first time its called. If the user does not perform at least one > > omnibox search before the init delay, the code below will try record rlz > events. > > > > I think its OK though since install events are stateful. > > an alternative would be to add a callback parameter to GetAccessPointRlz. If the > callback is non-NULL it will be invoked with the correct result. Otherwise, an > empty string is returned if no access point is cached. Thinking about it some, I don't think this is a problem. RLZ will take care of not recording this more than once. The code is fine as is.
I guess we are now only waiting for the PSO people blessing. http://codereview.chromium.org/6028012/diff/10001/chrome/browser/rlz/rlz.cc File chrome/browser/rlz/rlz.cc (right): http://codereview.chromium.org/6028012/diff/10001/chrome/browser/rlz/rlz.cc#n... chrome/browser/rlz/rlz.cc:191: RLZTracker::GetAccessPointRlz(rlz_lib::CHROME_OMNIBOX, &omnibox_rlz); On 2011/01/13 15:09:55, Roger Tawa wrote: > On 2011/01/13 09:25:25, jochen wrote: > > On 2011/01/12 15:46:52, Roger Tawa wrote: > > > This call may return an empty string, since we're not on the file thread, if > > > this is the first time its called. If the user does not perform at least > one > > > omnibox search before the init delay, the code below will try record rlz > > events. > > > > > > I think its OK though since install events are stateful. > > > > an alternative would be to add a callback parameter to GetAccessPointRlz. If > the > > callback is non-NULL it will be invoked with the correct result. Otherwise, an > > empty string is returned if no access point is cached. > > Thinking about it some, I don't think this is a problem. RLZ will take care of > not recording this more than once. The code is fine as is. In general I am not happy leaving this function like that because it still does IO on the UI thread but given that it is one time event it shouldn't be so bad. We can do as you propose and add a Callback option to the call or alternatively we could make this call repost itself second time with a slight delay if the GetAccessPointRlz returns false the first time. However RLZ does statefully keep the INSTALL event than this is unnecessary move.
http://codereview.chromium.org/6028012/diff/10001/chrome/browser/rlz/rlz.cc File chrome/browser/rlz/rlz.cc (right): http://codereview.chromium.org/6028012/diff/10001/chrome/browser/rlz/rlz.cc#n... chrome/browser/rlz/rlz.cc:191: RLZTracker::GetAccessPointRlz(rlz_lib::CHROME_OMNIBOX, &omnibox_rlz); On 2011/01/13 15:34:02, pastarmovj wrote: > On 2011/01/13 15:09:55, Roger Tawa wrote: > > On 2011/01/13 09:25:25, jochen wrote: > > > On 2011/01/12 15:46:52, Roger Tawa wrote: > > > > This call may return an empty string, since we're not on the file thread, > if > > > > this is the first time its called. If the user does not perform at least > > one > > > > omnibox search before the init delay, the code below will try record rlz > > > events. > > > > > > > > I think its OK though since install events are stateful. > > > > > > an alternative would be to add a callback parameter to GetAccessPointRlz. If > > the > > > callback is non-NULL it will be invoked with the correct result. Otherwise, > an > > > empty string is returned if no access point is cached. > > > > Thinking about it some, I don't think this is a problem. RLZ will take care > of > > not recording this more than once. The code is fine as is. > > In general I am not happy leaving this function like that because it still does > IO on the UI thread but given that it is one time event it shouldn't be so bad. > We can do as you propose and add a Callback option to the call or alternatively > we could make this call repost itself second time with a slight delay if the > GetAccessPointRlz returns false the first time. However RLZ does statefully keep > the INSTALL event than this is unnecessary move. Is there a need to post DelayedInitTask to the UI thread? Looking at the Run() it does not seem like there is any dependency. Could the DelayedInitTask be posted to the IO thread instead? See line 262 below.
I don't think so. I just cheched all calls this function makes and they all are either not thead specific or need file io. Unfortunately the internet connection on my train is too bad to test moving the call to the FILE thread. Wil do that on monday. On Jan 13, 2011 5:04 PM, <rogerta@chromium.org> wrote: > > > http://codereview.chromium.org/6028012/diff/10001/chrome/browser/rlz/rlz.cc > File chrome/browser/rlz/rlz.cc (right): > > http://codereview.chromium.org/6028012/diff/10001/chrome/browser/rlz/rlz.cc#n... > chrome/browser/rlz/rlz.cc:191: > RLZTracker::GetAccessPointRlz(rlz_lib::CHROME_OMNIBOX, &omnibox_rlz); > On 2011/01/13 15:34:02, pastarmovj wrote: >> >> On 2011/01/13 15:09:55, Roger Tawa wrote: >> > On 2011/01/13 09:25:25, jochen wrote: >> > > On 2011/01/12 15:46:52, Roger Tawa wrote: >> > > > This call may return an empty string, since we're not on the > > file thread, >> >> if >> > > > this is the first time its called. If the user does not perform > > at least >> >> > one >> > > > omnibox search before the init delay, the code below will try > > record rlz >> >> > > events. >> > > > >> > > > I think its OK though since install events are stateful. >> > > >> > > an alternative would be to add a callback parameter to > > GetAccessPointRlz. If >> >> > the >> > > callback is non-NULL it will be invoked with the correct result. > > Otherwise, >> >> an >> > > empty string is returned if no access point is cached. >> > >> > Thinking about it some, I don't think this is a problem. RLZ will > > take care >> >> of >> > not recording this more than once. The code is fine as is. > > >> In general I am not happy leaving this function like that because it > > still does >> >> IO on the UI thread but given that it is one time event it shouldn't > > be so bad. >> >> We can do as you propose and add a Callback option to the call or > > alternatively >> >> we could make this call repost itself second time with a slight delay > > if the >> >> GetAccessPointRlz returns false the first time. However RLZ does > > statefully keep >> >> the INSTALL event than this is unnecessary move. > > > Is there a need to post DelayedInitTask to the UI thread? Looking at > the Run() it does not seem like there is any dependency. Could the > DelayedInitTask be posted to the IO thread instead? See line 262 below. > > > http://codereview.chromium.org/6028012/
Please review. Moved the DelayedInitTask to the FILE thread and it seems that it works just fine from there. Moreover this ensures that the RLS calls inside that function will work as they used to before and removed one more AllowIO override that way too. I think this way we ensure no harm is done to the RLZ tracking.
http://codereview.chromium.org/6028012/diff/23001/chrome/browser/rlz/rlz.cc File chrome/browser/rlz/rlz.cc (right): http://codereview.chromium.org/6028012/diff/23001/chrome/browser/rlz/rlz.cc#n... chrome/browser/rlz/rlz.cc:308: access_values_state = ACCESS_VALUES_FRESH; there is now the possibility that multiple threads will touch the access_values_state and cached_ommibox_rlz variables. Shouldn't there a lock to protect them? (there was a possibility before too but it seems it may happen more often now)
http://codereview.chromium.org/6028012/diff/23001/chrome/browser/rlz/rlz.cc File chrome/browser/rlz/rlz.cc (right): http://codereview.chromium.org/6028012/diff/23001/chrome/browser/rlz/rlz.cc#n... chrome/browser/rlz/rlz.cc:308: access_values_state = ACCESS_VALUES_FRESH; I don't think this is possible. This code is only reached from the FILE thread because of the new addition. The other case this value is written is the PingNow function which is also only called through a task post on the FILE thread because of the last edit where we moved the PostDelayedTask to the file thread instead of the UI where it used to be before. Or am I skipping there something? On 2011/01/18 21:00:14, Roger Tawa wrote: > there is now the possibility that multiple threads will touch the > access_values_state and cached_ommibox_rlz variables. Shouldn't there a lock to > protect them? (there was a possibility before too but it seems it may happen > more often now)
http://codereview.chromium.org/6028012/diff/23001/chrome/browser/rlz/rlz.cc File chrome/browser/rlz/rlz.cc (right): http://codereview.chromium.org/6028012/diff/23001/chrome/browser/rlz/rlz.cc#n... chrome/browser/rlz/rlz.cc:308: access_values_state = ACCESS_VALUES_FRESH; On 2011/01/18 21:10:20, pastarmovj wrote: > I don't think this is possible. This code is only reached from the FILE thread > because of the new addition. The other case this value is written is the PingNow > function which is also only called through a task post on the FILE thread > because of the last edit where we moved the PostDelayedTask to the file thread > instead of the UI where it used to be before. > Or am I skipping there something? > > On 2011/01/18 21:00:14, Roger Tawa wrote: > > there is now the possibility that multiple threads will touch the > > access_values_state and cached_ommibox_rlz variables. Shouldn't there a lock > to > > protect them? (there was a possibility before too but it seems it may happen > > more often now) > The ui thread can access access_values_state and cached_ommibox_rlz at line 280 above, no? so its possible for the file thread (or pingnow thread) to be modifying one or both values while the ui thread is trying to read them.
I don't think concurrent access is that bad. Bad was that the cached value was set before the actual cache was populated and I fixed the order there. It used to be irrelevant before. http://codereview.chromium.org/6028012/diff/23001/chrome/browser/rlz/rlz.cc File chrome/browser/rlz/rlz.cc (right): http://codereview.chromium.org/6028012/diff/23001/chrome/browser/rlz/rlz.cc#n... chrome/browser/rlz/rlz.cc:308: access_values_state = ACCESS_VALUES_FRESH; Ah you meant between read and write access in the same function. Sure this can happen. Actually I'd only swap lines 308/309 as we can safely assume that the int assignment is atomic so we only want to allow reading of the cache value _after_ it has been initialized. On 2011/01/19 01:57:52, Roger Tawa wrote: > On 2011/01/18 21:10:20, pastarmovj wrote: > > I don't think this is possible. This code is only reached from the FILE thread > > because of the new addition. The other case this value is written is the > PingNow > > function which is also only called through a task post on the FILE thread > > because of the last edit where we moved the PostDelayedTask to the file thread > > instead of the UI where it used to be before. > > Or am I skipping there something? > > > > On 2011/01/18 21:00:14, Roger Tawa wrote: > > > there is now the possibility that multiple threads will touch the > > > access_values_state and cached_ommibox_rlz variables. Shouldn't there a > lock > > to > > > protect them? (there was a possibility before too but it seems it may > happen > > > more often now) > > > > The ui thread can access access_values_state and cached_ommibox_rlz at line 280 > above, no? so its possible for the file thread (or pingnow thread) to be > modifying one or both values while the ui thread is trying to read them.
Hi Julian, I believe that your assumption about this being safe is incorrect. The read/writes will not necessarily happen in the order that they appear in the code, see this article for more details: http://msdn.microsoft.com/en-us/library/ms686355(v=vs.85).aspx I believe you need to use atomic operations (see base/atomicops.h) or a mutex.
On 2011/01/20 07:04:55, Roger Tawa wrote: > Hi Julian, > > I believe that your assumption about this being safe is incorrect. The > read/writes will not necessarily happen in the order that they appear in the > code, see this article for more details: > > http://msdn.microsoft.com/en-us/library/ms686355%28v=vs.85%29.aspx > > I believe you need to use atomic operations (see base/atomicops.h) or a mutex. Sorry about that you are right. Please have a look at my proposed mutex solution implemented with a base::AutoLock.
lgtm Awesome Julian! Thanks for making the changes. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
