Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2801303002:
[subresource_filter] Check request flags before URL pattern. (Closed)
|
Created:
3 years, 8 months ago by pkalinnikov Modified:
3 years, 8 months ago CC:
chromium-reviews, subresource-filter-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[subresource_filter] Check request flags before URL pattern.
This CL makes IndexedRuleset's request matching 30% faster by putting the rule
flags check before the URL pattern matching. Most of the candidate rules have
non-matching flags, like ElementType, and matching the flags is cheaper than
that of a URL pattern.
BUG=708458
Review-Url: https://codereview.chromium.org/2801303002
Cr-Commit-Position: refs/heads/master@{#463231}
Committed: https://chromium.googlesource.com/chromium/src/+/e64c5d36227046633521b638ffc2f0b9a903590f
Patch Set 1 #
Total comments: 10
Patch Set 2 : Address comments. #
Messages
Total messages: 31 (18 generated)
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...
Description was changed from ========== [subresource_filter] Check request flags before URL pattern. This CL makes IndexedRuleset matching 30% requests faster. BUG=708458 ========== to ========== [subresource_filter] Check request flags before URL pattern. This CL makes IndexedRuleset's request matching 30% faster. BUG=708458 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
pkalinnikov@chromium.org changed reviewers: + csharrison@chromium.org, engedy@chromium.org
PTAL. https://codereview.chromium.org/2801303002/diff/1/components/subresource_filt... File components/subresource_filter/core/common/indexed_ruleset.cc (right): https://codereview.chromium.org/2801303002/diff/1/components/subresource_filt... components/subresource_filter/core/common/indexed_ruleset.cc:408: const url::Origin& parent_origin, nit: parent_document_origin
Description was changed from ========== [subresource_filter] Check request flags before URL pattern. This CL makes IndexedRuleset's request matching 30% faster. BUG=708458 ========== to ========== [subresource_filter] Check request flags before URL pattern. This CL makes IndexedRuleset's request matching 30% faster by putting the rule flags check before the URL pattern matching. Most of the candidate rules have non-matching flags, like ElementType, and matching the flags is cheaper than that of a URL pattern. BUG=708458 ==========
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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Great optimization! LGTM % nits. https://codereview.chromium.org/2801303002/diff/1/components/subresource_filt... File components/subresource_filter/core/common/indexed_ruleset.cc (right): https://codereview.chromium.org/2801303002/diff/1/components/subresource_filt... components/subresource_filter/core/common/indexed_ruleset.cc:374: // Returns whether a request matches flags of the the URL |rule|. Considers the nit: s/the the/the specified/ https://codereview.chromium.org/2801303002/diff/1/components/subresource_filt... components/subresource_filter/core/common/indexed_ruleset.cc:375: // |element_type| of the requested resource OR |activation_type| if the resource nit: Can you please clarify exactly how the two types are taken into account? That is, based on which _UNSPECIFIED. https://codereview.chromium.org/2801303002/diff/1/components/subresource_filt... components/subresource_filter/core/common/indexed_ruleset.cc:377: // parent document. nit: To me, throughout this CL, parent document sounds a bit ambiguous. Is it the document embedding this resource, or the parent of that? Charlie, do you have an idea for a less ambiguous term?
Will try to take a look at this soon. Very busy with meetings today/yesterday. https://codereview.chromium.org/2801303002/diff/1/components/subresource_filt... File components/subresource_filter/core/common/indexed_ruleset.cc (right): https://codereview.chromium.org/2801303002/diff/1/components/subresource_filt... components/subresource_filter/core/common/indexed_ruleset.cc:377: // parent document. On 2017/04/07 14:24:56, engedy wrote: > nit: To me, throughout this CL, parent document sounds a bit ambiguous. Is it > the document embedding this resource, or the parent of that? > > Charlie, do you have an idea for a less ambiguous term? I am ok with parent_document. |document| to me would be the name for the document embedding this resource.
Will try to take a look at this soon. Very busy with meetings today/yesterday.
https://codereview.chromium.org/2801303002/diff/1/components/subresource_filt... File components/subresource_filter/core/common/indexed_ruleset.cc (right): https://codereview.chromium.org/2801303002/diff/1/components/subresource_filt... components/subresource_filter/core/common/indexed_ruleset.cc:377: // parent document. On 2017/04/07 14:33:27, Charlie Harrison wrote: > On 2017/04/07 14:24:56, engedy wrote: > > nit: To me, throughout this CL, parent document sounds a bit ambiguous. Is it > > the document embedding this resource, or the parent of that? > > > > Charlie, do you have an idea for a less ambiguous term? > > I am ok with parent_document. |document| to me would be the name for the > document embedding this resource. But that is the point, we need a good word for the document *embedding* the resource/subdocument. So, how about |document| or |embedding_document|?
On 2017/04/07 15:55:17, pkalinnikov wrote: > https://codereview.chromium.org/2801303002/diff/1/components/subresource_filt... > File components/subresource_filter/core/common/indexed_ruleset.cc (right): > > https://codereview.chromium.org/2801303002/diff/1/components/subresource_filt... > components/subresource_filter/core/common/indexed_ruleset.cc:377: // parent > document. > On 2017/04/07 14:33:27, Charlie Harrison wrote: > > On 2017/04/07 14:24:56, engedy wrote: > > > nit: To me, throughout this CL, parent document sounds a bit ambiguous. Is > it > > > the document embedding this resource, or the parent of that? > > > > > > Charlie, do you have an idea for a less ambiguous term? > > > > I am ok with parent_document. |document| to me would be the name for the > > document embedding this resource. > > But that is the point, we need a good word for the document *embedding* the > resource/subdocument. So, how about |document| or |embedding_document|? Fine with both as long as we are really talking about the document containing the element that references this resource.
https://codereview.chromium.org/2801303002/diff/1/components/subresource_filt... File components/subresource_filter/core/common/indexed_ruleset.cc (right): https://codereview.chromium.org/2801303002/diff/1/components/subresource_filt... components/subresource_filter/core/common/indexed_ruleset.cc:377: // parent document. On 2017/04/07 15:55:17, pkalinnikov wrote: > On 2017/04/07 14:33:27, Charlie Harrison wrote: > > On 2017/04/07 14:24:56, engedy wrote: > > > nit: To me, throughout this CL, parent document sounds a bit ambiguous. Is > it > > > the document embedding this resource, or the parent of that? > > > > > > Charlie, do you have an idea for a less ambiguous term? > > > > I am ok with parent_document. |document| to me would be the name for the > > document embedding this resource. > > But that is the point, we need a good word for the document *embedding* the > resource/subdocument. So, how about |document| or |embedding_document|? Ah, sorry I misread. Yes |document| or |embedding_document| is much better than |parent_document| imo.
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, thanks! PTAL again. https://codereview.chromium.org/2801303002/diff/1/components/subresource_filt... File components/subresource_filter/core/common/indexed_ruleset.cc (right): https://codereview.chromium.org/2801303002/diff/1/components/subresource_filt... components/subresource_filter/core/common/indexed_ruleset.cc:374: // Returns whether a request matches flags of the the URL |rule|. Considers the On 2017/04/07 14:24:56, engedy wrote: > nit: s/the the/the specified/ Done. https://codereview.chromium.org/2801303002/diff/1/components/subresource_filt... components/subresource_filter/core/common/indexed_ruleset.cc:375: // |element_type| of the requested resource OR |activation_type| if the resource On 2017/04/07 14:24:56, engedy wrote: > nit: Can you please clarify exactly how the two types are taken into account? > That is, based on which _UNSPECIFIED. Done. https://codereview.chromium.org/2801303002/diff/1/components/subresource_filt... components/subresource_filter/core/common/indexed_ruleset.cc:408: const url::Origin& parent_origin, On 2017/04/07 14:04:55, pkalinnikov wrote: > nit: parent_document_origin Renamed to |document_origin|.
Still LGTM, thanks!
Charlie, do you want to take another look, or I can submit?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM great work. Ship it!
The CQ bit was checked by pkalinnikov@chromium.org
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": 1491826900036500,
"parent_rev": "347411d81e89c32d4136871272677641f075e37e", "commit_rev":
"e64c5d36227046633521b638ffc2f0b9a903590f"}
Message was sent while issue was closed.
Description was changed from ========== [subresource_filter] Check request flags before URL pattern. This CL makes IndexedRuleset's request matching 30% faster by putting the rule flags check before the URL pattern matching. Most of the candidate rules have non-matching flags, like ElementType, and matching the flags is cheaper than that of a URL pattern. BUG=708458 ========== to ========== [subresource_filter] Check request flags before URL pattern. This CL makes IndexedRuleset's request matching 30% faster by putting the rule flags check before the URL pattern matching. Most of the candidate rules have non-matching flags, like ElementType, and matching the flags is cheaper than that of a URL pattern. BUG=708458 Review-Url: https://codereview.chromium.org/2801303002 Cr-Commit-Position: refs/heads/master@{#463231} Committed: https://chromium.googlesource.com/chromium/src/+/e64c5d36227046633521b638ffc2... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/e64c5d36227046633521b638ffc2... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
