Reduce the perf overhead of our dangling markup experiments.

Reduce the perf overhead of our dangling markup experiments.

Given that Document::completeURL is a hotspot, this patch revamps the
approach to capture removed whitespace up in //url (where we're already
scanning through the string), and limits the scan for '<' to cases in
which we've actually removed whitespace.

BUG=680970, 682300
R=jochen@chromium.org

Review-Url: https://codereview.chromium.org/2643613002
Cr-Commit-Position: refs/heads/master@{#444734}
Committed: https://chromium.googlesource.com/chromium/src/+/41318f4888c1f60613c74cc86e539d173b16b0ad

[Mike West, 2017-01-18 06:37:55]

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

[commit-bot: I haz the power, 2017-01-18 06:38:08]

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

[Mike West, 2017-01-18 08:23:06]

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

[commit-bot: I haz the power, 2017-01-18 08:23: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, 2017-01-18 10:42:44]

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

[commit-bot: I haz the power, 2017-01-18 10:42:45]

Dry run: This issue passed the CQ dry run.

[Mike West, 2017-01-18 10:44:07]

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

[Mike West, 2017-01-18 10:44:07]

WDYT about this, esprehn@?

FYI: csharrison@, as you care about URL-parsing speed. :) I wonder whether we
really need the `RemoveURLWhitespace` behavior for actual websites. Have we
looked at this before? If the metrics I'm adding here show low usage, maybe we
can cut out some fairly expensive processing by just changing newlines, etc. to
parse errors.

[Charlie Harrison, 2017-01-18 13:15:18]

On 2017/01/18 10:44:07, Mike West (sloooooow) wrote:
> WDYT about this, esprehn@?
> 
> FYI: csharrison@, as you care about URL-parsing speed. :) I wonder whether we
> really need the `RemoveURLWhitespace` behavior for actual websites. Have we
> looked at this before? If the metrics I'm adding here show low usage, maybe we
> can cut out some fairly expensive processing by just changing newlines, etc.
to
> parse errors.

I am very interested in removing this step in URL parsing if possible. I think
for big URLs it takes ~1/4-1/5 or more of the parsing time. Probably Brett
should weigh in though because I have no historical context.

[Mike West, 2017-01-18 14:29:39]

On 2017/01/18 at 13:15:18, csharrison wrote:
> On 2017/01/18 10:44:07, Mike West (sloooooow) wrote:
> > WDYT about this, esprehn@?
> > 
> > FYI: csharrison@, as you care about URL-parsing speed. :) I wonder whether
we
> > really need the `RemoveURLWhitespace` behavior for actual websites. Have we
> > looked at this before? If the metrics I'm adding here show low usage, maybe
we
> > can cut out some fairly expensive processing by just changing newlines, etc.
to
> > parse errors.
> 
> I am very interested in removing this step in URL parsing if possible. I think
for big URLs it takes ~1/4-1/5 or more of the parsing time. Probably Brett
should weigh in though because I have no historical context.

Based on hacky regexes through HTTPArchive (see
https://bugs.chromium.org/p/chromium/issues/detail?id=680970#c3), newlines don't
happen that often. It would be nice if these metrics confirmed that... then we
can sit down with folks like Anne @ Mozilla to see if we can update the spec.

[Mike West, 2017-01-19 07:31:23]

mkwst@chromium.org changed reviewers:
+ jochen@chromium.org

[Mike West, 2017-01-19 07:31:23]

Jochen, mind taking a look at this this morning? esprehn@ was right; this is a
fairly substantial regression in parsing perf (see https://crbug.com/682300).

[Mike West, 2017-01-19 07:31:31]

Description was changed from

==========
Reduce the perf overhead of our dangling markup experiments.

Given that Document::completeURL is a hotspot, this patch revamps the
approach to capture removed whitespace up in //url (where we're already
scanning through the string), and limits the scan for '<' to cases in
which we've actually removed whitespace.

BUG=680970
R=esprehn@chromium.org
==========

to

==========
Reduce the perf overhead of our dangling markup experiments.

Given that Document::completeURL is a hotspot, this patch revamps the
approach to capture removed whitespace up in //url (where we're already
scanning through the string), and limits the scan for '<' to cases in
which we've actually removed whitespace.

BUG=680970, 682300
R=esprehn@chromium.org
==========

[Mike West, 2017-01-19 09:28:11]

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

[commit-bot: I haz the power, 2017-01-19 09:28:25]

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, 2017-01-19 09:29:52]

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

[commit-bot: I haz the power, 2017-01-19 09:29:52]

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...)

[Mike West, 2017-01-19 11:16:15]

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

[commit-bot: I haz the power, 2017-01-19 11:16:24]

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

[jochen (gone - plz use gerrit), 2017-01-19 12:35:42]

lgtm

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

https://codereview.chromium.org/2643613002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/weborigin/KURL.h:194: bool
whitespaceRemoved() const { return m_parsed.whitespace_removed; }
maybe add a unit test?

[Charlie Harrison, 2017-01-19 13:06:50]

non-owner lgtm

https://codereview.chromium.org/2643613002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2643613002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:4950: if
(completed.whitespaceRemoved()) {
Optional: I think an early return here would be nicer stylistically, as it is
the common case to have no whitespace.

[Mike West, 2017-01-19 13:13:51]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2017-01-19 13:13:51]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
csharrison@chromium.org
Link to the patchset: https://codereview.chromium.org/2643613002/#ps60001
(title: "unittest")

[commit-bot: I haz the power, 2017-01-19 13:14:00]

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

[Mike West, 2017-01-19 13:38:26]

Description was changed from

==========
Reduce the perf overhead of our dangling markup experiments.

Given that Document::completeURL is a hotspot, this patch revamps the
approach to capture removed whitespace up in //url (where we're already
scanning through the string), and limits the scan for '<' to cases in
which we've actually removed whitespace.

BUG=680970, 682300
R=esprehn@chromium.org
==========

to

==========
Reduce the perf overhead of our dangling markup experiments.

Given that Document::completeURL is a hotspot, this patch revamps the
approach to capture removed whitespace up in //url (where we're already
scanning through the string), and limits the scan for '<' to cases in
which we've actually removed whitespace.

BUG=680970, 682300
R=jochen@chromium.org
==========

[commit-bot: I haz the power, 2017-01-19 15:11:54]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484831631018300,
"parent_rev": "17231785eecb024061d82c7097a78d3169503aff", "commit_rev":
"41318f4888c1f60613c74cc86e539d173b16b0ad"}

[commit-bot: I haz the power, 2017-01-19 15:12:49]

Description was changed from

==========
Reduce the perf overhead of our dangling markup experiments.

Given that Document::completeURL is a hotspot, this patch revamps the
approach to capture removed whitespace up in //url (where we're already
scanning through the string), and limits the scan for '<' to cases in
which we've actually removed whitespace.

BUG=680970, 682300
R=jochen@chromium.org
==========

to

==========
Reduce the perf overhead of our dangling markup experiments.

Given that Document::completeURL is a hotspot, this patch revamps the
approach to capture removed whitespace up in //url (where we're already
scanning through the string), and limits the scan for '<' to cases in
which we've actually removed whitespace.

BUG=680970, 682300
R=jochen@chromium.org

Review-Url: https://codereview.chromium.org/2643613002
Cr-Commit-Position: refs/heads/master@{#444734}
Committed:
https://chromium.googlesource.com/chromium/src/+/41318f4888c1f60613c74cc86e53...
==========

[commit-bot: I haz the power, 2017-01-19 15:12:51]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/41318f4888c1f60613c74cc86e53...