Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2288023003:
Cache is_third_party requests in a one-entry cache. (Closed)
|
Created:
4 years, 3 months ago by pkalinnikov Modified:
4 years, 2 months ago Reviewers:
engedy CC:
chromium-reviews, mlamouri+watch-content_chromium.org, jam, darin-cc_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCache is_third_party requests in a one-entry cache.
For each request to IndexedRuleset::ShouldDisallowResourceLoad it needs to be
determined whether the requested resource is third-party in respect to the
document. This is accomplished by querying a pretty heavy operation,
registry_controlled_domains::SameDomainOrHost.
According to some ad-hoc measurements on top Alexa sites, about 70 percent of
network requests came from the same domain as the previous request. Therefore,
it makes perfect sense to cache requests to registry_controlled_domains in order
to improve performance.
A simple one-entry cache is implemented in this CL.
BUG=609747
Committed: https://crrev.com/3dd16db91769ac33489a5e6e3a68a9c532cd360a
Cr-Commit-Position: refs/heads/master@{#425690}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Rename IsThirdPartyCache to FirstPartyOrigin and refactor it. #Patch Set 3 : Clean up. #Patch Set 4 : Add DCHECK. #
Total comments: 27
Patch Set 5 : Address comments from engedy@ #Patch Set 6 : Rebase. #Patch Set 7 : Modified comment. #
Total comments: 6
Patch Set 8 : Address more comments from engedy@ #Messages
Total messages: 45 (32 generated)
pkalinnikov@chromium.org changed reviewers: + engedy@chromium.org
Hey Balazs, I'm adding you early, so that you can verify the approach. Also, it's not clear right now whether we should apply the same optimization to ShouldDisableFilteringForDocument, because the pattern of its usage is different.
Please some initial comments. https://codereview.chromium.org/2288023003/diff/1/components/subresource_filt... File components/subresource_filter/content/renderer/document_subresource_filter.cc (right): https://codereview.chromium.org/2288023003/diff/1/components/subresource_filt... components/subresource_filter/content/renderer/document_subresource_filter.cc:91: is_third_party_cache_.SetDocumentOrigin(document_origin_); nit: How about making this a unique_ptr<...>, passing the document origin in the ctor, and making it immutable for the lifetime of the instance? https://codereview.chromium.org/2288023003/diff/1/components/subresource_filt... components/subresource_filter/content/renderer/document_subresource_filter.cc:104: // TODO(pkalinnikov): Think about avoiding this conversion. This should be rather fast, and we only do it once. As long as there are no reasons other than performance, this should not be an issue. https://codereview.chromium.org/2288023003/diff/1/components/subresource_filt... File components/subresource_filter/core/common/indexed_ruleset.cc (right): https://codereview.chromium.org/2288023003/diff/1/components/subresource_filt... components/subresource_filter/core/common/indexed_ruleset.cc:460: const url::Origin& document_origin, Do you think it would make sense to no longer pass |document_origin| here separately, but to extend the responsibility of this cache, and instead name the class Initiator/FirstPartyOrigin/etc., and let it have the methods DomainIs(domain) and IsThirdParty(url)?
Please *see* ...
Balazs, please take a closer look. https://codereview.chromium.org/2288023003/diff/1/components/subresource_filt... File components/subresource_filter/content/renderer/document_subresource_filter.cc (right): https://codereview.chromium.org/2288023003/diff/1/components/subresource_filt... components/subresource_filter/content/renderer/document_subresource_filter.cc:91: is_third_party_cache_.SetDocumentOrigin(document_origin_); On 2016/08/29 16:33:04, engedy wrote: > nit: How about making this a unique_ptr<...>, passing the document origin in the > ctor, and making it immutable for the lifetime of the instance? Done. https://codereview.chromium.org/2288023003/diff/1/components/subresource_filt... components/subresource_filter/content/renderer/document_subresource_filter.cc:104: // TODO(pkalinnikov): Think about avoiding this conversion. On 2016/08/29 16:33:03, engedy wrote: > This should be rather fast, and we only do it once. As long as there are no > reasons other than performance, this should not be an issue. Do you suggest to remove the TODO? I still find this a bit ridiculous, that we convert URLs back and forth. You see, |parent_document_origin gets| get converted back to GURL in ShouldDisableFilteringForDocument when checking if it's a third party. Either we should use GURL instead, or refactor !net::registry_controlled_domains::SameDomainOrHost to accept parameters of different types. https://codereview.chromium.org/2288023003/diff/1/components/subresource_filt... File components/subresource_filter/core/common/indexed_ruleset.cc (right): https://codereview.chromium.org/2288023003/diff/1/components/subresource_filt... components/subresource_filter/core/common/indexed_ruleset.cc:460: const url::Origin& document_origin, On 2016/08/29 16:33:04, engedy wrote: > Do you think it would make sense to no longer pass |document_origin| here > separately, but to extend the responsibility of this cache, and instead name the > class Initiator/FirstPartyOrigin/etc., and let it have the methods > DomainIs(domain) and IsThirdParty(url)? Good idea. I renamed the class to FirstPartyOrigin, and now it's passed in here alone. However, I don't add DomainIs for 2 reasons: 1. I don't want to duplicate url::Origin's methods. 2. There is ShouldDisableFilteringForDocument above, which doesn't use FirstPartyOrigin (and it won't, because FirstPartyOrigin is slightly heavier), but it still needs to call IsMatch function below. IsMatch has 2 "clients", and the GCD of the 2 origin classes used by these clients is url::Origin.
Description was changed from ========== Add IsThirdPartyCache to the RulesetMatcher. BUG=609747 ========== to ========== Cache is_third_party requests in a one-entry cache. For each request to IndexedRuleset::ShouldDisallowResourceLoad it needs to be determined whether the requested resource is third-party in respect to the document. This is accomplished by querying a pretty heavy operation, registry_controlled_domains::SameDomainOrHost. According to some ad-hoc measurements on top Alexa sites + couple of news articles, about 70 percent of network requests come from the same domain as the previous request. Therefore, it makes perfect sense to cache requests to registry_controlled_domains in order to improve performance. A simple one-entry cache is implemented in this CL. BUG=609747 ==========
Description was changed from ========== Cache is_third_party requests in a one-entry cache. For each request to IndexedRuleset::ShouldDisallowResourceLoad it needs to be determined whether the requested resource is third-party in respect to the document. This is accomplished by querying a pretty heavy operation, registry_controlled_domains::SameDomainOrHost. According to some ad-hoc measurements on top Alexa sites + couple of news articles, about 70 percent of network requests come from the same domain as the previous request. Therefore, it makes perfect sense to cache requests to registry_controlled_domains in order to improve performance. A simple one-entry cache is implemented in this CL. BUG=609747 ========== to ========== Cache is_third_party requests in a one-entry cache. For each request to IndexedRuleset::ShouldDisallowResourceLoad it needs to be determined whether the requested resource is third-party in respect to the document. This is accomplished by querying a pretty heavy operation, registry_controlled_domains::SameDomainOrHost. According to some ad-hoc measurements on top Alexa sites + couple of news articles, about 70 percent of network requests came from the same domain as the previous request. Therefore, it makes perfect sense to cache requests to registry_controlled_domains in order to improve performance. A simple one-entry cache is implemented in this CL. BUG=609747 ==========
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM % comments. Thanks! https://codereview.chromium.org/2288023003/diff/1/components/subresource_filt... File components/subresource_filter/content/renderer/document_subresource_filter.cc (right): https://codereview.chromium.org/2288023003/diff/1/components/subresource_filt... components/subresource_filter/content/renderer/document_subresource_filter.cc:104: // TODO(pkalinnikov): Think about avoiding this conversion. On 2016/09/28 11:32:18, pkalinnikov wrote: > On 2016/08/29 16:33:03, engedy wrote: > > This should be rather fast, and we only do it once. As long as there are no > > reasons other than performance, this should not be an issue. > > Do you suggest to remove the TODO? > > I still find this a bit ridiculous, that we convert URLs back and forth. You > see, |parent_document_origin gets| get converted back to GURL in > ShouldDisableFilteringForDocument when checking if it's a third party. Either we > should use GURL instead, or refactor > !net::registry_controlled_domains::SameDomainOrHost to accept parameters of > different types. You are right. Let's keep the TODO. As discussed, we will need to do quite a bit of refactoring here anyway. The criteria are: -- we will need to propagate a `enabled` bit from the parent frame, -- we will only have the url::Origin of all ancestors, -- we will have the GURL of the current document, -- conversions should indeed be avoided as much as possible. https://codereview.chromium.org/2288023003/diff/1/components/subresource_filt... File components/subresource_filter/core/common/indexed_ruleset.cc (right): https://codereview.chromium.org/2288023003/diff/1/components/subresource_filt... components/subresource_filter/core/common/indexed_ruleset.cc:460: const url::Origin& document_origin, On 2016/09/28 11:32:18, pkalinnikov wrote: > On 2016/08/29 16:33:04, engedy wrote: > > Do you think it would make sense to no longer pass |document_origin| here > > separately, but to extend the responsibility of this cache, and instead name > the > > class Initiator/FirstPartyOrigin/etc., and let it have the methods > > DomainIs(domain) and IsThirdParty(url)? > > Good idea. I renamed the class to FirstPartyOrigin, and now it's passed in here > alone. > > However, I don't add DomainIs for 2 reasons: > 1. I don't want to duplicate url::Origin's methods. > 2. There is ShouldDisableFilteringForDocument above, which doesn't use > FirstPartyOrigin (and it won't, because FirstPartyOrigin is slightly heavier), > but it still needs to call IsMatch function below. IsMatch has 2 "clients", and > the GCD of the 2 origin classes used by these clients is url::Origin. Good point. https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... File components/subresource_filter/content/renderer/document_subresource_filter.cc (right): https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.cc:103: new FirstPartyOrigin(std::move(parent_document_origin))); nit: This is quite misleading when read. Please either recalculate this argument value, or assign this first to a variable named |document_origin| to indicate that is actually not the parent document origin. :o) https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... File components/subresource_filter/content/renderer/document_subresource_filter.h (right): https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.h:68: std::unique_ptr<FirstPartyOrigin> document_first_party_origin_; Let's name this either |document_origin_| or |first_party_origin_|, but not both. https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... File components/subresource_filter/core/common/first_party_origin.cc (right): https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... components/subresource_filter/core/common/first_party_origin.cc:32: cached_host_.assign(url.host_piece().begin(), url.host_piece().end()); nit: Use base::StringPiece::CopyToString. It essentially does exactly this. https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... File components/subresource_filter/core/common/first_party_origin.h (right): https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... components/subresource_filter/core/common/first_party_origin.h:15: // Represents a first party Origin. This class is intended to handle requests on Comment nit: Encapsulates the first-party origin of a document, and provides fast (cached) third-partiness checks against that origin, i.e. whether a given URL is third-party in relation to the document's origin. It uses a simple one-entry cache to optimize for the case when a significant number of consecutive URLs have the same domain. https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... components/subresource_filter/core/common/first_party_origin.h:18: // amount of subsequent URL requests have the same domain. nit: s/subsequent/consecutive/ https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... components/subresource_filter/core/common/first_party_origin.h:21: // Creates a FirstPartyOrigin associated with the provided |origin|. nit: Would remove comment, as it does not add much information, and maybe rename parameter to |document_origin| to be very clear. https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... components/subresource_filter/core/common/first_party_origin.h:26: // Returns whether |url| is a third party in respect to |this| origin. May nit: third-party in relation to https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... components/subresource_filter/core/common/first_party_origin.h:35: url::Origin document_origin_; nit: blank line after https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... components/subresource_filter/core/common/first_party_origin.h:37: // net::registry_controlled_domains::SameDomainOrHost. Get rid of it once the Given the caching, we already somewhat intrude into the internal details of how SameDomainOrHost() works, in the sense that we expect it to return the same result if one URL and the host string of the other URL is the same. E.g., it could have worked in a way that distinguishes between an empty host and a non-existent host. The only thing SameDomainOrHost() provides on top of the obvious is handling the edge-case when the host is weird/non-existent. Can we just do that ourselves? Otherwise I'm not sure how we could refactor net::registry_controlled_domains in a way that it still takes care of this edge case for us, yet allows for good performance. Open to suggestions, though. https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... components/subresource_filter/core/common/first_party_origin.h:41: // One-entry cache stores the result of IsThirdParty call for the Comment nit: One-entry cache that stores input/output of the last IsThirdParty call. https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... components/subresource_filter/core/common/first_party_origin.h:43: mutable std::string cached_host_; nit: How about: last_checked_host_, last_checked_host_was_third_party_? https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... File components/subresource_filter/core/common/first_party_origin_unittest.cc (right): https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... components/subresource_filter/core/common/first_party_origin_unittest.cc:16: for (int index = 0; index < 10; ++index) { Should 3 be enough instead of 10? https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... components/subresource_filter/core/common/first_party_origin_unittest.cc:46: FirstPartyOrigin first_party(url::Origin(GURL("https://example.com"))); nit: Please move this below the constant definitions, just above the loop below, for consistency.
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Addressed all but one comment regarding registry_controlled_domains. Working on the latter. https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... File components/subresource_filter/content/renderer/document_subresource_filter.cc (right): https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.cc:103: new FirstPartyOrigin(std::move(parent_document_origin))); On 2016/09/28 13:14:26, engedy wrote: > nit: This is quite misleading when read. Please either recalculate this > argument value, or assign this first to a variable named |document_origin| to > indicate that is actually not the parent document origin. :o) How about this? https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... File components/subresource_filter/content/renderer/document_subresource_filter.h (right): https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.h:68: std::unique_ptr<FirstPartyOrigin> document_first_party_origin_; On 2016/09/28 13:14:26, engedy wrote: > Let's name this either |document_origin_| or |first_party_origin_|, but not > both. Done. https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... File components/subresource_filter/core/common/first_party_origin.cc (right): https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... components/subresource_filter/core/common/first_party_origin.cc:32: cached_host_.assign(url.host_piece().begin(), url.host_piece().end()); On 2016/09/28 13:14:26, engedy wrote: > nit: Use base::StringPiece::CopyToString. It essentially does exactly this. Done. https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... File components/subresource_filter/core/common/first_party_origin.h (right): https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... components/subresource_filter/core/common/first_party_origin.h:15: // Represents a first party Origin. This class is intended to handle requests on On 2016/09/28 13:14:26, engedy wrote: > Comment nit: > > Encapsulates the first-party origin of a document, and provides fast (cached) > third-partiness checks against that origin, i.e. whether a given URL is > third-party in relation to the document's origin. > > It uses a simple one-entry cache to optimize for the case when a significant > number of consecutive URLs have the same domain. Done. https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... components/subresource_filter/core/common/first_party_origin.h:18: // amount of subsequent URL requests have the same domain. On 2016/09/28 13:14:26, engedy wrote: > nit: s/subsequent/consecutive/ Done. https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... components/subresource_filter/core/common/first_party_origin.h:21: // Creates a FirstPartyOrigin associated with the provided |origin|. On 2016/09/28 13:14:26, engedy wrote: > nit: Would remove comment, as it does not add much information, and maybe rename > parameter to |document_origin| to be very clear. Done. https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... components/subresource_filter/core/common/first_party_origin.h:26: // Returns whether |url| is a third party in respect to |this| origin. May On 2016/09/28 13:14:26, engedy wrote: > nit: third-party in relation to Done. https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... components/subresource_filter/core/common/first_party_origin.h:35: url::Origin document_origin_; On 2016/09/28 13:14:26, engedy wrote: > nit: blank line after Done. https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... components/subresource_filter/core/common/first_party_origin.h:41: // One-entry cache stores the result of IsThirdParty call for the On 2016/09/28 13:14:26, engedy wrote: > Comment nit: One-entry cache that stores input/output of the last IsThirdParty > call. Done. https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... components/subresource_filter/core/common/first_party_origin.h:43: mutable std::string cached_host_; On 2016/09/28 13:14:26, engedy wrote: > nit: How about: last_checked_host_, last_checked_host_was_third_party_? Done. https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... File components/subresource_filter/core/common/first_party_origin_unittest.cc (right): https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... components/subresource_filter/core/common/first_party_origin_unittest.cc:16: for (int index = 0; index < 10; ++index) { On 2016/09/28 13:14:26, engedy wrote: > Should 3 be enough instead of 10? Let's negotiate to 5 :) https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... components/subresource_filter/core/common/first_party_origin_unittest.cc:46: FirstPartyOrigin first_party(url::Origin(GURL("https://example.com"))); On 2016/09/28 13:14:26, engedy wrote: > nit: Please move this below the constant definitions, just above the loop below, > for consistency. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... File components/subresource_filter/content/renderer/document_subresource_filter.cc (right): https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.cc:103: new FirstPartyOrigin(std::move(parent_document_origin))); On 2016/09/28 14:15:52, pkalinnikov wrote: > On 2016/09/28 13:14:26, engedy wrote: > > nit: This is quite misleading when read. Please either recalculate this > > argument value, or assign this first to a variable named |document_origin| to > > indicate that is actually not the parent document origin. :o) > > How about this? Looks good.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Please take one more look. Can I submit? https://codereview.chromium.org/2288023003/diff/1/components/subresource_filt... File components/subresource_filter/content/renderer/document_subresource_filter.cc (right): https://codereview.chromium.org/2288023003/diff/1/components/subresource_filt... components/subresource_filter/content/renderer/document_subresource_filter.cc:104: // TODO(pkalinnikov): Think about avoiding this conversion. On 2016/09/28 13:14:26, engedy wrote: > On 2016/09/28 11:32:18, pkalinnikov wrote: > > On 2016/08/29 16:33:03, engedy wrote: > > > This should be rather fast, and we only do it once. As long as there are no > > > reasons other than performance, this should not be an issue. > > > > Do you suggest to remove the TODO? > > > > I still find this a bit ridiculous, that we convert URLs back and forth. You > > see, |parent_document_origin gets| get converted back to GURL in > > ShouldDisableFilteringForDocument when checking if it's a third party. Either > we > > should use GURL instead, or refactor > > !net::registry_controlled_domains::SameDomainOrHost to accept parameters of > > different types. > > You are right. Let's keep the TODO. As discussed, we will need to do quite a > bit of refactoring here anyway. The criteria are: > -- we will need to propagate a `enabled` bit from the parent frame, > -- we will only have the url::Origin of all ancestors, > -- we will have the GURL of the current document, > -- conversions should indeed be avoided as much as possible. Acknowledged. https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... File components/subresource_filter/core/common/first_party_origin.h (right): https://codereview.chromium.org/2288023003/diff/60001/components/subresource_... components/subresource_filter/core/common/first_party_origin.h:37: // net::registry_controlled_domains::SameDomainOrHost. Get rid of it once the On 2016/09/28 13:14:26, engedy wrote: > Given the caching, we already somewhat intrude into the internal details of how > SameDomainOrHost() works, in the sense that we expect it to return the same > result if one URL and the host string of the other URL is the same. E.g., it > could have worked in a way that distinguishes between an empty host and a > non-existent host. > > The only thing SameDomainOrHost() provides on top of the obvious is handling the > edge-case when the host is weird/non-existent. Can we just do that ourselves? > Otherwise I'm not sure how we could refactor net::registry_controlled_domains in > a way that it still takes care of this edge case for us, yet allows for good > performance. Open to suggestions, though. I changed comments here and there. Regarding the registry_controlled_domains refactoring, there is a (one of the) follow-up CL for that: https://codereview.chromium.org/2393163002/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks very nice, LGTM % nits. Thanks for doing this refactoring! https://codereview.chromium.org/2288023003/diff/120001/components/subresource... File components/subresource_filter/core/common/first_party_origin.cc (right): https://codereview.chromium.org/2288023003/diff/120001/components/subresource... components/subresource_filter/core/common/first_party_origin.cc:29: if (url.host_piece() == last_checked_host_) Let's not use the cache if |last_checked_host_| is empty, so that we are formally correct all the time. https://codereview.chromium.org/2288023003/diff/120001/components/subresource... File components/subresource_filter/core/common/first_party_origin.h (right): https://codereview.chromium.org/2288023003/diff/120001/components/subresource... components/subresource_filter/core/common/first_party_origin.h:45: // NOTE: registry_controlled_domains::SameDomainOrHost take GURLs as inputs, nit: takes https://codereview.chromium.org/2288023003/diff/120001/components/subresource... File components/subresource_filter/core/common/first_party_origin_unittest.cc (right): https://codereview.chromium.org/2288023003/diff/120001/components/subresource... components/subresource_filter/core/common/first_party_origin_unittest.cc:45: TEST(FirstPartyOriginTest, MixedFirstAndThirdParties) { Could you please add a unit test to cover URLs that do not have a host? E.g., data:// URLs.
https://codereview.chromium.org/2288023003/diff/120001/components/subresource... File components/subresource_filter/core/common/first_party_origin.cc (right): https://codereview.chromium.org/2288023003/diff/120001/components/subresource... components/subresource_filter/core/common/first_party_origin.cc:29: if (url.host_piece() == last_checked_host_) On 2016/10/17 13:04:33, engedy wrote: > Let's not use the cache if |last_checked_host_| is empty, so that we are > formally correct all the time. Done. https://codereview.chromium.org/2288023003/diff/120001/components/subresource... File components/subresource_filter/core/common/first_party_origin.h (right): https://codereview.chromium.org/2288023003/diff/120001/components/subresource... components/subresource_filter/core/common/first_party_origin.h:45: // NOTE: registry_controlled_domains::SameDomainOrHost take GURLs as inputs, On 2016/10/17 13:04:33, engedy wrote: > nit: takes Done. https://codereview.chromium.org/2288023003/diff/120001/components/subresource... File components/subresource_filter/core/common/first_party_origin_unittest.cc (right): https://codereview.chromium.org/2288023003/diff/120001/components/subresource... components/subresource_filter/core/common/first_party_origin_unittest.cc:45: TEST(FirstPartyOriginTest, MixedFirstAndThirdParties) { On 2016/10/17 13:04:33, engedy wrote: > Could you please add a unit test to cover URLs that do not have a host? E.g., > data:// URLs. Done.
Description was changed from ========== Cache is_third_party requests in a one-entry cache. For each request to IndexedRuleset::ShouldDisallowResourceLoad it needs to be determined whether the requested resource is third-party in respect to the document. This is accomplished by querying a pretty heavy operation, registry_controlled_domains::SameDomainOrHost. According to some ad-hoc measurements on top Alexa sites + couple of news articles, about 70 percent of network requests came from the same domain as the previous request. Therefore, it makes perfect sense to cache requests to registry_controlled_domains in order to improve performance. A simple one-entry cache is implemented in this CL. BUG=609747 ========== to ========== Cache is_third_party requests in a one-entry cache. For each request to IndexedRuleset::ShouldDisallowResourceLoad it needs to be determined whether the requested resource is third-party in respect to the document. This is accomplished by querying a pretty heavy operation, registry_controlled_domains::SameDomainOrHost. According to some ad-hoc measurements on top Alexa sites, about 70 percent of network requests came from the same domain as the previous request. Therefore, it makes perfect sense to cache requests to registry_controlled_domains in order to improve performance. A simple one-entry cache is implemented in this CL. BUG=609747 ==========
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by pkalinnikov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org Link to the patchset: https://codereview.chromium.org/2288023003/#ps140001 (title: "Address more comments from engedy@")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Cache is_third_party requests in a one-entry cache. For each request to IndexedRuleset::ShouldDisallowResourceLoad it needs to be determined whether the requested resource is third-party in respect to the document. This is accomplished by querying a pretty heavy operation, registry_controlled_domains::SameDomainOrHost. According to some ad-hoc measurements on top Alexa sites, about 70 percent of network requests came from the same domain as the previous request. Therefore, it makes perfect sense to cache requests to registry_controlled_domains in order to improve performance. A simple one-entry cache is implemented in this CL. BUG=609747 ========== to ========== Cache is_third_party requests in a one-entry cache. For each request to IndexedRuleset::ShouldDisallowResourceLoad it needs to be determined whether the requested resource is third-party in respect to the document. This is accomplished by querying a pretty heavy operation, registry_controlled_domains::SameDomainOrHost. According to some ad-hoc measurements on top Alexa sites, about 70 percent of network requests came from the same domain as the previous request. Therefore, it makes perfect sense to cache requests to registry_controlled_domains in order to improve performance. A simple one-entry cache is implemented in this CL. BUG=609747 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Cache is_third_party requests in a one-entry cache. For each request to IndexedRuleset::ShouldDisallowResourceLoad it needs to be determined whether the requested resource is third-party in respect to the document. This is accomplished by querying a pretty heavy operation, registry_controlled_domains::SameDomainOrHost. According to some ad-hoc measurements on top Alexa sites, about 70 percent of network requests came from the same domain as the previous request. Therefore, it makes perfect sense to cache requests to registry_controlled_domains in order to improve performance. A simple one-entry cache is implemented in this CL. BUG=609747 ========== to ========== Cache is_third_party requests in a one-entry cache. For each request to IndexedRuleset::ShouldDisallowResourceLoad it needs to be determined whether the requested resource is third-party in respect to the document. This is accomplished by querying a pretty heavy operation, registry_controlled_domains::SameDomainOrHost. According to some ad-hoc measurements on top Alexa sites, about 70 percent of network requests came from the same domain as the previous request. Therefore, it makes perfect sense to cache requests to registry_controlled_domains in order to improve performance. A simple one-entry cache is implemented in this CL. BUG=609747 Committed: https://crrev.com/3dd16db91769ac33489a5e6e3a68a9c532cd360a Cr-Commit-Position: refs/heads/master@{#425690} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/3dd16db91769ac33489a5e6e3a68a9c532cd360a Cr-Commit-Position: refs/heads/master@{#425690} |
