Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2010713003:
Only call CanonicalizeUrl once from UrlToFullHashes (Closed)
|
Created:
4 years, 7 months ago by Joe Mason Modified:
4 years, 7 months ago Reviewers:
Nathan Parker CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOnly call CanonicalizeUrl once from UrlToFullHashes
BUG=606022
Committed: https://crrev.com/e625cb78079a7c15a944cff3fe89b28a3779b697
Cr-Commit-Position: refs/heads/master@{#396035}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Rename GenerateHostEtc #Messages
Total messages: 15 (5 generated)
joenotcharles@chromium.org changed reviewers: + nparker@chromium.org
PTAL https://codereview.chromium.org/2010713003/diff/1/components/safe_browsing_db... File components/safe_browsing_db/util.cc (left): https://codereview.chromium.org/2010713003/diff/1/components/safe_browsing_db... components/safe_browsing_db/util.cc:353: const std::string host = canon_host; // const sidesteps GCC bugs below! This note was added in commit 080438b8886070e399c326f757cd96976a7a81ec in 2008. I think it's safe to remove now, especially as the new code passes the host through a const reference param, so if the GCC bug in question still exists it probably wouldn't triggered anymore.
This seems reasonable, but do you know how much time this saves? I'd rather not trade performance for complexity w/o perf numbers. Thanks https://codereview.chromium.org/2010713003/diff/1/components/safe_browsing_db... File components/safe_browsing_db/util.cc (right): https://codereview.chromium.org/2010713003/diff/1/components/safe_browsing_db... components/safe_browsing_db/util.cc:32: void GenerateHostsToCheck(const std::string& host, Give these a different name, so they differ by more than signature. Maybe GenerateHostsToCheckFromHost and FromParts or some such
On 2016/05/25 17:23:35, Nathan Parker wrote: > This seems reasonable, but do you know how much time this saves? I'd rather not > trade performance for complexity w/o perf numbers. Thanks On my machine each call to CanonicalizeUrl takes between 0.016 ms and 0.020 ms. Someone commented on the bug that an average page has 350 resources, so with that much saving per resource this would save 5.6 ms to 7.0 ms per page load.
https://codereview.chromium.org/2010713003/diff/1/components/safe_browsing_db... File components/safe_browsing_db/util.cc (right): https://codereview.chromium.org/2010713003/diff/1/components/safe_browsing_db... components/safe_browsing_db/util.cc:32: void GenerateHostsToCheck(const std::string& host, On 2016/05/25 17:23:35, Nathan Parker wrote: > Give these a different name, so they differ by more than signature. Maybe > GenerateHostsToCheckFromHost and FromParts or some such Done.
lgtm 5-7ms per page load seems worthwhile, though that's CPU, not wallclock time. It's probably <<1ms of actual delay since many resources load in parallel.
The CQ bit was checked by joenotcharles@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2010713003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2010713003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by joenotcharles@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2010713003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2010713003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Only call CanonicalizeUrl once from UrlToFullHashes BUG=606022 ========== to ========== Only call CanonicalizeUrl once from UrlToFullHashes BUG=606022 Committed: https://crrev.com/e625cb78079a7c15a944cff3fe89b28a3779b697 Cr-Commit-Position: refs/heads/master@{#396035} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/e625cb78079a7c15a944cff3fe89b28a3779b697 Cr-Commit-Position: refs/heads/master@{#396035} |
