Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2668213002:
Remove Origin->GURL conversion in DocumentSubresourceFilter (Closed)
|
Created:
3 years, 10 months ago by Charlie Harrison Modified:
3 years, 10 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove Origin->GURL conversion in DocumentSubresourceFilter
SameDomainOrHost was recently changed [1] to allow for comparing Origins
and GURLs, so we should remove the extra URL parsing here, which is a very
expensive operation.
[1] https://codereview.chromium.org/2568133007/
BUG=651554
Review-Url: https://codereview.chromium.org/2668213002
Cr-Commit-Position: refs/heads/master@{#447576}
Committed: https://chromium.googlesource.com/chromium/src/+/37f125e437377ad9b3b988edac21075c75ec5432
Patch Set 1 #
Total comments: 5
Patch Set 2 : reviews (std::string::assign + comment) #
Messages
Total messages: 22 (16 generated)
The CQ bit was checked by csharrison@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...
Description was changed from ========== Remove Origin->GURL conversion in DocumentSubresourceFilter SameDomainOrHost was recently changed [1] to allow for comparing Origins and GURLs. [1] https://codereview.chromium.org/2568133007/ BUG=none ========== to ========== Remove Origin->GURL conversion in DocumentSubresourceFilter SameDomainOrHost was recently changed [1] to allow for comparing Origins and GURLs, so we should remove the extra URL parsing here. [1] https://codereview.chromium.org/2568133007/ BUG=none ==========
Description was changed from ========== Remove Origin->GURL conversion in DocumentSubresourceFilter SameDomainOrHost was recently changed [1] to allow for comparing Origins and GURLs, so we should remove the extra URL parsing here. [1] https://codereview.chromium.org/2568133007/ BUG=none ========== to ========== Remove Origin->GURL conversion in DocumentSubresourceFilter SameDomainOrHost was recently changed [1] to allow for comparing Origins and GURLs, so we should remove the extra URL parsing here, which is a very expensive operation. [1] https://codereview.chromium.org/2568133007/ BUG=651554 ==========
csharrison@chromium.org changed reviewers: + engedy@chromium.org
Hi engedy, I am just starting to look at this code and I found this inefficiency. PTAL?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
engedy@chromium.org changed reviewers: + pkalinnikov@chromium.org - engedy@chromium.org
engedy@chromium.org changed reviewers: + engedy@chromium.org
LGTM % a phrasing suggestion. @Pavel, you had some plans to improve performance around here. Could you please take a quick look and confirm that this change coherent with those efforts? https://codereview.chromium.org/2668213002/diff/1/components/subresource_filt... File components/subresource_filter/core/common/first_party_origin.h (right): https://codereview.chromium.org/2668213002/diff/1/components/subresource_filt... components/subresource_filter/core/common/first_party_origin.h:41: // NOTE: registry_controlled_domains::SameDomainOrHost takes GURL/Origin as Phrasing suggestion: To improve performance, the cache stores only the host part of the last checked URL. Checking only the host part before returning the cached result is correct, because registry_controlled_domains::SameDomainOrHost would return the same result for any GURL that has the same non-empty host part as the last checked URL.
LGTM % comments. This is very relevant, thank you for fixing the TODO. https://codereview.chromium.org/2668213002/diff/1/components/subresource_filt... File components/subresource_filter/core/common/first_party_origin.cc (right): https://codereview.chromium.org/2668213002/diff/1/components/subresource_filt... components/subresource_filter/core/common/first_party_origin.cc:30: last_checked_host_ = url.host(); I would use string's assign(ptr, size) on url.host_piece(), because in many cases it would prevent a memory allocation (i.e., when url.host_piece() is not longer than |last_checked_host_|) which otherwise happens every time in url.host(). https://codereview.chromium.org/2668213002/diff/1/components/subresource_filt... File components/subresource_filter/core/common/first_party_origin.h (right): https://codereview.chromium.org/2668213002/diff/1/components/subresource_filt... components/subresource_filter/core/common/first_party_origin.h:41: // NOTE: registry_controlled_domains::SameDomainOrHost takes GURL/Origin as On 2017/02/01 09:49:50, engedy wrote: > Phrasing suggestion: > > To improve performance, the cache stores only the host part of the last checked > URL. Checking only the host part before returning the cached result is correct, > because registry_controlled_domains::SameDomainOrHost would return the same > result for any GURL that has the same non-empty host part as the last checked > URL. +1
The CQ bit was checked by csharrison@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...
Thanks!! https://codereview.chromium.org/2668213002/diff/1/components/subresource_filt... File components/subresource_filter/core/common/first_party_origin.cc (right): https://codereview.chromium.org/2668213002/diff/1/components/subresource_filt... components/subresource_filter/core/common/first_party_origin.cc:30: last_checked_host_ = url.host(); On 2017/02/01 10:16:39, pkalinnikov wrote: > I would use string's assign(ptr, size) on url.host_piece(), because in many > cases it would prevent a memory allocation (i.e., when url.host_piece() is not > longer than |last_checked_host_|) which otherwise happens every time in > url.host(). Done. Pulled out the base::StringPiece into a local to avoid so many method calls. https://codereview.chromium.org/2668213002/diff/1/components/subresource_filt... File components/subresource_filter/core/common/first_party_origin.h (right): https://codereview.chromium.org/2668213002/diff/1/components/subresource_filt... components/subresource_filter/core/common/first_party_origin.h:41: // NOTE: registry_controlled_domains::SameDomainOrHost takes GURL/Origin as On 2017/02/01 09:49:50, engedy wrote: > Phrasing suggestion: > > To improve performance, the cache stores only the host part of the last checked > URL. Checking only the host part before returning the cached result is correct, > because registry_controlled_domains::SameDomainOrHost would return the same > result for any GURL that has the same non-empty host part as the last checked > URL. Done. Thanks!
The CQ bit was unchecked by csharrison@chromium.org
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org, pkalinnikov@chromium.org Link to the patchset: https://codereview.chromium.org/2668213002/#ps20001 (title: "reviews (std::string::assign + comment)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1485972692579230,
"parent_rev": "3249c30a2d284c39241d2e7371656fe79c825669", "commit_rev":
"37f125e437377ad9b3b988edac21075c75ec5432"}
Message was sent while issue was closed.
Description was changed from ========== Remove Origin->GURL conversion in DocumentSubresourceFilter SameDomainOrHost was recently changed [1] to allow for comparing Origins and GURLs, so we should remove the extra URL parsing here, which is a very expensive operation. [1] https://codereview.chromium.org/2568133007/ BUG=651554 ========== to ========== Remove Origin->GURL conversion in DocumentSubresourceFilter SameDomainOrHost was recently changed [1] to allow for comparing Origins and GURLs, so we should remove the extra URL parsing here, which is a very expensive operation. [1] https://codereview.chromium.org/2568133007/ BUG=651554 Review-Url: https://codereview.chromium.org/2668213002 Cr-Commit-Position: refs/heads/master@{#447576} Committed: https://chromium.googlesource.com/chromium/src/+/37f125e437377ad9b3b988edac21... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/37f125e437377ad9b3b988edac21... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
