Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11522009:
X-Chrome-Variations logic refactoring (Closed)
|
Created:
8 years ago by Bart N. Modified:
8 years ago Reviewers:
Peter Kasting, sky, Mark P, SteveT, Use pkasting(at)chromium.org, Steve, Ben Goodger (Google) CC:
chromium-reviews, James Su, darin-cc_chromium.org Visibility:
Public. |
DescriptionX-Chrome-Variations refactoring.
Caveats:
(1) I had to add a lock since both AC and RDH run on different threads
(2) Because the cache is lazily initialized, it's possible that a few AC requests will run without headers
(3) SP doesn't have access to ResourceContext and in turn we can't get ProfileIOData, so we can't transmit "UMA is enabled" header
(4) I used a singleton
BUG=163999
TEST=hand tested by adding LOG(INFO) and checking headers sent on autocomplete/search requests
Also, ran tcpdump and observed transmitted headers.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173740
Patch Set 1 #
Total comments: 36
Patch Set 2 : Addressed comments. #
Total comments: 6
Patch Set 3 : Renamed SuggestFieldTrial and updated exp IDs range. #
Total comments: 14
Patch Set 4 : #Patch Set 5 : Addressed all remaining comments. #
Total comments: 2
Patch Set 6 : #Patch Set 7 : Addressed Steve's comments. #Patch Set 8 : Merged with head. #
Total comments: 12
Messages
Total messages: 54 (0 generated)
The initial implementation is ready for review. I've tested it by building a binary and putting LOG(INFO) statement. Please see all caveats listed in the description of this CL.
It seems like all the complexity of multithreading, locking, and a singleton is needed because you're trying to cache the result of calling GetActiveFieldTrialGroups() and converting it to a string. Is this really necessary? Could we just call that function every time and convert? If so it seems like we can dramatically simplify things. Also, that function is threadsafe, right? Because it looks as if you call it from multiple threads. https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... File chrome/browser/chrome_metrics_helper.cc (right): https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.cc:20: using content::BrowserThread; Nit: Normally we avoid using directives unless they save a significant number of lines; here it seems like you could eliminate both of these. https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.cc:26: ChromeMetricsHelper::ChromeMetricsHelper() Nit: Function definition order must match declaration order. https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.cc:33: void ChromeMetricsHelper::AppendHeadersToRequest( Nit: Based on the implementations of these two functions, it seems like this class' API could just expose AppendHeaders() directly, and leave it up to callers to provide an appropriate HttpRequestHeaders* and other data. This would mean this class wouldn't have to know anything about URLFetchers or URLRequests, which seems like a win to me. https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.cc:36: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); You may need to add a call to IsWellKnownThread() here so you don't break in tests that don't name particular threads -- see similar checks at other places in our codebase. https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.cc:92: void ChromeMetricsHelper::OnFieldTrialGroupFinalized( Is this supposed to be called exclusively on one particular thread? If so add a DCHECK to that effect. If not, add comments as to what threads we could see this on and why. (This is actually good advice for all these functions when your class is multithreaded.) https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.cc:130: void ChromeMetricsHelper::UpdateVariationIDsHeaderValue() { Nit: While it's technically not necessary since both the callers do this, I suggest taking the lock here. This is harmless and will keep you safe if you change the callers. https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.cc:140: if (variation_ids_set_.size() > 20) { You cannot handle DCHECK failure. A DCHECK means "this cannot ever happen even in the face of a hostile server". I suspect what you want here is to simply leave the conditional and remove the DCHECK. It's also not clear where you're picking these values from or why the right response is to clear entirely rather than clamping. Some commentary would help. https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.cc:161: DVLOG(1) << "Failed to base64 encode Variation IDs value: " << serialized; Is logging really what you want here? Note that it's often hard to collect good logs from clients. If Base64Encode() should never ever fail, you could just DCHECK and proceed unconditionally. https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... File chrome/browser/chrome_metrics_helper.h (right): https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.h:46: // base::FieldTrialList::Observer implementation. Nit: If this isn't called directly through a ChromeMetricsHelper*, can you make it private? This will keep the public API for the class clearer. https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.h:56: friend struct DefaultSingletonTraits<ChromeMetricsHelper>; Nit: Put friend declaration above constructor/destructor, then a blank line. https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.h:58: void AppendHeaders(const GURL& url, Nit: Comment? https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.h:76: // Whether or not we've initialized the Cache. Nit: Cache -> cache https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.h:82: // thread, we do not need to synchronize its uses. I'm confused as to what this comment means. It sounds like it's saying "all access to these variables is on the same thread and thus unlocked" but the comments on the lock say that we use it to protect these. ??
I left a few comments up to Steve to answer, as I'm not entirely sure about the existing code (some things I already addressed). Re: caching - again, I don't know about the cost. My guess is that it is non-trivial, otherwise it wouldn't be cached, but let's see what Steve says. https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... File chrome/browser/chrome_metrics_helper.cc (right): https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.cc:20: using content::BrowserThread; On 2012/12/11 21:08:48, Peter Kasting wrote: > Nit: Normally we avoid using directives unless they save a significant number of > lines; here it seems like you could eliminate both of these. Done for AutoLock, however the existing BrowserThread makes actually the code more readable, especially after you asked me to add IsWellKnownThread. https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.cc:26: ChromeMetricsHelper::ChromeMetricsHelper() On 2012/12/11 21:08:48, Peter Kasting wrote: > Nit: Function definition order must match declaration order. Done. https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.cc:33: void ChromeMetricsHelper::AppendHeadersToRequest( On 2012/12/11 21:08:48, Peter Kasting wrote: > Nit: Based on the implementations of these two functions, it seems like this > class' API could just expose AppendHeaders() directly, and leave it up to > callers to provide an appropriate HttpRequestHeaders* and other data. This > would mean this class wouldn't have to know anything about URLFetchers or > URLRequests, which seems like a win to me. I think it's a good suggestion. I made this class as close to the original implementation as possible (to avoid too much shuffling), but if Steve has no objections, I'd also prefer to get rid of the other 2 methods and leave it up to the caller. https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.cc:36: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); On 2012/12/11 21:08:48, Peter Kasting wrote: > You may need to add a call to IsWellKnownThread() here so you don't break in > tests that don't name particular threads -- see similar checks at other places > in our codebase. Good to know. Anyway, this the old code. Done. https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.cc:92: void ChromeMetricsHelper::OnFieldTrialGroupFinalized( On 2012/12/11 21:08:48, Peter Kasting wrote: > Is this supposed to be called exclusively on one particular thread? If so add a > DCHECK to that effect. If not, add comments as to what threads we could see > this on and why. (This is actually good advice for all these functions when > your class is multithreaded.) I think it can be called only from the IO thread (Steve, can you confirm?). One thing I've noticed is that the existing implementation calls base::FieldTrialList::AddObserver(this) from InitVariationIDsCacheIfNeeded which in turn may be called from different threads. https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.cc:130: void ChromeMetricsHelper::UpdateVariationIDsHeaderValue() { On 2012/12/11 21:08:48, Peter Kasting wrote: > Nit: While it's technically not necessary since both the callers do this, I > suggest taking the lock here. This is harmless and will keep you safe if you > change the callers. Doesn't it incur extra CPU cost? This method is private, so I'm not sure if I buy your argument. https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.cc:140: if (variation_ids_set_.size() > 20) { On 2012/12/11 21:08:48, Peter Kasting wrote: > You cannot handle DCHECK failure. A DCHECK means "this cannot ever happen even > in the face of a hostile server". I suspect what you want here is to simply > leave the conditional and remove the DCHECK. > > It's also not clear where you're picking these values from or why the right > response is to clear entirely rather than clamping. Some commentary would help. I'll leave it up to Steve to answer this question; most of the code here is simply moved from another class. https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.cc:161: DVLOG(1) << "Failed to base64 encode Variation IDs value: " << serialized; On 2012/12/11 21:08:48, Peter Kasting wrote: > Is logging really what you want here? Note that it's often hard to collect good > logs from clients. > > If Base64Encode() should never ever fail, you could just DCHECK and proceed > unconditionally. Steve? https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... File chrome/browser/chrome_metrics_helper.h (right): https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.h:46: // base::FieldTrialList::Observer implementation. On 2012/12/11 21:08:48, Peter Kasting wrote: > Nit: If this isn't called directly through a ChromeMetricsHelper*, can you make > it private? This will keep the public API for the class clearer. Sure. https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.h:56: friend struct DefaultSingletonTraits<ChromeMetricsHelper>; On 2012/12/11 21:08:48, Peter Kasting wrote: > Nit: Put friend declaration above constructor/destructor, then a blank line. Done. https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.h:76: // Whether or not we've initialized the Cache. On 2012/12/11 21:08:48, Peter Kasting wrote: > Nit: Cache -> cache Done. https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.h:82: // thread, we do not need to synchronize its uses. On 2012/12/11 21:08:48, Peter Kasting wrote: > I'm confused as to what this comment means. It sounds like it's saying "all > access to these variables is on the same thread and thus unlocked" but the > comments on the lock say that we use it to protect these. ?? Ah, most of the code in this file is simply moved (no changes) from the existing implementation. I forgot to update this comment, which is no longer true. Done.
https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... File chrome/browser/chrome_metrics_helper.cc (right): https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.cc:130: void ChromeMetricsHelper::UpdateVariationIDsHeaderValue() { On 2012/12/11 23:18:56, Bart N. wrote: > On 2012/12/11 21:08:48, Peter Kasting wrote: > > Nit: While it's technically not necessary since both the callers do this, I > > suggest taking the lock here. This is harmless and will keep you safe if you > > change the callers. > Doesn't it incur extra CPU cost? This method is private, so I'm not sure if I > buy your argument. I don't know about the cost, but I can't imagine it's material; this is getting called occasionally as opposed to tens of thousands of time in some loop. You're right that privacy helps -- we're not going to add outside-class callers, unless we expose this publicly -- but even so, other within-class functions could change in the future. It seems like a small price to pay for clarity and safety. Another possibility would be to instead DCHECK that the lock is already held on entry. That's just as safe and cheaper, at the cost of causing debug-only runtime failures that might not be noticed if someone ever updates the code in a way that breaks things.
Per in-person discussion with Bart, he's going to add to this change: * change the name of the omnibox search suggest experiment (so can easily identify users who are running with this change vs. running the older code). (Slicing by precise release version doesn't cut it due to constraints with our analysis tools.) * claim a new block of twenty experiment IDs for this experiment (same reason: to distinguish users running with this change from users without it). --mark On 2012/12/12 00:01:06, Peter Kasting wrote: > https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... > File chrome/browser/chrome_metrics_helper.cc (right): > > https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... > chrome/browser/chrome_metrics_helper.cc:130: void > ChromeMetricsHelper::UpdateVariationIDsHeaderValue() { > On 2012/12/11 23:18:56, Bart N. wrote: > > On 2012/12/11 21:08:48, Peter Kasting wrote: > > > Nit: While it's technically not necessary since both the callers do this, I > > > suggest taking the lock here. This is harmless and will keep you safe if > you > > > change the callers. > > Doesn't it incur extra CPU cost? This method is private, so I'm not sure if I > > buy your argument. > > I don't know about the cost, but I can't imagine it's material; this is getting > called occasionally as opposed to tens of thousands of time in some loop. > > You're right that privacy helps -- we're not going to add outside-class callers, > unless we expose this publicly -- but even so, other within-class functions > could change in the future. It seems like a small price to pay for clarity and > safety. > > Another possibility would be to instead DCHECK that the lock is already held on > entry. That's just as safe and cheaper, at the cost of causing debug-only > runtime failures that might not be noticed if someone ever updates the code in a > way that breaks things.
https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... File chrome/browser/chrome_metrics_helper.cc (right): https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.cc:130: void ChromeMetricsHelper::UpdateVariationIDsHeaderValue() { On 2012/12/12 00:01:06, Peter Kasting wrote: > On 2012/12/11 23:18:56, Bart N. wrote: > > On 2012/12/11 21:08:48, Peter Kasting wrote: > > > Nit: While it's technically not necessary since both the callers do this, I > > > suggest taking the lock here. This is harmless and will keep you safe if > you > > > change the callers. > > Doesn't it incur extra CPU cost? This method is private, so I'm not sure if I > > buy your argument. > > I don't know about the cost, but I can't imagine it's material; this is getting > called occasionally as opposed to tens of thousands of time in some loop. OK. Two things: (1) The cost of acquiring a lock is low, but it is definitely non-zero and should be avoided if possible, especially in cases like this (2) I'm not sure if you are aware of that, but by default locks are not recursive; I didn't mention it in first place, since Chrome uses Lock class (which I don't know about). I checked the implementation and in fact, the locks are non-recursive. To put it simple, you are asking me to introduce a deadlock. Is that what you want? > You're right that privacy helps -- we're not going to add outside-class callers, > unless we expose this publicly -- but even so, other within-class functions > could change in the future. It seems like a small price to pay for clarity and > safety. > > Another possibility would be to instead DCHECK that the lock is already held on > entry. That's just as safe and cheaper, at the cost of causing debug-only > runtime failures that might not be noticed if someone ever updates the code in a > way that breaks things. This suggestion actually makes sense, although you probably meant to a call to: lock_.AssertAcquired() rather than a DCHECK, right?
On 2012/12/12 02:06:17, Bart N. wrote: > (1) The cost of acquiring a lock is low, but it is definitely non-zero and > should be avoided if possible, especially in cases like this For reference, what does "cases like this" mean? > (2) I'm not sure if you are aware of that, but by default locks are not > recursive; I didn't mention it in first place, since Chrome uses Lock class > (which I don't know about). I checked the implementation and in fact, the locks > are non-recursive. To put it simple, you are asking me to introduce a deadlock. > Is that what you want? Oops, obviously we can't do this then. > > Another possibility would be to instead DCHECK that the lock is already held > on > > entry. That's just as safe and cheaper, at the cost of causing debug-only > > runtime failures that might not be noticed if someone ever updates the code in > a > > way that breaks things. > This suggestion actually makes sense, although you probably meant to a call to: > lock_.AssertAcquired() rather than a DCHECK, right? Sure.
On 2012/12/12 02:13:47, Peter Kasting wrote: > On 2012/12/12 02:06:17, Bart N. wrote: > > (1) The cost of acquiring a lock is low, but it is definitely non-zero and > > should be avoided if possible, especially in cases like this > > For reference, what does "cases like this" mean? In a trivial case where a private method is always called from another method, which already holds onto a lock. By trivial, I mean (a) there is no recursion and extra nesting (b) the flow is simple (no complex branching where a lock might not have been acquired). The "private" part is very important here; you -- as a designer of a class -- control all private methods. By adding extra checks we either don't trust our coding skills or are expecting that the class may be subject to many changes. None of these applies in this case. Having AcquireLock check is fine in this case, although not really necessary. > > > (2) I'm not sure if you are aware of that, but by default locks are not > > recursive; I didn't mention it in first place, since Chrome uses Lock class > > (which I don't know about). I checked the implementation and in fact, the > locks > > are non-recursive. To put it simple, you are asking me to introduce a > deadlock. > > Is that what you want? > > Oops, obviously we can't do this then. > > > > Another possibility would be to instead DCHECK that the lock is already held > > on > > > entry. That's just as safe and cheaper, at the cost of causing debug-only > > > runtime failures that might not be noticed if someone ever updates the code > in > > a > > > way that breaks things. > > This suggestion actually makes sense, although you probably meant to a call > to: > > lock_.AssertAcquired() rather than a DCHECK, right? > > Sure.
Some comments and responses from me. Mostly looking good! One general question: The title of this CL implies that there may be more refactoring that needs to be done. Is the X-Chrome-Variations functionality not fully integrated with SearchProvider after this patch? What could be missing? (sorry about the delay - was completely offline for a couple days due to travel) https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... File chrome/browser/chrome_metrics_helper.cc (right): https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.cc:33: void ChromeMetricsHelper::AppendHeadersToRequest( +1 to this class not knowing about Fetches vs Requests. The implementation may have been a bit specialized to support the RDH, but I think if we're going to refactor it out here, we'll want to simplify the API as much as possible and reduce dependencies. https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.cc:92: void ChromeMetricsHelper::OnFieldTrialGroupFinalized( On 2012/12/11 23:18:56, Bart N. wrote: > On 2012/12/11 21:08:48, Peter Kasting wrote: > > Is this supposed to be called exclusively on one particular thread? If so add > a > > DCHECK to that effect. If not, add comments as to what threads we could see > > this on and why. (This is actually good advice for all these functions when > > your class is multithreaded.) > I think it can be called only from the IO thread (Steve, can you confirm?). > > One thing I've noticed is that the existing implementation calls > base::FieldTrialList::AddObserver(this) from InitVariationIDsCacheIfNeeded which > in turn may be called from different threads. So this method can be called on this class after this class adds itself as an Observer of the FieldTrialList. The FTL knows to call this method on the same thread as the one it was registered on. Back in ChromeResourceDispatchHostDelegate, this would only happen on the IO thread. Today, it appears that it could happen on different threads, given your above comment. Does that make sense? https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.cc:140: if (variation_ids_set_.size() > 20) { Yeah so this was added as a double safety: * The DCHECK is meant to make developers aware that we're seeing more Variation IDs being transmitted than we'd like. * The hard > 20 check is meant to be a defense against bad server payloads that cause the client to transmit too many IDs to Google. I believe 10 and 20 were chosen somewhat arbitrarily. We basically believed that we should probably not have more than 10 such experiments running (though we could re-evaluate that if demand increases). Happy to change this if you think the strategy we chose here is ineffective or wrong. https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.cc:161: DVLOG(1) << "Failed to base64 encode Variation IDs value: " << serialized; Hmm, yeah, I believe we were just trying to follow precedence of what other users of Base64Encode did upon failure... But looking through the codebase today, it appears that most folks LOG(FATAL) or DCHECK/NOTREACHED... we should do the same here. So could you change the DVLOG to NOTREACHED(), here? https://codereview.chromium.org/11522009/diff/6001/chrome/browser/chrome_metr... File chrome/browser/chrome_metrics_helper.h (right): https://codereview.chromium.org/11522009/diff/6001/chrome/browser/chrome_metr... chrome/browser/chrome_metrics_helper.h:32: // This class is a thread-safe singleton. Could you add a brief doc for this class? https://codereview.chromium.org/11522009/diff/6001/chrome/browser/renderer_ho... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/11522009/diff/6001/chrome/browser/renderer_ho... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:34: #include "chrome/common/metrics/variations/variations_util.h" Can you check if this header is still needed? https://codereview.chromium.org/11522009/diff/6001/chrome/browser/renderer_ho... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:35: #include "chrome/common/metrics/proto/chrome_experiments.pb.h" Ditto.
lgtm for the (trivial) changes to the autocomplete side (I know you'll need this OWNER approval in order to check it in.)
Oops, realized I LGTMed this too soon. Ping me again when you've changed the suggest field trial as discussed in comment #5. --mark
On 2012/12/14 15:57:30, Mark P wrote: > Oops, realized I LGTMed this too soon. Ping me again when you've changed the > suggest field trial as discussed in comment #5. Mark, this is done in patch #3. PTAL Steve, I'll send the remaining changes in next hour or two.
minor nits, now properly LGTM from the autocomplete side https://codereview.chromium.org/11522009/diff/16001/chrome/browser/autocomple... File chrome/browser/autocomplete/autocomplete_field_trial.cc (right): https://codereview.chromium.org/11522009/diff/16001/chrome/browser/autocomple... chrome/browser/autocomplete/autocomplete_field_trial.cc:139: "0", 2013, 7, 1, NULL); Let's bump the expiration date a bit too. How about 7 -> 9 https://codereview.chromium.org/11522009/diff/16001/chrome/common/metrics/var... File chrome/common/metrics/variations/variation_ids.h (right): https://codereview.chromium.org/11522009/diff/16001/chrome/common/metrics/var... chrome/common/metrics/variations/variation_ids.h:138: // Name: OmniboxSearchSuggestStarted2013Q1 SuggestStarted -> SuggestTrialStarted Joel pointed out to me I forgot the word Trial in the previous time I claimed variation IDs. Let's add the word Trial here to make this comment correctly match the actual field trial name. https://codereview.chromium.org/11522009/diff/16001/chrome/common/metrics/var... chrome/common/metrics/variations/variation_ids.h:141: // the earlier omnibox suggest field trial in this file because trial -> trials https://codereview.chromium.org/11522009/diff/16001/chrome/common/metrics/var... chrome/common/metrics/variations/variation_ids.h:145: kSuggestTrialStarted2013Q1IDMin = 3310060, Judging by the format of this file, it seems the preference is to not skip blocks of IDs. I also understand that not having round numbers makes it a little harder to mentally translate field trial group numbers to experiment IDs. I'll let Steve decide which feature is more important and therefore whether you should skip numbers here as you do.
https://codereview.chromium.org/11522009/diff/16001/chrome/browser/autocomple... File chrome/browser/autocomplete/autocomplete_field_trial.cc (right): https://codereview.chromium.org/11522009/diff/16001/chrome/browser/autocomple... chrome/browser/autocomplete/autocomplete_field_trial.cc:139: "0", 2013, 7, 1, NULL); On 2012/12/14 16:56:41, Mark P wrote: > Let's bump the expiration date a bit too. How about > 7 -> 9 Done. https://codereview.chromium.org/11522009/diff/16001/chrome/common/metrics/var... File chrome/common/metrics/variations/variation_ids.h (right): https://codereview.chromium.org/11522009/diff/16001/chrome/common/metrics/var... chrome/common/metrics/variations/variation_ids.h:138: // Name: OmniboxSearchSuggestStarted2013Q1 On 2012/12/14 16:56:41, Mark P wrote: > SuggestStarted > -> > SuggestTrialStarted > > Joel pointed out to me I forgot the word Trial in the previous time I claimed > variation IDs. Let's add the word Trial here to make this comment correctly > match the actual field trial name. Done. In other places it seems fine, so I'm changing anything else. https://codereview.chromium.org/11522009/diff/16001/chrome/common/metrics/var... chrome/common/metrics/variations/variation_ids.h:141: // the earlier omnibox suggest field trial in this file because On 2012/12/14 16:56:41, Mark P wrote: > trial > -> > trials Done. https://codereview.chromium.org/11522009/diff/16001/chrome/common/metrics/var... chrome/common/metrics/variations/variation_ids.h:145: kSuggestTrialStarted2013Q1IDMin = 3310060, On 2012/12/14 16:56:41, Mark P wrote: > Judging by the format of this file, it seems the preference is to not skip > blocks of IDs. I also understand that not having round numbers makes it a > little harder to mentally translate field trial group numbers to experiment IDs. > I'll let Steve decide which feature is more important and therefore whether you > should skip numbers here as you do. I'm confused about your comment. I think we are just lucky - I haven't skipped any block, I simply followed the comment below which asked to use kNextID (which was ...060) and increment it accordingly. If there a was block skipped, it must have been decided in the previous change.
https://codereview.chromium.org/11522009/diff/16001/chrome/browser/autocomple... File chrome/browser/autocomplete/autocomplete_field_trial.cc (right): https://codereview.chromium.org/11522009/diff/16001/chrome/browser/autocomple... chrome/browser/autocomplete/autocomplete_field_trial.cc:139: "0", 2013, 7, 1, NULL); On 2012/12/14 17:06:07, Bart N. wrote: > On 2012/12/14 16:56:41, Mark P wrote: > > Let's bump the expiration date a bit too. How about > > 7 -> 9 > > Done. And the word July -> September. (sorry I forgot to mention this) https://codereview.chromium.org/11522009/diff/16001/chrome/common/metrics/var... File chrome/common/metrics/variations/variation_ids.h (right): https://codereview.chromium.org/11522009/diff/16001/chrome/common/metrics/var... chrome/common/metrics/variations/variation_ids.h:145: kSuggestTrialStarted2013Q1IDMin = 3310060, On 2012/12/14 17:06:07, Bart N. wrote: > On 2012/12/14 16:56:41, Mark P wrote: > > Judging by the format of this file, it seems the preference is to not skip > > blocks of IDs. I also understand that not having round numbers makes it a > > little harder to mentally translate field trial group numbers to experiment > IDs. > > I'll let Steve decide which feature is more important and therefore whether > you > > should skip numbers here as you do. > I'm confused about your comment. > > I think we are just lucky - I haven't skipped any block, I simply followed the > comment below which asked to use kNextID (which was ...060) and increment it > accordingly. If there a was block skipped, it must have been decided in the > previous change. Interesting. I didn't notice the kNextId had already skipped some stuff. (Maybe they're reserving nearby ID space?) In that case, this sounds fine to me.
Steve, it's ready for your review. Mark, FYI: I've slightly changed the search provider's change, so it's no longer one line). https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... File chrome/browser/chrome_metrics_helper.cc (right): https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.cc:33: void ChromeMetricsHelper::AppendHeadersToRequest( On 2012/12/13 16:24:08, SteveT wrote: > +1 to this class not knowing about Fetches vs Requests. > > The implementation may have been a bit specialized to support the RDH, but I > think if we're going to refactor it out here, we'll want to simplify the API as > much as possible and reduce dependencies. Done. https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.cc:92: void ChromeMetricsHelper::OnFieldTrialGroupFinalized( On 2012/12/13 16:24:08, SteveT wrote: > On 2012/12/11 23:18:56, Bart N. wrote: > > On 2012/12/11 21:08:48, Peter Kasting wrote: > > > Is this supposed to be called exclusively on one particular thread? If so > add > > a > > > DCHECK to that effect. If not, add comments as to what threads we could see > > > this on and why. (This is actually good advice for all these functions when > > > your class is multithreaded.) > > I think it can be called only from the IO thread (Steve, can you confirm?). > > > > One thing I've noticed is that the existing implementation calls > > base::FieldTrialList::AddObserver(this) from InitVariationIDsCacheIfNeeded > which > > in turn may be called from different threads. > > So this method can be called on this class after this class adds itself as an > Observer of the FieldTrialList. The FTL knows to call this method on the same > thread as the one it was registered on. > > Back in ChromeResourceDispatchHostDelegate, this would only happen on the IO > thread. Today, it appears that it could happen on different threads, given your > above comment. > > Does that make sense? I think so. However, I've realized that the existing and new implementation have a flaw: (1) AddObserver will not register the given observer if the current thread has no MessageLoop (2) AddObserver will be called at most once (because it's guarded by variation_ids_cache_initialized_ (3) If the first call to AddObserver is called on a thread w/o Message loop, we will never receive notifications! In practices, both UI/IO threads have MessageLoops, but I'm wondering if we should a DCHECK (before a call to AddObserver) to make sure that the current thread has MessageLoop. Something like: DCHECK(MessageLoop::current()); Another option is to make AddObserver return whether it registered an observer or not and then DCHECK on it. https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... File chrome/browser/chrome_metrics_helper.h (right): https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.h:58: void AppendHeaders(const GURL& url, On 2012/12/11 21:08:48, Peter Kasting wrote: > Nit: Comment? Done. https://codereview.chromium.org/11522009/diff/6001/chrome/browser/chrome_metr... File chrome/browser/chrome_metrics_helper.h (right): https://codereview.chromium.org/11522009/diff/6001/chrome/browser/chrome_metr... chrome/browser/chrome_metrics_helper.h:32: // This class is a thread-safe singleton. On 2012/12/13 16:24:08, SteveT wrote: > Could you add a brief doc for this class? Done. https://codereview.chromium.org/11522009/diff/6001/chrome/browser/renderer_ho... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/11522009/diff/6001/chrome/browser/renderer_ho... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:34: #include "chrome/common/metrics/variations/variations_util.h" On 2012/12/13 16:24:08, SteveT wrote: > Can you check if this header is still needed? Not needed, removed. https://codereview.chromium.org/11522009/diff/6001/chrome/browser/renderer_ho... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:35: #include "chrome/common/metrics/proto/chrome_experiments.pb.h" On 2012/12/13 16:24:08, SteveT wrote: > Ditto. Done. https://codereview.chromium.org/11522009/diff/16001/chrome/browser/autocomple... File chrome/browser/autocomplete/autocomplete_field_trial.cc (right): https://codereview.chromium.org/11522009/diff/16001/chrome/browser/autocomple... chrome/browser/autocomplete/autocomplete_field_trial.cc:139: "0", 2013, 7, 1, NULL); On 2012/12/14 17:09:55, Mark P wrote: > On 2012/12/14 17:06:07, Bart N. wrote: > > On 2012/12/14 16:56:41, Mark P wrote: > > > Let's bump the expiration date a bit too. How about > > > 7 -> 9 > > > > Done. > > And the word July -> September. > (sorry I forgot to mention this) Yeah, I haven't noticed either :) Done. On a side note, I think we should get rid of the comment (both lines). In general, we should avoid comments that state obvious things. In this particular case the comments don't bring anything that is not already clearly expressed in the arguments below. On the other hand, if the date year/mm/day was encoded as a number of days since 1900/1/1, it'd totally make sense to have the comment. https://codereview.chromium.org/11522009/diff/16001/chrome/common/metrics/var... File chrome/common/metrics/variations/variation_ids.h (right): https://codereview.chromium.org/11522009/diff/16001/chrome/common/metrics/var... chrome/common/metrics/variations/variation_ids.h:145: kSuggestTrialStarted2013Q1IDMin = 3310060, On 2012/12/14 17:09:55, Mark P wrote: > On 2012/12/14 17:06:07, Bart N. wrote: > > On 2012/12/14 16:56:41, Mark P wrote: > > > Judging by the format of this file, it seems the preference is to not skip > > > blocks of IDs. I also understand that not having round numbers makes it a > > > little harder to mentally translate field trial group numbers to experiment > > IDs. > > > I'll let Steve decide which feature is more important and therefore whether > > you > > > should skip numbers here as you do. > > I'm confused about your comment. > > > > I think we are just lucky - I haven't skipped any block, I simply followed the > > comment below which asked to use kNextID (which was ...060) and increment it > > accordingly. If there a was block skipped, it must have been decided in the > > previous change. > > Interesting. I didn't notice the kNextId had already skipped some stuff. > (Maybe they're reserving nearby ID space?) In that case, this sounds fine to > me. Acknowledged.
A couple small comments inline. I think we're pretty close. Did you try E2E tests with this patch? Repeating my previous question: The title of this CL implies that there may be more refactoring that needs to be done. Is the X-Chrome-Variations functionality not fully integrated with SearchProvider after this patch? What could be missing? https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... File chrome/browser/chrome_metrics_helper.cc (right): https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.cc:92: void ChromeMetricsHelper::OnFieldTrialGroupFinalized( This seems like a general issue with the pattern, then. I'm surprised there isn't some warning about this (perhaps it's in the docs and I missed it). Anyway, we definitely do want to be aware of such cases, so let's DCHECK as you suggested? Note that I looked through code search a bit and there appears to be cases where people DCHECK(MessageLoop::current()), but not necessary for the same reasons as discussed here. But also - what Chrome thread doesn't have a MessageLoop object associated with it? https://codereview.chromium.org/11522009/diff/23001/chrome/browser/chrome_met... File chrome/browser/chrome_metrics_helper.cc (right): https://codereview.chromium.org/11522009/diff/23001/chrome/browser/chrome_met... chrome/browser/chrome_metrics_helper.cc:40: return; nit: I think we prefer curlies around a single line statement if the "if" condition spans multiple lines.
Still looks good on the autocomplete side. https://codereview.chromium.org/11522009/diff/16001/chrome/browser/autocomple... File chrome/browser/autocomplete/autocomplete_field_trial.cc (right): https://codereview.chromium.org/11522009/diff/16001/chrome/browser/autocomple... chrome/browser/autocomplete/autocomplete_field_trial.cc:139: "0", 2013, 7, 1, NULL); On 2012/12/14 18:21:42, Bart N. wrote: > On 2012/12/14 17:09:55, Mark P wrote: > > On 2012/12/14 17:06:07, Bart N. wrote: > > > On 2012/12/14 16:56:41, Mark P wrote: > > > > Let's bump the expiration date a bit too. How about > > > > 7 -> 9 > > > > > > Done. > > > > And the word July -> September. > > (sorry I forgot to mention this) > Yeah, I haven't noticed either :) Done. > > On a side note, I think we should get rid of the comment (both lines). In > general, we should avoid comments that state obvious things. In this particular > case the comments don't bring anything that is not already clearly expressed in > the arguments below. > > On the other hand, if the date year/mm/day was encoded as a number of days since > 1900/1/1, it'd totally make sense to have the comment. I've seen bugs where the coder has the wrong order for dates (1/7 vs 7/1). This wasn't caught by the reviewer because the reviewer didn't know when the coder intended things to expire so the dates looked fine to him. So the explanation at least helps the reviewer check the code. Whether it needs to be there in a comment in the file, that's another question... in any case, this is fine as is.
On 2012/12/14 18:35:41, SteveT wrote: > A couple small comments inline. I think we're pretty close. > > Did you try E2E tests with this patch? Not yet. I need a custom GWS and dump headers there. I'll do it before submission. > > Repeating my previous question: The title of this CL implies that there may be > more Did you check the description? I fixed it before I sent my previous round of comments. > refactoring that needs to be done. Is the X-Chrome-Variations functionality not > fully integrated with SearchProvider after this patch? What could be missing? Looking at the bot failures, there seem to be some issues with thread assumptions. I'm now looking into some P0 bug, so I'll be back to this issue later.
https://codereview.chromium.org/11522009/diff/16001/chrome/browser/autocomple... File chrome/browser/autocomplete/autocomplete_field_trial.cc (right): https://codereview.chromium.org/11522009/diff/16001/chrome/browser/autocomple... chrome/browser/autocomplete/autocomplete_field_trial.cc:139: "0", 2013, 7, 1, NULL); On 2012/12/14 18:37:34, Mark P wrote: > On 2012/12/14 18:21:42, Bart N. wrote: > > On 2012/12/14 17:09:55, Mark P wrote: > > > On 2012/12/14 17:06:07, Bart N. wrote: > > > > On 2012/12/14 16:56:41, Mark P wrote: > > > > > Let's bump the expiration date a bit too. How about > > > > > 7 -> 9 > > > > > > > > Done. > > > > > > And the word July -> September. > > > (sorry I forgot to mention this) > > Yeah, I haven't noticed either :) Done. > > > > On a side note, I think we should get rid of the comment (both lines). In > > general, we should avoid comments that state obvious things. In this > particular > > case the comments don't bring anything that is not already clearly expressed > in > > the arguments below. > > > > On the other hand, if the date year/mm/day was encoded as a number of days > since > > 1900/1/1, it'd totally make sense to have the comment. > > I've seen bugs where the coder has the wrong order for dates (1/7 vs 7/1). This Coming from Europe, I've experienced this issues many times. In such cases, if possible, a better approach is to pick date such as there is no confusion. In this case, I'd simply change it to the last day of the previous month, e.g. 6/30 instead of 1/7. This approach is better, because most likely we will get a DCHECK failure somewhere. A reviewer may still fail to notice the wrong format. > wasn't caught by the reviewer because the reviewer didn't know when the coder > intended things to expire so the dates looked fine to him. So the explanation > at least helps the reviewer check the code. Whether it needs to be there in a > comment in the file, that's another question... > > in any case, this is fine as is.
Waiting on your thread-related bot fixes before I give you the lg.tm.
On 2012/12/15 20:25:35, SteveT wrote: > Waiting on your thread-related bot fixes before I give you the lg.tm. Sure - I'll try to have something later today and ping back when done.
On 2012/12/15 21:09:23, Bart N. wrote: > On 2012/12/15 20:25:35, SteveT wrote: > > Waiting on your thread-related bot fixes before I give you the lg.tm. > Sure - I'll try to have something later today and ping back when done. BTW, the fix is trivial, I just need to get on VPN...
Steve, I believe I've addressed all your comments, specifically: (1) CL title and description are updated (2) I ran a test with tcpdump on port 80 and was looking at X-Chrome-Variations headers transmitted to google.com; this is not a full E2E test, but I'm fairly confident that past GWS we are all good, since it was working fine for /s requests. (3) I fixed the failing tests; the issue was related to accessing some profile's IO data which wasn't available in incognito mode. I fixed it such as it won't be accessed unless in normal mode. It passed on linux_rel, now trying on all bots. PTAL https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... File chrome/browser/chrome_metrics_helper.cc (right): https://codereview.chromium.org/11522009/diff/1/chrome/browser/chrome_metrics... chrome/browser/chrome_metrics_helper.cc:92: void ChromeMetricsHelper::OnFieldTrialGroupFinalized( On 2012/12/14 18:35:41, SteveT wrote: > This seems like a general issue with the pattern, then. I'm surprised there > isn't some warning about this (perhaps it's in the docs and I missed it). > > Anyway, we definitely do want to be aware of such cases, so let's DCHECK as you > suggested? Note that I looked through code search a bit and there appears to be > cases where people DCHECK(MessageLoop::current()), but not necessary for the > same reasons as discussed here. > > But also - what Chrome thread doesn't have a MessageLoop object associated with > it? No idea. In our case, both IO/UI have it. Done. https://codereview.chromium.org/11522009/diff/23001/chrome/browser/chrome_met... File chrome/browser/chrome_metrics_helper.cc (right): https://codereview.chromium.org/11522009/diff/23001/chrome/browser/chrome_met... chrome/browser/chrome_metrics_helper.cc:40: return; On 2012/12/14 18:35:41, SteveT wrote: > nit: I think we prefer curlies around a single line statement if the "if" > condition spans multiple lines. Done.
LGTM - just wanted to check that our threading assumptions were still good. I think you still need an OWNERS approval from Peter for a couple things, but after that, you can feel free to commit. Do let me know the results of the E2E tests.
lgtm Still looks good from the autocomplete/... side
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bartn@chromium.org/11522009/32004
Presubmit check for 11522009-32004 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome
Presubmit checks took 1.5s to calculate.
Scott/Ben, can you stamp the top level chrome/chrome_browser.gypi change, please?
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bartn@chromium.org/11522009/32004
Steve, need you to take a look at some of my comments here. https://codereview.chromium.org/11522009/diff/32004/chrome/browser/chrome_met... File chrome/browser/chrome_metrics_helper.cc (right): https://codereview.chromium.org/11522009/diff/32004/chrome/browser/chrome_met... chrome/browser/chrome_metrics_helper.cc:32: // 2. Only transmit for non-Incognito profiles. Why this bullet? Incognito is to protect your local disk, not to anonymize you to servers. I don't think we should be checking incognito state here. https://codereview.chromium.org/11522009/diff/32004/chrome/browser/chrome_met... chrome/browser/chrome_metrics_helper.cc:110: if (variation_ids_set_.size() > 20) { I read Steve's explanation for this, but the style guide bans what's going on here. We can't use DCHECK as a "this probably shouldn't be happening, notify developers" function, and we can't write subsequent handler code for if the DCHECKed condition fails. I suggest simply removing the DCHECK. I don't think its value is high. There are also other options, such as converting it to a DVLOG or something, but in general we don't tend to collect or read log data, so I would only log if the logging is definitively going to get noticed were this condition to happen in the real world. https://codereview.chromium.org/11522009/diff/32004/chrome/browser/chrome_met... File chrome/browser/chrome_metrics_helper.h (right): https://codereview.chromium.org/11522009/diff/32004/chrome/browser/chrome_met... chrome/browser/chrome_metrics_helper.h:28: template <typename T> struct DefaultSingletonTraits; Tiny nit: I have no idea if we document this somewhere, but it seems like we usually put non-namespaced class declarations above the namespaced ones. And I'd probably eliminate the newlines between them. https://codereview.chromium.org/11522009/diff/32004/chrome/browser/chrome_met... chrome/browser/chrome_metrics_helper.h:32: // This class is a thread-safe singleton. So, this critical question never got answered by Steve: does this class really need to be a threadsafe singleton? Or, to put it differently: is the cost of calling GetActiveFieldTrialGroups() so large that we need all the caching behavior this class implements? Because if we eliminate that caching, then the only issue is whether that function can be called on multiple threads. The answer to that is unclear to me; the implementation uses a lock, which suggests it's thread safe, but the header comments are unclear. https://codereview.chromium.org/11522009/diff/32004/chrome/browser/chrome_met... chrome/browser/chrome_metrics_helper.h:77: std::set<chrome_variations::VariationID> variation_ids_set_; Nit: Consider a typedef (e.g. "Variations") for this type.
Back to you guys - sorry about missing those comments earlier. https://codereview.chromium.org/11522009/diff/32004/chrome/browser/chrome_met... File chrome/browser/chrome_metrics_helper.cc (right): https://codereview.chromium.org/11522009/diff/32004/chrome/browser/chrome_met... chrome/browser/chrome_metrics_helper.cc:32: // 2. Only transmit for non-Incognito profiles. We didn't have very strong reasons for removing this from incognito mode in our discussions way back, but I think our immediate instinct was to shy away from studying incognito sessions too much. I don't think we lose too much from NOT receiving this data from incognito sessions, nor would we gain much if we were to include it. I've cc'd Tyler who was also involved in the discussions about this decision, and we can debate further if you wish. https://codereview.chromium.org/11522009/diff/32004/chrome/browser/chrome_met... chrome/browser/chrome_metrics_helper.cc:110: if (variation_ids_set_.size() > 20) { Again, I believe I mentioned this before, but we (myself + the GWS guys) chose 10 and 20 as almost arbitrary thresholds. It's hard to tell what value might be considered "too high". If you're interested, we can try to measure the lengths of the header string at these levels and figure out what might be actually "too high" to transmit to Google.com all the time. But you're right in that if the style guide says "a DCHECK means this should never happen", then we shouldn't be addressing cases that go beyond that point. I think we still want a DVLOG here to warn developers if we go over, though, since the <= 10 check was meant to be a warning. Does that sound fair? https://codereview.chromium.org/11522009/diff/32004/chrome/browser/chrome_met... File chrome/browser/chrome_metrics_helper.h (right): https://codereview.chromium.org/11522009/diff/32004/chrome/browser/chrome_met... chrome/browser/chrome_metrics_helper.h:32: // This class is a thread-safe singleton. So it's not just the call to GetActiveFieldTrialGroups that is expensive (that method just enumerates 20-30 field trials and returns their names in a vector), but also the rest of the work to create the cache (serializing it in a protobuf-string format, and then base64-hashing it). Calling this once isn't bad, but we figured calling this _every_ time a request is sent to google.com might be a bit much. Now, we have not measured the actual cost of this vs the cost of the locking, so it's hard to say for sure if it's *necessary*, but I suspect it's still nice to have this optimization. Do you think we're blowing this out of proportion here? Bart - As for the comment, were you referring to this class being thread safe since you've implemented proper locking in it, or were you referring to it getting some thread-safe properties out of being used with DefaultSingletonTraits and Singleton<...> (not familiar with whether or not that is something you get for free from this Singleton configuration).
One more high level comment - we agreed with Peter to uncheck the commit checkbox given that Peter promised to ensure that this change lands in beta, should it not make on time before the cutoff today. https://codereview.chromium.org/11522009/diff/32004/chrome/browser/chrome_met... File chrome/browser/chrome_metrics_helper.h (right): https://codereview.chromium.org/11522009/diff/32004/chrome/browser/chrome_met... chrome/browser/chrome_metrics_helper.h:32: // This class is a thread-safe singleton. On 2012/12/17 20:30:46, SteveT wrote: > So it's not just the call to GetActiveFieldTrialGroups that is expensive (that > method just enumerates 20-30 field trials and returns their names in a vector), > but also the rest of the work to create the cache (serializing it in a > protobuf-string format, and then base64-hashing it). Calling this once isn't > bad, but we figured calling this _every_ time a request is sent to http://google.com > might be a bit much. > > Now, we have not measured the actual cost of this vs the cost of the locking, so > it's hard to say for sure if it's *necessary*, but I suspect it's still nice to > have this optimization. Do you think we're blowing this out of proportion here? > > Bart - As for the comment, were you referring to this class being thread safe > since you've implemented proper locking in it, or were you referring to it > getting some thread-safe properties out of being used with > DefaultSingletonTraits and Singleton<...> (not familiar with whether or not that > is something you get for free from this Singleton configuration). No, I was referring to the fact that all public methods of this class can be called from multiple threads, specifically AppendHeaders(...). We have to make sure this holds, because AppendHeaders is called from IO and UI threads and we want to avoid a race condition where headers are being updated while a call to AppendHeaders is in progress. If I understand Peter's comment correctly, he objects the singleton part which is necessary because of caching. Thread-safety constraint will have to remain regardless if we use singleton or not, because we call AppendHeaders from two different threads. The only question remains is whether we need a lock or not. Certainly, if the underlying GetFieldTrialList method is thread-safe, we won't need the lock here (given that we no longer need singleton). Lastly, regarding lock cost -- I'm almost certain the lock cost is lower than iterating over all field trial lists and computing Base64, but both methods can actually be cheap in absolute terms. One more thing to keep in mind that when this change lands, we will be calling it pretty much on each keystroke.
Back to you. https://codereview.chromium.org/11522009/diff/32004/chrome/browser/chrome_met... File chrome/browser/chrome_metrics_helper.h (right): https://codereview.chromium.org/11522009/diff/32004/chrome/browser/chrome_met... chrome/browser/chrome_metrics_helper.h:32: // This class is a thread-safe singleton. GetActiveFieldTrialGroups (you were referring to this, right?) is itself guarded by a lock. But of course that doesn't protect the X-Chrome-Variations header string which is written/read potentially from different threads. So it sounds to me like we still need to lock that as a separate resource. Let me know if I'm missing something here..
https://codereview.chromium.org/11522009/diff/32004/chrome/browser/chrome_met... File chrome/browser/chrome_metrics_helper.h (right): https://codereview.chromium.org/11522009/diff/32004/chrome/browser/chrome_met... chrome/browser/chrome_metrics_helper.h:32: // This class is a thread-safe singleton. On 2012/12/17 20:49:56, SteveT wrote: > GetActiveFieldTrialGroups (you were referring to this, right?) is itself guarded > by a lock. But of course that doesn't protect the X-Chrome-Variations header > string which is written/read potentially from different threads. So it sounds to > me like we still need to lock that as a separate resource. Let me know if I'm > missing something here.. Yes, GetActiveFieldTrialGroups. So my understanding is as follows: if GetActiveFieldTrialGroups+UpdateVariationIDsHeaderValue is cheap (and we can decide what cheap means for us, e.g. <= 1us), we don't need the whole singleton logic. We can basically have it stateless (e.g. let UpdateVariationIDsHeaderValue return the header value rather than update class state). I think this is what Peter asked for.
On 2012/12/17 20:49:56, SteveT wrote: > GetActiveFieldTrialGroups (you were referring to this, right?) is itself guarded > by a lock. But of course that doesn't protect the X-Chrome-Variations header > string which is written/read potentially from different threads. So it sounds to > me like we still need to lock that as a separate resource. Not if we don't keep that as a member at all, and actually recalculate on every single call. Which is what I was proposing. Basically in that world this class becomes a single utility function to call GetActiveFieldTrialGroups() and base64-encode it. Basically, my contention is not that the cost of doing this is equivalent to the cost of locking -- it's that unless the cost of this actually matters, code simplicity trumps all else. It would be nice to implement the dead-simple way and then use either a histogram of the tick count cost of the funciton or a quick profiling run to see if any of this actually costs enough that we should go ahead and build a threadsafe cache. Note that if we do want to build such a cache, one could argue that it'd be better to put it in field_trial.cc itself, as this is basically GetActiveFieldTrialGroups() as a string instead of a vector. In that case we could potentially update the cache directly when the field trials change and never at any other time, for relatively little code. Last note -- regarding the DCHECK->DVLOG issue, using DVLOG is OK with me if you think you're actually going to ever see and collect those logs.
On 2012/12/17 21:10:10, Peter Kasting wrote: > On 2012/12/17 20:49:56, SteveT wrote: > > GetActiveFieldTrialGroups (you were referring to this, right?) is itself > guarded > > by a lock. But of course that doesn't protect the X-Chrome-Variations header > > string which is written/read potentially from different threads. So it sounds > to > > me like we still need to lock that as a separate resource. > > Not if we don't keep that as a member at all, and actually recalculate on every > single call. Which is what I was proposing. Basically in that world this class > becomes a single utility function to call GetActiveFieldTrialGroups() and > base64-encode it. > > Basically, my contention is not that the cost of doing this is equivalent to the > cost of locking -- it's that unless the cost of this actually matters, code > simplicity trumps all else. It would be nice to implement the dead-simple way > and then use either a histogram of the tick count cost of the funciton or a > quick profiling run to see if any of this actually costs enough that we should > go ahead and build a threadsafe cache. Agreed about the simplicity. So we'll have to measure this before we can make a strong statement about it. But yes, perhaps a Singleton for something that could be a utility function is overkill. > > Note that if we do want to build such a cache, one could argue that it'd be > better to put it in field_trial.cc itself, as this is basically > GetActiveFieldTrialGroups() as a string instead of a vector. In that case we > could potentially update the cache directly when the field trials change and > never at any other time, for relatively little code. One thing to point out is that in the past we've had resistance to building very feature-specific functionality (such as creating a special header for special requests) into the FieldTrials code, since it's part of the base libraries. > > Last note -- regarding the DCHECK->DVLOG issue, using DVLOG is OK with me if you > think you're actually going to ever see and collect those logs. Cool. This is definitely a warning that we want to be looking out for.
That last mail didn't transmit this inline comment with my proposal... https://codereview.chromium.org/11522009/diff/32004/chrome/browser/chrome_met... File chrome/browser/chrome_metrics_helper.h (right): https://codereview.chromium.org/11522009/diff/32004/chrome/browser/chrome_met... chrome/browser/chrome_metrics_helper.h:32: // This class is a thread-safe singleton. Yes, that seems fair. Is it OK for us to keep the code as-is for now, and measure/act on this after M25? Perhaps merge in something if it turns out to be performance-critical?
The comment about possible not putting this in field_trials.cc is certainly a reasonable concern. I would probably still be OK with doing it -- this is already used by at least two consumers, and it's not really unreasonable to say "just like the existing function, but returned as an encoded string" -- but I'd be happy to get the feedback of Brett or some other base owner. As for when to do these changes -- in the limit we can always do changes as multiple CLs, of course. I just figure, since we're newly introducing this object, it'd be nice to do it right the first time. And a simple changelist is also one that's easier to test, merge into other branches, etc. But if you guys think it's a significant hardship to fix this CL as opposed to doing a followup, I won't stop you.
Bart - Feel free to proceed with what's best for you here. If measuring the cost and making the change is easy, go with that, otherwise I think a followup is fine, here. As for the issue with incognito (I've cc'd Tyler): Peter, do you feel strongly about this treatment of incognito? I'm guessing that your understanding of the feature is "Chrome won't track me" as opposed to "Google won't track me"?
Hey guys - a few remarks from my side. First of all, Peter, I'm pretty sure you are well aware what the motivation of this CL was. Given that the owner (stevet@) was OOO and limited time before the cutoff, I agreed to take on this CL to make sure we can land it on time. The idea was to take the code and wrap it in a class, such is it could be reused from two places, without making major design changes. I think I made it clear in the original CL caveats. Now, you are objecting the original design and you have absolutely valid points, however I disagree with the approach of doing it in one CL given the circumstances and I'm deeply saddened about the outcome. I wish it was clear enough last week that (1) you want this CL to address existing design issues and (2) you are OK with requesting a merge later on. If so, I wouldn't take on it and waste a chunk of my weekend time knowing that it goes in vain. Having said that, I think that right approach is to run some performance tests and find out if we indeed need to cache headers. If we don't, there is no question about code simplicity and we should stick with the better design. I'd like to leave it up to the original owner, though. Bart
On 2012/12/17 22:04:54, SteveT wrote: > Bart - Feel free to proceed with what's best for you here. If measuring the cost > and making the change is easy, go with that, otherwise I think a followup is > fine, here. My preference is to submit as is, but to follow up to address remaining concerns. I simply cannot afford to spend more time on this CL. > > As for the issue with incognito (I've cc'd Tyler): Peter, do you feel strongly > about this treatment of incognito? I'm guessing that your understanding of the > feature is "Chrome won't track me" as opposed to "Google won't track me"?
Regarding the question of incognito mode: we made a decision to omit incognito windows from the variations header to simplify our privacy conversation with the initial design. I completely agree with Steve's assessment above: "I don't think we lose too much from NOT receiving this data from incognito sessions, nor would we gain much if we were to include it." The X-Chrome-Variations header is not PII and the incognito goals are definitely about local and not server oriented tracking. So I would be fine with including them in Incognito mode, but wouldn't be especially interested in investing resources to adding them. Probably worth gut-checking with battre@ for a Privacy perspective.
On 2012/12/17 22:13:46, Bart N. wrote: > First of all, Peter, I'm pretty sure you are well aware what the motivation of > this CL was. Given that the owner (stevet@) was OOO and limited time before the > cutoff, I agreed to take on this CL to make sure we can land it on time. Sorry, I didn't know last week that you had any worry about the branch date. Branch dates are so meaningless to me that I hardly pay attention to them. They're important as go/no-go points for major features, but for small details and under-the-hood refactorings, we can generally merge things we need when we need to unless we're way too close to the actual stable ship point. > Now, you are objecting the original design and you have absolutely valid points, > however I disagree with the approach of doing it in one CL given the > circumstances and I'm deeply saddened about the outcome. Again, I'm sorry you're deeply saddened. I can understand why you'd feel frustrated when you felt like the only reason you worked on this to begin with is because you wanted it in days ago and Steve was out. That goes double if you spent time on this this weekend. Completely makes sense! I didn't realize you wanted this change to go in as-is without any major changes regardless. Maybe after I did my initial review and asked questions about the entire approach, saying, "regardless of the merit of this question, I'm not comfortable addressing it in this CL; if you want this fixed, please negotiate with Steve separately so I can stop work on this and do something more critical" would have been useful. I don't mind hearing it now -- I just would have liked to avoid frustrating you unnecessarily. My position is always the same on all CLs: * Yes, I'd like it done right, even if that means a bigger change; but, * If something Absolutely Has To Go In as-is and we save refactoring until later, that's fine, as long as it does eventually happen. I'm asking the questions I'm asking about the core design because they need to be asked at some point, but if now isn't the right time to fix them, then lay out the plan for when the right time is, and move forward. I don't want you feeling frustrated -- assert your needs and we'll work it out :)
On 2012/12/17 22:51:07, odean wrote: > Regarding the question of incognito mode: we made a decision to omit incognito > windows from the variations header to simplify our privacy conversation with the > initial design. I completely agree with Steve's assessment above: "I don't think > we lose too much from NOT receiving this data from incognito sessions, nor would > we gain much if we were to include it." I don't actually know anything about the significance of this data so I can't comment on that assertion -- I'll just note that wasn't the source of my concern. > The X-Chrome-Variations header is not PII and the incognito goals are definitely > about local and not server oriented tracking. So I would be fine with including > them in Incognito mode, but wouldn't be especially interested in investing > resources to adding them. Probably worth gut-checking with battre@ for a Privacy > perspective. The implementation difference here is pretty trivial -- by not checking incognito state we slightly simplify the callers and callee, but that's it. The main reason I didn't want to check incognito state is because it muddies the waters for someone reading the code about what this data means and what incognito does. It implies there's something about sending this data that violates users' incognito expectations, which I claim isn't really true. I'd just rather keep the conceptual line brighter where we can, and not reference incognito if it isn't necessary.
On 2012/12/18 00:55:04, Peter Kasting wrote: > On 2012/12/17 22:13:46, Bart N. wrote: > > First of all, Peter, I'm pretty sure you are well aware what the motivation of > > this CL was. Given that the owner (stevet@) was OOO and limited time before > the > > cutoff, I agreed to take on this CL to make sure we can land it on time. > > Sorry, I didn't know last week that you had any worry about the branch date. > > Branch dates are so meaningless to me that I hardly pay attention to them. > They're important as go/no-go points for major features, but for small details > and under-the-hood refactorings, we can generally merge things we need when we > need to unless we're way too close to the actual stable ship point. Peter, I think you are overoptimistic; in fact we had in the past one particular merge request, for which you were comfortable with, but kareng@ was not and our merge was rejected (I can forward the email thread if you don't remember). Also, Mark mentioned a couple of times that's better to avoid requesting a merge unless really, really critical. I don't know all the Chrome politics, but one thing I know for sure is that if a change lands on time, there is no need to merge. Steve, here is my stance - if you are OK with this CL and Peter is OK as well, please go ahead and check the commit box. Otherwise, feel free to use it as a starting point for a more robust fix. Meanwhile, I'm going to assign the bug back to you. We will need to ship it in M25, if possible, otherwise we will end up with 12 weeks of no experiments (we are already going to miss M24 which has the same bug).
On 2012/12/18 02:01:24, Bart N. wrote: > Peter, I think you are overoptimistic; in fact we had in the past one particular > merge request, for which you were comfortable with, but kareng@ was not and our > merge was rejected (I can forward the email thread if you don't remember). Indeed I don't remember which one this was. Also "comfortable with" isn't the same as "I will fight for this on your behalf", which is what I was guaranteeing with this change :).
I believe we've roughly agreed that we can address these additional performance issues in a followup patch. I'll check the box now and let it make it's way in (I thought the box had been checked earlier, but it looks like it was cancelled). Hopefully we didn't miss the train :\ If we did, we'll have to do some fighting to get it merged in to avoid losing these experiments.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bartn@chromium.org/11522009/32004
Message was sent while issue was closed.
Change committed as 173740
Message was sent while issue was closed.
Hey guys, looks like this patch didn't make it into M25 (which branched at 173683). How do we want to proceed?
Message was sent while issue was closed.
On 2012/12/19 19:30:31, SteveT wrote: > Hey guys, looks like this patch didn't make it into M25 (which branched at > 173683). > > How do we want to proceed? Request a merge. Just say it didn't quite make it and we need this data. If you get any pushback, forward things to me.
Message was sent while issue was closed.
Cool, thanks. Merge requested. We'll see what the TPMs say. In the meantime I'll start planning the testing and refactoring... |
