Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2540893004:
[url] Avoid scanning for whitespace twice during ResolveRelative (Closed)
|
Created:
4 years ago by Charlie Harrison Modified:
4 years ago Reviewers:
Mike West CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[url] Avoid scanning for whitespace twice during ResolveRelative
For calls to url::ResolveRelative, if the input is not in fact relative,
we call RemoveURLWhitespace twice on the same buffer. This is not necessary.
On a test of loading 100 identical ~1mb data URL images, this patch speeds
up the renderer by ~8% on Linux.
BUG=657978
Committed: https://crrev.com/c64537201a63fc8436fcedd8edab32be3669405b
Cr-Commit-Position: refs/heads/master@{#435604}
Patch Set 1 #Patch Set 2 : minor tweaks + one sanity test #
Total comments: 2
Patch Set 3 : DONT -> DO_NOT #Messages
Total messages: 22 (14 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 ========== [url] Do not check needlessly check for whitespace removal twice For calls to url::ResolveRelative, if the input is not in fact relative, we call RemoveURLWhitespace twice on the same buffer. This is not necessary. On a test of loading 100 identical ~1mb data URL images, this patch speeds up the renderer by ~8% on Linux. BUG=657978 ========== to ========== [url] Avoid scanning for whitespace twice during ResolveRelative For calls to url::ResolveRelative, if the input is not in fact relative, we call RemoveURLWhitespace twice on the same buffer. This is not necessary. On a test of loading 100 identical ~1mb data URL images, this patch speeds up the renderer by ~8% on Linux. BUG=657978 ==========
csharrison@chromium.org changed reviewers: + mkwst@chromium.org
Hi Mike, would you be willing to review this? I think Brett is not reviewing (according to their nick) until Dec 5, but I'm happy to wait until then if you'd like.
Oh and just for reference, this code path is exercised heavily in Blink to initialize KURLs. I added a test to GURL just because it GURL::Resolve also goes through that code path.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM % tiny nit. https://codereview.chromium.org/2540893004/diff/20001/url/url_util.cc File url/url_util.cc (right): https://codereview.chromium.org/2540893004/diff/20001/url/url_util.cc#newcode26 url/url_util.cc:26: DONT_REMOVE_WHITESPACE, Nit: I'd prefer `DO_NOT` to `DONT`. The lack of a quote just bothers me aesthetically. :)
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...
Many thanks!
https://codereview.chromium.org/2540893004/diff/20001/url/url_util.cc File url/url_util.cc (right): https://codereview.chromium.org/2540893004/diff/20001/url/url_util.cc#newcode26 url/url_util.cc:26: DONT_REMOVE_WHITESPACE, On 2016/12/01 09:39:36, Mike West (slow) wrote: > Nit: I'd prefer `DO_NOT` to `DONT`. The lack of a quote just bothers me > aesthetically. :) Done.
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 mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/2540893004/#ps40001 (title: "DONT -> DO_NOT")
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": 40001, "attempt_start_ts": 1480598863571400,
"parent_rev": "5939f8dd62324fbfa375dfb905354e9c4b953e27", "commit_rev":
"9dd8da664f4213eb56140bbb5b51c6c659a435af"}
Message was sent while issue was closed.
Description was changed from ========== [url] Avoid scanning for whitespace twice during ResolveRelative For calls to url::ResolveRelative, if the input is not in fact relative, we call RemoveURLWhitespace twice on the same buffer. This is not necessary. On a test of loading 100 identical ~1mb data URL images, this patch speeds up the renderer by ~8% on Linux. BUG=657978 ========== to ========== [url] Avoid scanning for whitespace twice during ResolveRelative For calls to url::ResolveRelative, if the input is not in fact relative, we call RemoveURLWhitespace twice on the same buffer. This is not necessary. On a test of loading 100 identical ~1mb data URL images, this patch speeds up the renderer by ~8% on Linux. BUG=657978 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [url] Avoid scanning for whitespace twice during ResolveRelative For calls to url::ResolveRelative, if the input is not in fact relative, we call RemoveURLWhitespace twice on the same buffer. This is not necessary. On a test of loading 100 identical ~1mb data URL images, this patch speeds up the renderer by ~8% on Linux. BUG=657978 ========== to ========== [url] Avoid scanning for whitespace twice during ResolveRelative For calls to url::ResolveRelative, if the input is not in fact relative, we call RemoveURLWhitespace twice on the same buffer. This is not necessary. On a test of loading 100 identical ~1mb data URL images, this patch speeds up the renderer by ~8% on Linux. BUG=657978 Committed: https://crrev.com/c64537201a63fc8436fcedd8edab32be3669405b Cr-Commit-Position: refs/heads/master@{#435604} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c64537201a63fc8436fcedd8edab32be3669405b Cr-Commit-Position: refs/heads/master@{#435604} |
