New issue
Advanced search Search tips

Issue 651554 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 348655



Sign in to add a comment

Many places in the code re-parse GURLs from serialized url::Origins

Project Member Reported by csharrison@chromium.org, Sep 29 2016

Issue description

This can be quite costly. We should be able to add a method to Origin to create a parsed GURL from the already parsed structure in Origin.

 
Components: -Blink>Loader
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 4 2016

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

commit 048bee100cbbc999219901a759fc86abc67c726d
Author: csharrison <csharrison@chromium.org>
Date: Tue Oct 04 00:08:21 2016

Add url::Origin::GetURL() to convert Origins to URLs without reparsing

The previous canonical way to convert an Origin to a GURL was
to call GURL(origin.Serialize()). This is expensive and often in the
critical path of user perceived page loads. One such caller has
been converted to the new method, and the rest will be converted
in a followup patch.

BUG=651554

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

[modify] https://crrev.com/048bee100cbbc999219901a759fc86abc67c726d/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/048bee100cbbc999219901a759fc86abc67c726d/url/origin.cc
[modify] https://crrev.com/048bee100cbbc999219901a759fc86abc67c726d/url/origin.h
[modify] https://crrev.com/048bee100cbbc999219901a759fc86abc67c726d/url/origin_unittest.cc
[modify] https://crrev.com/048bee100cbbc999219901a759fc86abc67c726d/url/scheme_host_port.cc
[modify] https://crrev.com/048bee100cbbc999219901a759fc86abc67c726d/url/scheme_host_port.h

Project Member

Comment 3 by bugdroid1@chromium.org, Oct 8 2016

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

commit 6fbc9faa8aad3b74423c32254148eba990a8c069
Author: csharrison <csharrison@chromium.org>
Date: Sat Oct 08 03:31:50 2016

Fix SchemeHostPort::GetURL() and add more tests

The implementation erroneously
 - Added an extra / to invalid URLs
 - Populated empty (len 0) url::Parsed components when they were empty in
   the class. Instead, they should have begin 0 and len -1.

BUG=651554

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

[modify] https://crrev.com/6fbc9faa8aad3b74423c32254148eba990a8c069/url/origin_unittest.cc
[modify] https://crrev.com/6fbc9faa8aad3b74423c32254148eba990a8c069/url/scheme_host_port.cc
[modify] https://crrev.com/6fbc9faa8aad3b74423c32254148eba990a8c069/url/scheme_host_port_unittest.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Oct 12 2016

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

commit aec2c5453d3e7769057aab74142b403d87791a8b
Author: csharrison <csharrison@chromium.org>
Date: Wed Oct 12 19:40:36 2016

Replace usage of GURL(origin.Serialize()) with origin.GetURL()

The bulk of this was via the following command:
git grep -l "GURL([a-zA-Z0-9\._]*\.Serialize())" | xargs sed -i \
 's/GURL(\([a-zA-Z0-9\._]*\)\.Serialize())/\1.GetURL()/g'

Which was then followed by a 'git cl format', and removing changes from
url unit tests, which check that this change has identical semantics.

renderer_host/media also had ConvertToGURL(Origin) methods
which were removed.

BUG=651554
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[modify] https://crrev.com/aec2c5453d3e7769057aab74142b403d87791a8b/chrome/browser/browsing_data/origin_filter_builder.cc
[modify] https://crrev.com/aec2c5453d3e7769057aab74142b403d87791a8b/chrome/browser/budget_service/budget_database.cc
[modify] https://crrev.com/aec2c5453d3e7769057aab74142b403d87791a8b/chrome/browser/budget_service/budget_manager.cc
[modify] https://crrev.com/aec2c5453d3e7769057aab74142b403d87791a8b/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/aec2c5453d3e7769057aab74142b403d87791a8b/chrome/browser/site_details.cc
[modify] https://crrev.com/aec2c5453d3e7769057aab74142b403d87791a8b/chrome/browser/ui/android/bluetooth_chooser_android.cc
[modify] https://crrev.com/aec2c5453d3e7769057aab74142b403d87791a8b/chrome/browser/ui/login/login_handler.cc
[modify] https://crrev.com/aec2c5453d3e7769057aab74142b403d87791a8b/components/subresource_filter/core/common/indexed_ruleset.cc
[modify] https://crrev.com/aec2c5453d3e7769057aab74142b403d87791a8b/content/browser/cache_storage/cache_storage_dispatcher_host.cc
[modify] https://crrev.com/aec2c5453d3e7769057aab74142b403d87791a8b/content/browser/frame_host/render_frame_message_filter.cc
[modify] https://crrev.com/aec2c5453d3e7769057aab74142b403d87791a8b/content/browser/indexed_db/indexed_db_backing_store.cc
[modify] https://crrev.com/aec2c5453d3e7769057aab74142b403d87791a8b/content/browser/indexed_db/indexed_db_context_impl.cc
[modify] https://crrev.com/aec2c5453d3e7769057aab74142b403d87791a8b/content/browser/indexed_db/indexed_db_internals_ui.cc
[modify] https://crrev.com/aec2c5453d3e7769057aab74142b403d87791a8b/content/browser/indexed_db/indexed_db_quota_client.cc
[modify] https://crrev.com/aec2c5453d3e7769057aab74142b403d87791a8b/content/browser/indexed_db/indexed_db_quota_client.h
[modify] https://crrev.com/aec2c5453d3e7769057aab74142b403d87791a8b/content/browser/indexed_db/indexed_db_unittest.cc
[modify] https://crrev.com/aec2c5453d3e7769057aab74142b403d87791a8b/content/browser/permissions/permission_service_impl.cc
[modify] https://crrev.com/aec2c5453d3e7769057aab74142b403d87791a8b/content/browser/renderer_host/database_message_filter.cc
[modify] https://crrev.com/aec2c5453d3e7769057aab74142b403d87791a8b/content/browser/renderer_host/media/media_stream_manager.cc
[modify] https://crrev.com/aec2c5453d3e7769057aab74142b403d87791a8b/content/browser/renderer_host/media/media_stream_ui_proxy.cc
[modify] https://crrev.com/aec2c5453d3e7769057aab74142b403d87791a8b/content/browser/renderer_host/render_message_filter.cc
[modify] https://crrev.com/aec2c5453d3e7769057aab74142b403d87791a8b/content/renderer/dom_storage/local_storage_cached_area.cc
[modify] https://crrev.com/aec2c5453d3e7769057aab74142b403d87791a8b/extensions/browser/extension_web_contents_observer.cc
[modify] https://crrev.com/aec2c5453d3e7769057aab74142b403d87791a8b/ios/web/public/origin_util.mm
[modify] https://crrev.com/aec2c5453d3e7769057aab74142b403d87791a8b/net/base/registry_controlled_domains/registry_controlled_domain.cc

Blocking: 348655
Project Member

Comment 6 by bugdroid1@chromium.org, Oct 27 2016

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

commit d88f975e033692c119c62373f3782875fad40115
Author: csharrison <csharrison@chromium.org>
Date: Wed Oct 26 23:56:36 2016

Convert WebSecurityOrigin -> GURL without re-parsing the url

BUG=651554

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

[modify] https://crrev.com/d88f975e033692c119c62373f3782875fad40115/chrome/renderer/chrome_content_renderer_client.cc
[modify] https://crrev.com/d88f975e033692c119c62373f3782875fad40115/chrome/renderer/content_settings_observer.cc
[modify] https://crrev.com/d88f975e033692c119c62373f3782875fad40115/chrome/renderer/extensions/media_galleries_custom_bindings.cc
[modify] https://crrev.com/d88f975e033692c119c62373f3782875fad40115/chrome/renderer/extensions/resource_request_policy.cc
[modify] https://crrev.com/d88f975e033692c119c62373f3782875fad40115/chrome/renderer/plugins/chrome_plugin_placeholder.cc
[modify] https://crrev.com/d88f975e033692c119c62373f3782875fad40115/chrome/renderer/worker_content_settings_client_proxy.cc
[modify] https://crrev.com/d88f975e033692c119c62373f3782875fad40115/content/child/notifications/notification_manager.cc
[modify] https://crrev.com/d88f975e033692c119c62373f3782875fad40115/content/child/notifications/notification_manager.h
[modify] https://crrev.com/d88f975e033692c119c62373f3782875fad40115/content/child/storage_util.cc
[modify] https://crrev.com/d88f975e033692c119c62373f3782875fad40115/content/renderer/dom_storage/local_storage_namespace.cc
[modify] https://crrev.com/d88f975e033692c119c62373f3782875fad40115/content/renderer/dom_storage/local_storage_namespace.h
[modify] https://crrev.com/d88f975e033692c119c62373f3782875fad40115/content/renderer/dom_storage/webstoragenamespace_impl.cc
[modify] https://crrev.com/d88f975e033692c119c62373f3782875fad40115/content/renderer/dom_storage/webstoragenamespace_impl.h
[modify] https://crrev.com/d88f975e033692c119c62373f3782875fad40115/content/renderer/media/android/webmediaplayer_android.cc
[modify] https://crrev.com/d88f975e033692c119c62373f3782875fad40115/content/renderer/media/cdm/pepper_cdm_wrapper_impl.cc
[modify] https://crrev.com/d88f975e033692c119c62373f3782875fad40115/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/d88f975e033692c119c62373f3782875fad40115/content/renderer/render_view_impl.cc
[modify] https://crrev.com/d88f975e033692c119c62373f3782875fad40115/content/renderer/shared_worker/embedded_shared_worker_stub.cc
[modify] https://crrev.com/d88f975e033692c119c62373f3782875fad40115/media/blink/key_system_config_selector.cc
[modify] https://crrev.com/d88f975e033692c119c62373f3782875fad40115/media/blink/webcontentdecryptionmodule_impl.cc
[modify] https://crrev.com/d88f975e033692c119c62373f3782875fad40115/media/blink/webencryptedmediaclient_impl.cc
[modify] https://crrev.com/d88f975e033692c119c62373f3782875fad40115/media/blink/webmediaplayer_util.cc
[modify] https://crrev.com/d88f975e033692c119c62373f3782875fad40115/third_party/WebKit/Source/modules/storage/StorageNamespace.cpp
[modify] https://crrev.com/d88f975e033692c119c62373f3782875fad40115/third_party/WebKit/public/platform/WebStorageNamespace.h

Unfortunately I did some more in-depth tracing/profiling and replacing

WebStringToGURL(web_security_origin.toString())

with

url::Origin(web_security_origin).GetURL()

Only produces marginal benefits (though it is a nicer API). I'm going to hold off on finishing the move until I can figure out a way to make the two steps (operator url::Origin and GetURL()) a bit faster.

Most of the time is spent doing string copies, and some policies that happens to be a bit slow.
Project Member

Comment 8 by bugdroid1@chromium.org, Feb 1 2017

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

commit 37f125e437377ad9b3b988edac21075c75ec5432
Author: csharrison <csharrison@chromium.org>
Date: Wed Feb 01 19:26:47 2017

Remove Origin->GURL conversion in DocumentSubresourceFilter

SameDomainOrHost was recently changed [1] to allow for comparing Origins
and GURLs, so we should remove the extra URL parsing here, which is a very
expensive operation.

[1] https://codereview.chromium.org/2568133007/

BUG=651554

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

[modify] https://crrev.com/37f125e437377ad9b3b988edac21075c75ec5432/components/subresource_filter/core/common/first_party_origin.cc
[modify] https://crrev.com/37f125e437377ad9b3b988edac21075c75ec5432/components/subresource_filter/core/common/first_party_origin.h

Sign in to add a comment