Optimize KURL protocols

Optimize KURL protocols

This patch optimizes KURL::protocol and KURL::protocolIs by keeping
an AtomicString m_protocol on KURL. This reduces string allocations
throughout the code using KURL::protocol().

This also fixes an inconsistency with KURL::protocolIs that will return
true for invalid URLs.

BUG=348655
Committed: https://crrev.com/775abc2d7c903f191f7b24f8b299ebabbea3f624
Cr-Commit-Position: refs/heads/master@{#438197}

[Charlie Harrison, 2016-10-31 02:24:08]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 02:24:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-31 03:37:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-31 03:37:47]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-11-03 04:17:24]

Description was changed from

==========
Optimize KURL protocols

This patch optimizes KURL::protocol and KURL::protocolIs by keeping
an AtomicString m_protocol on KURL.

BUG=
==========

to

==========
Optimize KURL protocols

This patch optimizes KURL::protocol and KURL::protocolIs by keeping
an AtomicString m_protocol on KURL.

This also fixes an inconsistency with KURL::protocolIs that will return
true for invalid URL. With this patch,
KURL::protocolIs(<protocol>) == (KURL.protocol() == <protocol>).

BUG=
==========

[Charlie Harrison, 2016-11-03 05:25:20]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 05:25:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-03 07:26:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-03 07:26:17]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[Charlie Harrison, 2016-11-03 12:19:34]

Description was changed from

==========
Optimize KURL protocols

This patch optimizes KURL::protocol and KURL::protocolIs by keeping
an AtomicString m_protocol on KURL.

This also fixes an inconsistency with KURL::protocolIs that will return
true for invalid URL. With this patch,
KURL::protocolIs(<protocol>) == (KURL.protocol() == <protocol>).

BUG=
==========

to

==========
Optimize KURL protocols

This patch optimizes KURL::protocol and KURL::protocolIs by keeping
an AtomicString m_protocol on KURL. This reduces string allocations
throughout the code using KURL::protocol().

This also fixes an inconsistency with KURL::protocolIs that will return
true for invalid URLs.

BUG=348655
==========

[Charlie Harrison, 2016-11-03 12:49:25]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 12:49:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2016-11-03 12:52:32]

csharrison@chromium.org changed reviewers:
+ esprehn@chromium.org

[Charlie Harrison, 2016-11-03 12:52:33]

esprehn, this has less win than the string hashing CL, but it's a good step for
further optimizing SchemeRegistry.

Wins will also compound once we remove the HashMap there and can just do a
series AtomicString comparisons.

There's also a reasonable reduction in KURL complexity here. WDYT?

[commit-bot: I haz the power, 2016-11-03 14:41:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-03 14:41:14]

Dry run: This issue passed the CQ dry run.

[esprehn, 2016-11-04 23:48:32]

I think something is missing in this patch? Your string is not atomic and you're
assigning the string to itself which doesn't do anything.

https://codereview.chromium.org/2463703002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/weborigin/KURL.cpp (right):

https://codereview.chromium.org/2463703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/weborigin/KURL.cpp:788: m_protocol =
(m_protocol);
what does this do? assigning m_protocol to itself seems like it doesn't do
anything.

https://codereview.chromium.org/2463703002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/weborigin/KURL.h (right):

https://codereview.chromium.org/2463703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/weborigin/KURL.h:218: String m_protocol;
This isn't atomic

[Charlie Harrison, 2016-11-04 23:58:36]

Sorry, let me re-upload the patch.

https://codereview.chromium.org/2463703002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/weborigin/KURL.cpp (right):

https://codereview.chromium.org/2463703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/weborigin/KURL.cpp:788: m_protocol =
(m_protocol);
On 2016/11/04 23:48:32, esprehn wrote:
> what does this do? assigning m_protocol to itself seems like it doesn't do
> anything.

Oops. That should be m_protocol = AtomicString(m_protocol). Sorry about that :/

[Charlie Harrison, 2016-11-05 01:16:49]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-05 01:17:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-05 02:44:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-05 02:44:39]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-11-07 21:44:10]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-07 21:44:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2016-11-07 21:45:27]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-07 21:45:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-07 21:49:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-07 21:49:28]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Charlie Harrison, 2016-11-07 21:53:15]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-07 21:53:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2016-11-07 23:02:52]

So, here's the updated patch with a few changes.

 - I've added http/https atoms, and rather than hashing the protocol in
initProtocolMetadata, we first try to see if the url is http/https (common
case). To avoid an allocation I've added componentStringView method.
 - http/https atom added in SchemeRegistry

[commit-bot: I haz the power, 2016-11-07 23:18:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-07 23:18:01]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Charlie Harrison, 2016-11-29 20:41:12]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 20:41:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 23:13:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-29 23:13:20]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Charlie Harrison, 2016-12-12 16:04:56]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 16:05:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-12 16:29:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-12 16:29:15]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[Charlie Harrison, 2016-12-12 19:15:16]

esprehn: would you take another look at this?

Another benefit here is we avoid doing hashSlowCase for every newly allocated
protocol. A common pattern is calling KURL::protocol() and looking it up in a
map.

[esprehn, 2016-12-12 22:18:46]

Are you sure this is right? The old code ignored case, and the new code is
case-sensitive so a protocol of "HTTP" is now not in the http family and
protocolIs("HTTP") will return false. Is that correct?

https://codereview.chromium.org/2463703002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/weborigin/KURL.cpp (left):

https://codereview.chromium.org/2463703002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/weborigin/KURL.cpp:815: return
internalProtocolIs(scheme, spec, "http-so");
This was all case insensitive before.

https://codereview.chromium.org/2463703002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/weborigin/KURL.cpp (right):

https://codereview.chromium.org/2463703002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/weborigin/KURL.cpp:814: m_protocol ==
"http-so" || m_protocol == "https-so";
What strange protocols are these?

https://codereview.chromium.org/2463703002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/weborigin/KURL.cpp:826: return m_protocol ==
protocol;
internalProtocolIs was case insensitive, are you sure this is right?

[Charlie Harrison, 2016-12-12 22:26:07]

I think this should be OK. In general we want protocol comparisons to be case
sensitive (same with host comparison). This is how e.g. GURL::SchemeIs() is
implemented.

https://codereview.chromium.org/2463703002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/weborigin/KURL.cpp (left):

https://codereview.chromium.org/2463703002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/weborigin/KURL.cpp:815: return
internalProtocolIs(scheme, spec, "http-so");
On 2016/12/12 22:18:46, esprehn wrote:
> This was all case insensitive before.

We should always have a canonical, lowercase scheme here, so the case
insensitivity was not necessary. We could probably DCHECK this fact though.

https://codereview.chromium.org/2463703002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/weborigin/KURL.cpp (right):

https://codereview.chromium.org/2463703002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/weborigin/KURL.cpp:814: m_protocol ==
"http-so" || m_protocol == "https-so";
On 2016/12/12 22:18:46, esprehn wrote:
> What strange protocols are these?

suborigin protocols:
https://w3c.github.io/webappsec-suborigins/#representation

I think these are rare so I didn't add them to the atomic string table.

https://codereview.chromium.org/2463703002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/weborigin/KURL.cpp:826: return m_protocol ==
protocol;
On 2016/12/12 22:18:46, esprehn wrote:
> internalProtocolIs was case insensitive, are you sure this is right?

assertProtocolIsGood checks that the input protocol wasn't uppercase ASCII.

[esprehn, 2016-12-12 23:19:48]

lgtm

[Charlie Harrison, 2016-12-13 15:30:10]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 15:30:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2016-12-13 16:03:22]

Many thanks for the review. I've just cleaned things up slightly and added a few
DCHECKs.

https://codereview.chromium.org/2463703002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/weborigin/KURL.cpp (left):

https://codereview.chromium.org/2463703002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/weborigin/KURL.cpp:815: return
internalProtocolIs(scheme, spec, "http-so");
On 2016/12/12 22:26:06, Charlie Harrison wrote:
> On 2016/12/12 22:18:46, esprehn wrote:
> > This was all case insensitive before.
> 
> We should always have a canonical, lowercase scheme here, so the case
> insensitivity was not necessary. We could probably DCHECK this fact though.

Actually, we have a DCHECK_EQ(protocol(), protocol().lower()) in KURL::init from
a previous patch which removed a lot of random protocol.lower() in a bunch of
places in the codebase. Let me move it to initProtocolMetadata which will catch
more cases.

[Charlie Harrison, 2016-12-13 16:03:31]

The CQ bit was unchecked by csharrison@chromium.org

[Charlie Harrison, 2016-12-13 16:03:32]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2016-12-13 16:03:32]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/2463703002/#ps160001
(title: "add dchecks")

[commit-bot: I haz the power, 2016-12-13 16:04:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 17:10:39]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1481645012011040,
"parent_rev": "11fd06a29111ddc3f9c32f761dd9c813f40b5a41", "commit_rev":
"334c786f4ac4fdd6df634ecc970b33dfc0702d79"}

[commit-bot: I haz the power, 2016-12-13 17:11:13]

Description was changed from

==========
Optimize KURL protocols

This patch optimizes KURL::protocol and KURL::protocolIs by keeping
an AtomicString m_protocol on KURL. This reduces string allocations
throughout the code using KURL::protocol().

This also fixes an inconsistency with KURL::protocolIs that will return
true for invalid URLs.

BUG=348655
==========

to

==========
Optimize KURL protocols

This patch optimizes KURL::protocol and KURL::protocolIs by keeping
an AtomicString m_protocol on KURL. This reduces string allocations
throughout the code using KURL::protocol().

This also fixes an inconsistency with KURL::protocolIs that will return
true for invalid URLs.

BUG=348655

Review-Url: https://codereview.chromium.org/2463703002
==========

[commit-bot: I haz the power, 2016-12-13 17:11:16]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-12-13 17:12:39]

Description was changed from

==========
Optimize KURL protocols

This patch optimizes KURL::protocol and KURL::protocolIs by keeping
an AtomicString m_protocol on KURL. This reduces string allocations
throughout the code using KURL::protocol().

This also fixes an inconsistency with KURL::protocolIs that will return
true for invalid URLs.

BUG=348655

Review-Url: https://codereview.chromium.org/2463703002
==========

to

==========
Optimize KURL protocols

This patch optimizes KURL::protocol and KURL::protocolIs by keeping
an AtomicString m_protocol on KURL. This reduces string allocations
throughout the code using KURL::protocol().

This also fixes an inconsistency with KURL::protocolIs that will return
true for invalid URLs.

BUG=348655

Committed: https://crrev.com/775abc2d7c903f191f7b24f8b299ebabbea3f624
Cr-Commit-Position: refs/heads/master@{#438197}
==========

[commit-bot: I haz the power, 2016-12-13 17:12:40]

Patchset 9 (id:??) landed as
https://crrev.com/775abc2d7c903f191f7b24f8b299ebabbea3f624
Cr-Commit-Position: refs/heads/master@{#438197}

[Charlie Harrison, 2016-12-20 04:16:24]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/2586253004/ by csharrison@chromium.org.

The reason for reverting is: Causing DCHECK flakes. crbug.com/674388.