Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2497753002:
Invert host/domain checks in SameDomainOrHost (Closed)
|
Created:
4 years, 1 month ago by Charlie Harrison Modified:
4 years, 1 month ago Reviewers:
Peter Kasting CC:
chromium-reviews, cbentzel+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInvert host/domain checks in SameDomainOrHost
Checking for domains in the DAFSA is slow. Let's check if hosts are
identical first as a quick fast path for URLs with the same host.
The lookup in the fixed string set takes ~10% of
RDHI::ContinuePendingBeginRequest and ~7.5% of URLRequest::Start.
BUG=664174
Committed: https://crrev.com/11d71cb74a864e5459c97a701a1e2f8eadb694c8
Cr-Commit-Position: refs/heads/master@{#432047}
Patch Set 1 #
Total comments: 1
Patch Set 2 : pkasting review #
Total comments: 2
Messages
Total messages: 20 (10 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
csharrison@chromium.org changed reviewers: + pkasting@chromium.org
pkasting, PTAL. AFAICT semantics are completely preserved with this inversion.
Note: this code is rather subtle, so I'm not 100% confident there are no edge cases here. The input to the DAFSA is the host_piece of the url. If the hosts are identical, then the results from the DAFSA will be identical. The only gotcha I can see is that the host matching logic uses possibly_invalid_spec, which may result in a different matching scheme than gurl1.host_piece() == gurl2.host_piece(). In any, I appreciate any feedback here.
https://codereview.chromium.org/2497753002/diff/1/net/base/registry_controlle... File net/base/registry_controlled_domains/registry_controlled_domain.cc (right): https://codereview.chromium.org/2497753002/diff/1/net/base/registry_controlle... net/base/registry_controlled_domains/registry_controlled_domain.cc:349: return false; I think a slightly faster, and hopefully more readable, implementation of this function would be: // Quickly reject cases where either host is empty. if (!gurl1.has_host() || !gurl2.has_host()) return false; // Check for exact host matches, which is faster than looking up the domain // and registry. if (gurl1.host_piece() == gurl2.host_piece()) return true; // Check for a domain and registry match. const auto domain1 = GetDomainAndRegistryAsStringPiece(gurl1, filter); return !domain1.empty() && (domain1 == GetDomainAndRegistryAsStringPiece(gurl2, filter));
To answer your concerns: possibly_invalid_spec() accesses the same thing (|spec_|) as host_piece(), so the simpler host_piece()-based comparison should be functionally identical. (See code in gurl.h.) Moving the host checks about the TLD+1 checks shouldn't be a problem because you haven't actually changed the return values for any cases, just reordered things. (I verified this.)
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...
Ah thank you for the comments. I have incorporated your suggestions into the new PS and updated some comments in the header which assumed an ordering of the checks (one of the reasons I was hesitant here). I'm guessing some of the confusion came from the use of the parsed structure, rather than the host_piece method. I assumed that there was some subtleties that the host_piece method missed, but now I am thinking this code was written before the StringPiece API in gurl was added.
LGTM Yes, I think the StringPiece APIs to GURL postdate this code. https://codereview.chromium.org/2497753002/diff/20001/net/base/registry_contr... File net/base/registry_controlled_domains/registry_controlled_domain.h (right): https://codereview.chromium.org/2497753002/diff/20001/net/base/registry_contr... net/base/registry_controlled_domains/registry_controlled_domain.h:195: // URLs. Note that this means the trailing dot, if any, must match too. This comment reordering makes me wonder if the function should be renamed SameHostOrDomain() or maybe just OnSameSite() or something. Probably that just mucks with a lot of code to little benefit, though.
https://codereview.chromium.org/2497753002/diff/20001/net/base/registry_contr... File net/base/registry_controlled_domains/registry_controlled_domain.h (right): https://codereview.chromium.org/2497753002/diff/20001/net/base/registry_contr... net/base/registry_controlled_domains/registry_controlled_domain.h:195: // URLs. Note that this means the trailing dot, if any, must match too. On 2016/11/14 23:21:03, Peter Kasting wrote: > This comment reordering makes me wonder if the function should be renamed > SameHostOrDomain() or maybe just OnSameSite() or something. > > Probably that just mucks with a lot of code to little benefit, though. Yeah I don't think the rename is necessary, though I do agree it would be slightly more accurate. The method is pretty much logical OR so I don't think a re-ordering is super confusing.
The CQ bit was unchecked by csharrison@chromium.org
The CQ bit was checked by csharrison@chromium.org
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.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Invert host/domain checks in SameDomainOrHost Checking for domains in the DAFSA is slow. Let's check if hosts are identical first as a quick fast path for URLs with the same host. The lookup in the fixed string set takes ~10% of RDHI::ContinuePendingBeginRequest and ~7.5% of URLRequest::Start. BUG=664174 ========== to ========== Invert host/domain checks in SameDomainOrHost Checking for domains in the DAFSA is slow. Let's check if hosts are identical first as a quick fast path for URLs with the same host. The lookup in the fixed string set takes ~10% of RDHI::ContinuePendingBeginRequest and ~7.5% of URLRequest::Start. BUG=664174 Committed: https://crrev.com/11d71cb74a864e5459c97a701a1e2f8eadb694c8 Cr-Commit-Position: refs/heads/master@{#432047} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/11d71cb74a864e5459c97a701a1e2f8eadb694c8 Cr-Commit-Position: refs/heads/master@{#432047} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
