New issue
Advanced search Search tips

Issue 672877 link

Starred by 1 user

Issue metadata

Status: Archived
Owner:
Closed: Jan 11
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 348655



Sign in to add a comment

Origin conversions should be cheaper

Project Member Reported by csharrison@chromium.org, Dec 9 2016

Issue description

WebSecurityOrigin -> url::Origin has some overhead:
 - String allocation for WebString::utf8 (pretty unavoidable)
 - Needless string copying (avoidable with move or swap)
 - SchemeHostPort IsValidInput which does some policy work (inc. searching a list of schemes)

url::Origin::GetURL() has some overhead:
 - string allocations
 - string append
 - Some comparisons
 - Unsure if any of this is avoidable.

Ideally, we want [1] to be at least 2x faster than [2]
[1]: url::Origin(web_security_origin).GetURL()
[2]: WebStringToGURL(web_security_origin.toString())
 
Blocking: 348655
Project Member

Comment 2 by bugdroid1@chromium.org, Dec 13 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f07ac3c6704c3db574549ebc2f2e9880dcbf66b7

commit f07ac3c6704c3db574549ebc2f2e9880dcbf66b7
Author: csharrison <csharrison@chromium.org>
Date: Tue Dec 13 04:15:02 2016

Add std::string constructors for Origin/SchemeHostPort to reduce copies

WebSecurityOrigin forwards temporaries to Origin. We should just use
these, rather than consider them opaque StringPieces and copying data
out of them.

This CL also removes some dead code.

BUG= 672877 

Review-Url: https://codereview.chromium.org/2561363002
Cr-Commit-Position: refs/heads/master@{#438054}

[modify] https://crrev.com/f07ac3c6704c3db574549ebc2f2e9880dcbf66b7/content/child/blink_platform_impl_unittest.cc
[modify] https://crrev.com/f07ac3c6704c3db574549ebc2f2e9880dcbf66b7/url/origin.cc
[modify] https://crrev.com/f07ac3c6704c3db574549ebc2f2e9880dcbf66b7/url/origin.h
[modify] https://crrev.com/f07ac3c6704c3db574549ebc2f2e9880dcbf66b7/url/origin_unittest.cc
[modify] https://crrev.com/f07ac3c6704c3db574549ebc2f2e9880dcbf66b7/url/scheme_host_port.cc
[modify] https://crrev.com/f07ac3c6704c3db574549ebc2f2e9880dcbf66b7/url/scheme_host_port.h

Project Member

Comment 3 by bugdroid1@chromium.org, Dec 15 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8327037293547a118f463e2e359274b51de1d6d3

commit 8327037293547a118f463e2e359274b51de1d6d3
Author: csharrison <csharrison@chromium.org>
Date: Thu Dec 15 17:22:21 2016

Do not check for ASCII in WebSecurityOrigin->Origin conversion

Protocols, hosts, and suborigins are guaranteed to be canonicalized to
ASCII. Using WebString::ascii DCHECKs this fact, whereas WebString::utf8
branches on it. Using ascii() prevents a scan-through of each part of
the origin.

BUG= 672877 

Review-Url: https://codereview.chromium.org/2573113002
Cr-Commit-Position: refs/heads/master@{#438858}

[modify] https://crrev.com/8327037293547a118f463e2e359274b51de1d6d3/third_party/WebKit/public/platform/WebSecurityOrigin.h

Project Member

Comment 4 by bugdroid1@chromium.org, Jan 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/39a260f9aff94ecd6d518778e2ea900f58933636

commit 39a260f9aff94ecd6d518778e2ea900f58933636
Author: csharrison <csharrison@chromium.org>
Date: Mon Jan 09 22:24:14 2017

Remove comparisons for blob/filesystem schemes in scheme_host_port

These are checked implicitly via the fact that blob schemes are not
standard, and filesystem schemes have type SCHEME_WITHOUT_AUTHORITY.

IsStandard is in critical path of loading many resources, as it is
used for conversions to url::Origin.

BUG= 672877 

Review-Url: https://codereview.chromium.org/2560923002
Cr-Commit-Position: refs/heads/master@{#442372}

[modify] https://crrev.com/39a260f9aff94ecd6d518778e2ea900f58933636/url/scheme_host_port.cc
[modify] https://crrev.com/39a260f9aff94ecd6d518778e2ea900f58933636/url/scheme_host_port_unittest.cc

You started fixing this bug over two years ago. Are you still working on it? You can update the status to "archived", "wontfix", or "closed". You can remove yourself as owner and change status to "untriaged", but if this is still a real bug, please do not sit on it.
Status: Archived (was: Started)
There's probably more to do, but it's so low priority it's fine to archive.

Sign in to add a comment