Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
307743003:
Add more Google domains to the list of domains which should have (Closed)
|
Created:
6 years, 6 months ago by Ryan Hamilton Modified:
6 years, 6 months ago CC:
chromium-reviews, Ilya Sherman, asvitkine+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd more Google domains to the list of domains which should have X-Chrome-Data sent to them.
BUG=379341
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274021
Patch Set 1 #
Total comments: 9
Patch Set 2 : Fix comments #Patch Set 3 : Fix comments #Patch Set 4 : Fix comments from avd #
Total comments: 4
Patch Set 5 : Fix more comments from avd #
Total comments: 8
Patch Set 6 : fix comments #Patch Set 7 : use host.find_last_of #
Total comments: 1
Patch Set 8 : Back to v1 #
Messages
Total messages: 26 (0 generated)
https://codereview.chromium.org/307743003/diff/1/chrome/browser/metrics/varia... File chrome/browser/metrics/variations/variations_http_header_provider.cc (right): https://codereview.chromium.org/307743003/diff/1/chrome/browser/metrics/varia... chrome/browser/metrics/variations/variations_http_header_provider.cc:244: EndsWith(host, ".ytimg.com", false) || If you prefer, we could stick these suffixes into a hash_set, and then check to see if the suffix of the host is in the set?
https://codereview.chromium.org/307743003/diff/1/chrome/browser/metrics/varia... File chrome/browser/metrics/variations/variations_http_header_provider.cc (right): https://codereview.chromium.org/307743003/diff/1/chrome/browser/metrics/varia... chrome/browser/metrics/variations/variations_http_header_provider.cc:244: EndsWith(host, ".ytimg.com", false) || On 2014/05/28 22:38:31, Ryan Hamilton wrote: > If you prefer, we could stick these suffixes into a hash_set, and then check to > see if the suffix of the host is in the set? Yeah, that would be better if we really want that many domains here. By the way, are all of these absolutely necessary? My understanding is you want to measure the effect of QUIC acceleration on requests to these domains from the server-side, hence why these are being added. So will you really look at metrics at each one of those domains server-side? I also wonder if perhaps it would make sense to have more fine-grained control over these. Listing these domains here will result in all server-visible experiments being sent as headers to them now - which will actually make many web sessions a tiny bit slower (i.e. due to extra headers to all these popular sites). Perhaps a better solution would be to have individual experiments list the domains they care about in their configs - that way only QUIC can list these specific sites, so the headers sent to them would be more minimal. WDYT?
On 2014/05/29 00:08:53, Alexei Svitkine wrote: > https://codereview.chromium.org/307743003/diff/1/chrome/browser/metrics/varia... > File chrome/browser/metrics/variations/variations_http_header_provider.cc > (right): > > https://codereview.chromium.org/307743003/diff/1/chrome/browser/metrics/varia... > chrome/browser/metrics/variations/variations_http_header_provider.cc:244: > EndsWith(host, ".ytimg.com", false) || > On 2014/05/28 22:38:31, Ryan Hamilton wrote: > > If you prefer, we could stick these suffixes into a hash_set, and then check > to > > see if the suffix of the host is in the set? > > Yeah, that would be better if we really want that many domains here. By the way, > are all of these absolutely necessary? My understanding is you want to measure > the effect of QUIC acceleration on requests to these domains from the > server-side, hence why these are being added. So will you really look at metrics > at each one of those domains server-side? > > I also wonder if perhaps it would make sense to have more fine-grained control > over these. Listing these domains here will result in all server-visible > experiments being sent as headers to them now - which will actually make many > web sessions a tiny bit slower (i.e. due to extra headers to all these popular > sites). > > Perhaps a better solution would be to have individual experiments list the > domains they care about in their configs - that way only QUIC can list these > specific sites, so the headers sent to them would be more minimal. WDYT? At a minimum, we need experiments listed for both the QUIC connection... and the control, which is HTTP over TCP, or SPDY commonly. ...and it turns out that our failure to gather data from these other named servers very significantly reduced our data collection (and confused the heck out of us). In the case of SPDY or QUIC, such redundant (repeated) header transmissions should be compressed out to almost nothing (in a single SPDY or QUIC connection, fetching a series of resources). As a result, I *think* the serialization latency impact will be small very often... although it will have a tad of impact when there is only one resource every fetched over a SPDY/QUIC connection.
I think it's OK to add the extra domains here, but let's do it in a set that we initialize on startup and just check set membership. Also, I'd really prefer if we could trim the list to just the ones that are really necessary. For example, www.googlevideo.com seems to be just a redirect (same reason we don't list gmail.com), so it doesn't seem necessary to list it. Are there others like this that can be removed? https://codereview.chromium.org/307743003/diff/1/chrome/browser/metrics/varia... File chrome/browser/metrics/variations/variations_http_header_provider.cc (right): https://codereview.chromium.org/307743003/diff/1/chrome/browser/metrics/varia... chrome/browser/metrics/variations/variations_http_header_provider.cc:238: EndsWith(host, ".googleapis.com", false) || This seems to be repeated. https://codereview.chromium.org/307743003/diff/1/chrome/browser/metrics/varia... chrome/browser/metrics/variations/variations_http_header_provider.cc:241: EndsWith(host, ".googlevideo.com", false) || This seems to be just a redirect, do we really need it? https://codereview.chromium.org/307743003/diff/1/chrome/browser/metrics/varia... chrome/browser/metrics/variations/variations_http_header_provider.cc:243: EndsWith(host, ".youtube.com", false) || Youtube is already being checked lower down.
https://codereview.chromium.org/307743003/diff/1/chrome/browser/metrics/varia... File chrome/browser/metrics/variations/variations_http_header_provider.cc (right): https://codereview.chromium.org/307743003/diff/1/chrome/browser/metrics/varia... chrome/browser/metrics/variations/variations_http_header_provider.cc:238: EndsWith(host, ".googleapis.com", false) || On 2014/05/29 20:14:38, Alexei Svitkine wrote: > This seems to be repeated. Done. https://codereview.chromium.org/307743003/diff/1/chrome/browser/metrics/varia... chrome/browser/metrics/variations/variations_http_header_provider.cc:241: EndsWith(host, ".googlevideo.com", false) || On 2014/05/29 20:14:38, Alexei Svitkine wrote: > This seems to be just a redirect, do we really need it? www.googlevideo.com may be a redirect, but there are lots of other hosts in this domain that are not. This is the current primary domain from which youtube video content is served. https://codereview.chromium.org/307743003/diff/1/chrome/browser/metrics/varia... chrome/browser/metrics/variations/variations_http_header_provider.cc:243: EndsWith(host, ".youtube.com", false) || On 2014/05/29 20:14:38, Alexei Svitkine wrote: > Youtube is already being checked lower down. Done. https://codereview.chromium.org/307743003/diff/1/chrome/browser/metrics/varia... chrome/browser/metrics/variations/variations_http_header_provider.cc:244: EndsWith(host, ".ytimg.com", false) || On 2014/05/29 00:08:54, Alexei Svitkine wrote: > On 2014/05/28 22:38:31, Ryan Hamilton wrote: > > If you prefer, we could stick these suffixes into a hash_set, and then check > to > > see if the suffix of the host is in the set? > > Yeah, that would be better if we really want that many domains here. By the way, > are all of these absolutely necessary? My understanding is you want to measure > the effect of QUIC acceleration on requests to these domains from the > server-side, hence why these are being added. So will you really look at metrics > at each one of those domains server-side? That's the plan, yes. > I also wonder if perhaps it would make sense to have more fine-grained control > over these. Listing these domains here will result in all server-visible > experiments being sent as headers to them now - which will actually make many > web sessions a tiny bit slower (i.e. due to extra headers to all these popular > sites). > > Perhaps a better solution would be to have individual experiments list the > domains they care about in their configs - that way only QUIC can list these > specific sites, so the headers sent to them would be more minimal. WDYT? Sounds good... But not as part of this CL, I think.
Thanks for the other fixes. https://codereview.chromium.org/307743003/diff/50001/chrome/browser/metrics/v... File chrome/browser/metrics/variations/variations_http_header_provider_unittest.cc (right): https://codereview.chromium.org/307743003/diff/50001/chrome/browser/metrics/v... chrome/browser/metrics/variations/variations_http_header_provider_unittest.cc:116: { "http://WWW.ANDROID.COM", true }, thanks for adding a test case with mixed case. https://codereview.chromium.org/307743003/diff/50001/chrome/browser/metrics/v... chrome/browser/metrics/variations/variations_http_header_provider_unittest.cc:128: { "http://www.ytimg.com", true }, + { "http://wwwytimg.com", false }, + { "http://ytimg.com", false },
Thanks for the review. https://codereview.chromium.org/307743003/diff/50001/chrome/browser/metrics/v... File chrome/browser/metrics/variations/variations_http_header_provider_unittest.cc (right): https://codereview.chromium.org/307743003/diff/50001/chrome/browser/metrics/v... chrome/browser/metrics/variations/variations_http_header_provider_unittest.cc:116: { "http://WWW.ANDROID.COM", true }, On 2014/05/29 23:38:20, avd wrote: > thanks for adding a test case with mixed case. My pleasure! https://codereview.chromium.org/307743003/diff/50001/chrome/browser/metrics/v... chrome/browser/metrics/variations/variations_http_header_provider_unittest.cc:128: { "http://www.ytimg.com", true }, On 2014/05/29 23:38:20, avd wrote: > + { "http://wwwytimg.com", false }, > + { "http://ytimg.com", false }, Done.
https://codereview.chromium.org/307743003/diff/70001/chrome/browser/metrics/v... File chrome/browser/metrics/variations/variations_http_header_provider.cc (right): https://codereview.chromium.org/307743003/diff/70001/chrome/browser/metrics/v... chrome/browser/metrics/variations/variations_http_header_provider.cc:254: for (size_t i = host.length(); i > 0; --i) { Can you use string::find_last_of() instead? (the size_t find_last_of (const char* s, size_t pos, size_t n) const; version can take |n| as a param I think). https://codereview.chromium.org/307743003/diff/70001/chrome/browser/metrics/v... chrome/browser/metrics/variations/variations_http_header_provider.cc:269: std::string suffix = StringToLowerASCII(host.substr(suffix_start)); Wait, what if it's not ASCII? https://codereview.chromium.org/307743003/diff/70001/chrome/browser/metrics/v... chrome/browser/metrics/variations/variations_http_header_provider.cc:270: if (ContainsKey(suffixes_to_set_headers_for_, suffix)) { Nit: No {}'s
https://codereview.chromium.org/307743003/diff/70001/chrome/browser/metrics/v... File chrome/browser/metrics/variations/variations_http_header_provider.cc (right): https://codereview.chromium.org/307743003/diff/70001/chrome/browser/metrics/v... chrome/browser/metrics/variations/variations_http_header_provider.cc:254: for (size_t i = host.length(); i > 0; --i) { On 2014/05/30 17:23:10, Alexei Svitkine wrote: > Can you use string::find_last_of() instead? > > (the size_t find_last_of (const char* s, size_t pos, size_t n) const; version > can take |n| as a param I think). I tried that, but it doesn't seem to work. Here's the documentation on that method. Unless I'm misreading, "n" doesn't affect the number of occurrences to search for, but rather the number of characters in s to consider: Pointer to an array of characters. If argument n is specified (3), the first n characters in the array are searched for. Otherwise (2), a null-terminated sequence is expected: the length of the sequence with the characters to match is determined by the first occurrence of a null character. But if I'm misreading this, I'd be happy to use it. I tried: size_t suffix_start = host.find_last_of(".", host.length(), 2); https://codereview.chromium.org/307743003/diff/70001/chrome/browser/metrics/v... chrome/browser/metrics/variations/variations_http_header_provider.cc:269: std::string suffix = StringToLowerASCII(host.substr(suffix_start)); On 2014/05/30 17:23:10, Alexei Svitkine wrote: > Wait, what if it's not ASCII? All of the suffixes in "suffixes_to_set_headers_for_" are ASCII. So if host contains non-ASCII, we won't lower case it. But that's fine because it wasn't going to match anyway. https://codereview.chromium.org/307743003/diff/70001/chrome/browser/metrics/v... chrome/browser/metrics/variations/variations_http_header_provider.cc:270: if (ContainsKey(suffixes_to_set_headers_for_, suffix)) { On 2014/05/30 17:23:10, Alexei Svitkine wrote: > Nit: No {}'s Done.
https://codereview.chromium.org/307743003/diff/70001/chrome/browser/metrics/v... File chrome/browser/metrics/variations/variations_http_header_provider.cc (right): https://codereview.chromium.org/307743003/diff/70001/chrome/browser/metrics/v... chrome/browser/metrics/variations/variations_http_header_provider.cc:254: for (size_t i = host.length(); i > 0; --i) { On 2014/05/30 19:04:38, Ryan Hamilton wrote: > On 2014/05/30 17:23:10, Alexei Svitkine wrote: > > Can you use string::find_last_of() instead? > > > > (the size_t find_last_of (const char* s, size_t pos, size_t n) const; version > > can take |n| as a param I think). > > I tried that, but it doesn't seem to work. Here's the documentation on that > method. Unless I'm misreading, "n" doesn't affect the number of occurrences to > search for, but rather the number of characters in s to consider: > > Pointer to an array of characters. > If argument n is specified (3), the first n characters in the array are searched > for. > Otherwise (2), a null-terminated sequence is expected: the length of the > sequence with the characters to match is determined by the first occurrence of a > null character. > > But if I'm misreading this, I'd be happy to use it. I tried: > > size_t suffix_start = host.find_last_of(".", host.length(), 2); I see. Perhaps just call it twice then?
https://codereview.chromium.org/307743003/diff/70001/chrome/browser/metrics/v... File chrome/browser/metrics/variations/variations_http_header_provider.cc (right): https://codereview.chromium.org/307743003/diff/70001/chrome/browser/metrics/v... chrome/browser/metrics/variations/variations_http_header_provider.cc:254: for (size_t i = host.length(); i > 0; --i) { On 2014/05/30 19:06:19, Alexei Svitkine wrote: > On 2014/05/30 19:04:38, Ryan Hamilton wrote: > > On 2014/05/30 17:23:10, Alexei Svitkine wrote: > > > Can you use string::find_last_of() instead? > > > > > > (the size_t find_last_of (const char* s, size_t pos, size_t n) const; > version > > > can take |n| as a param I think). > > > > I tried that, but it doesn't seem to work. Here's the documentation on that > > method. Unless I'm misreading, "n" doesn't affect the number of occurrences to > > search for, but rather the number of characters in s to consider: > > > > Pointer to an array of characters. > > If argument n is specified (3), the first n characters in the array are > searched > > for. > > Otherwise (2), a null-terminated sequence is expected: the length of the > > sequence with the characters to match is determined by the first occurrence of > a > > null character. > > > > But if I'm misreading this, I'd be happy to use it. I tried: > > > > size_t suffix_start = host.find_last_of(".", host.length(), 2); > > I see. Perhaps just call it twice then? Heh, sure. Done.
Sorry for being annoying about this, but since this code will be run for every URL, would you mind actually checking whether this version is faster than your original patchset? I'm thinking you can just put a loop around the unit test body and run it e.g. 10000 times and compare the perf between the two. https://codereview.chromium.org/307743003/diff/130001/chrome/browser/metrics/... File chrome/browser/metrics/variations/variations_http_header_provider.cc (right): https://codereview.chromium.org/307743003/diff/130001/chrome/browser/metrics/... chrome/browser/metrics/variations/variations_http_header_provider.cc:252: size_t suffix_start = host.find_last_of(".", host.length(), 2); Remove unnecessary last arg. Same below.
On 2014/05/30 20:36:11, Alexei Svitkine wrote: > Sorry for being annoying about this, but since this code will be run for every > URL, would you mind actually checking whether this version is faster than your > original patchset? > > I'm thinking you can just put a loop around the unit test body and run it e.g. > 10000 times and compare the perf between the two. with hash_set [ RUN ] VariationsHttpHeaderProviderTest.ShouldAppendHeaders [ OK ] VariationsHttpHeaderProviderTest.ShouldAppendHeaders (34193 ms) initial version [ RUN ] VariationsHttpHeaderProviderTest.ShouldAppendHeaders [ OK ] VariationsHttpHeaderProviderTest.ShouldAppendHeaders (18202 ms) Shall I revert to the initial version of the CL?
Yes, sorry for the extra work. By the way, just to double check, in your testing the hash set version, did you make sure that the class creation was outside the loop? On Fri, May 30, 2014 at 5:16 PM, <rch@chromium.org> wrote: > On 2014/05/30 20:36:11, Alexei Svitkine wrote: > >> Sorry for being annoying about this, but since this code will be run for >> every >> URL, would you mind actually checking whether this version is faster than >> your >> original patchset? >> > > I'm thinking you can just put a loop around the unit test body and run it >> e.g. >> 10000 times and compare the perf between the two. >> > > with hash_set > > [ RUN ] VariationsHttpHeaderProviderTest.ShouldAppendHeaders > [ OK ] VariationsHttpHeaderProviderTest.ShouldAppendHeaders (34193 > ms) > > initial version > > [ RUN ] VariationsHttpHeaderProviderTest.ShouldAppendHeaders > [ OK ] VariationsHttpHeaderProviderTest.ShouldAppendHeaders (18202 > ms) > > Shall I revert to the initial version of the CL? > > https://codereview.chromium.org/307743003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
It is surprising that each iteration through the loop took 3.4ms On Fri, May 30, 2014 at 5:18 PM, Alexei Svitkine <asvitkine@chromium.org> wrote: > Yes, sorry for the extra work. > > By the way, just to double check, in your testing the hash set version, > did you make sure that the class creation was outside the loop? > > > On Fri, May 30, 2014 at 5:16 PM, <rch@chromium.org> wrote: > >> On 2014/05/30 20:36:11, Alexei Svitkine wrote: >> >>> Sorry for being annoying about this, but since this code will be run for >>> every >>> URL, would you mind actually checking whether this version is faster >>> than your >>> original patchset? >>> >> >> I'm thinking you can just put a loop around the unit test body and run >>> it e.g. >>> 10000 times and compare the perf between the two. >>> >> >> with hash_set >> >> [ RUN ] VariationsHttpHeaderProviderTest.ShouldAppendHeaders >> [ OK ] VariationsHttpHeaderProviderTest.ShouldAppendHeaders (34193 >> ms) >> >> initial version >> >> [ RUN ] VariationsHttpHeaderProviderTest.ShouldAppendHeaders >> [ OK ] VariationsHttpHeaderProviderTest.ShouldAppendHeaders (18202 >> ms) >> >> Shall I revert to the initial version of the CL? >> >> https://codereview.chromium.org/307743003/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hmm, indeed. Did you try this in a debug build by any chance? On Fri, May 30, 2014 at 5:20 PM, Antonio Vicente <avd@google.com> wrote: > It is surprising that each iteration through the loop took 3.4ms > > > On Fri, May 30, 2014 at 5:18 PM, Alexei Svitkine <asvitkine@chromium.org> > wrote: > >> Yes, sorry for the extra work. >> >> By the way, just to double check, in your testing the hash set version, >> did you make sure that the class creation was outside the loop? >> >> >> On Fri, May 30, 2014 at 5:16 PM, <rch@chromium.org> wrote: >> >>> On 2014/05/30 20:36:11, Alexei Svitkine wrote: >>> >>>> Sorry for being annoying about this, but since this code will be run >>>> for every >>>> URL, would you mind actually checking whether this version is faster >>>> than your >>>> original patchset? >>>> >>> >>> I'm thinking you can just put a loop around the unit test body and run >>>> it e.g. >>>> 10000 times and compare the perf between the two. >>>> >>> >>> with hash_set >>> >>> [ RUN ] VariationsHttpHeaderProviderTest.ShouldAppendHeaders >>> [ OK ] VariationsHttpHeaderProviderTest.ShouldAppendHeaders >>> (34193 ms) >>> >>> initial version >>> >>> [ RUN ] VariationsHttpHeaderProviderTest.ShouldAppendHeaders >>> [ OK ] VariationsHttpHeaderProviderTest.ShouldAppendHeaders >>> (18202 ms) >>> >>> Shall I revert to the initial version of the CL? >>> >>> https://codereview.chromium.org/307743003/ >>> >> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Fri, May 30, 2014 at 2:18 PM, Alexei Svitkine <asvitkine@chromium.org> wrote: > Yes, sorry for the extra work. > > By the way, just to double check, in your testing the hash set version, > did you make sure that the class creation was outside the loop? > I tried it both ways, and it didn't make an appreciable difference. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Fri, May 30, 2014 at 2:32 PM, Alexei Svitkine <asvitkine@chromium.org> wrote: > Hmm, indeed. Did you try this in a debug build by any chance? > Indeed. I'll go try this with a Release build. So much fun... To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Fri, May 30, 2014 at 2:40 PM, Ryan Hamilton <rch@chromium.org> wrote: > On Fri, May 30, 2014 at 2:32 PM, Alexei Svitkine <asvitkine@chromium.org > > wrote: > >> Hmm, indeed. Did you try this in a debug build by any chance? >> > > Indeed. I'll go try this with a Release build. So much fun... > With a Release build we see: hash_set: [ OK ] VariationsHttpHeaderProviderTest.ShouldAppendHeaders (8593 ms) initial version: [ OK ] VariationsHttpHeaderProviderTest.ShouldAppendHeaders (4418 ms) To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
PTAL
LGTM, thanks for checking performance. Can you also file a crbug to track this CL and set it to M37? (Makes it easier to track for what release something like this landed.)
On 2014/05/30 22:28:04, Alexei Svitkine wrote: > LGTM, thanks for checking performance. > > Can you also file a crbug to track this CL and set it to M37? (Makes it easier > to track for what release something like this landed.) Done. Thanks!
The CQ bit was checked by rch@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rch@chromium.org/307743003/150001
Message was sent while issue was closed.
Change committed as 274021 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
