New issue
Advanced search Search tips

Issue 622464 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Reduce UserScript copy, improve user script code

Project Member Reported by lazyboy@chromium.org, Jun 22 2016

Issue description

I just noticed that we copy around UserScript [1] object which contains vector of UserScript::File and such.

This is a metabug to clean up and improve some of these scenarios.

How big of a win this is, isn't totally clear to me yet. But we should avoid these kind of obvious suboptimal stuffs regardless.

[1] https://cs.chromium.org/chromium/src/extensions/common/user_script.h
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 22 2016

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

commit 28acad804b4c3592ae4aa6465302d895c750c7a6
Author: lazyboy <lazyboy@chromium.org>
Date: Wed Jun 22 23:14:20 2016

Convert prefix search in UserScript code to use base::StartsWith()

Using string.find('pattern') == 0 is O(|string|), so don't do that.

BUG= 622464 
Test=None, internal only change.

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

[modify] https://crrev.com/28acad804b4c3592ae4aa6465302d895c750c7a6/extensions/browser/extension_user_script_loader.cc
[modify] https://crrev.com/28acad804b4c3592ae4aa6465302d895c750c7a6/extensions/browser/web_ui_user_script_loader.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Jul 29 2016

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

commit 8c0f270b6265c8a21fc98c42b04a627b507a0bb6
Author: lazyboy <lazyboy@chromium.org>
Date: Fri Jul 29 16:34:13 2016

[UserScript cleanup] Remove use of ScopedVector from UserScriptSet.

Change it to vector of unique_ptrs.
Also remove an used param from UserScriptSet::OnUserScriptUpdated().

BUG= 622464 
Test=None, internal cleanup.

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

[modify] https://crrev.com/8c0f270b6265c8a21fc98c42b04a627b507a0bb6/extensions/renderer/dispatcher.cc
[modify] https://crrev.com/8c0f270b6265c8a21fc98c42b04a627b507a0bb6/extensions/renderer/dispatcher.h
[modify] https://crrev.com/8c0f270b6265c8a21fc98c42b04a627b507a0bb6/extensions/renderer/script_injection_manager.cc
[modify] https://crrev.com/8c0f270b6265c8a21fc98c42b04a627b507a0bb6/extensions/renderer/script_injection_manager.h
[modify] https://crrev.com/8c0f270b6265c8a21fc98c42b04a627b507a0bb6/extensions/renderer/user_script_injector.cc
[modify] https://crrev.com/8c0f270b6265c8a21fc98c42b04a627b507a0bb6/extensions/renderer/user_script_injector.h
[modify] https://crrev.com/8c0f270b6265c8a21fc98c42b04a627b507a0bb6/extensions/renderer/user_script_set.cc
[modify] https://crrev.com/8c0f270b6265c8a21fc98c42b04a627b507a0bb6/extensions/renderer/user_script_set.h
[modify] https://crrev.com/8c0f270b6265c8a21fc98c42b04a627b507a0bb6/extensions/renderer/user_script_set_manager.cc
[modify] https://crrev.com/8c0f270b6265c8a21fc98c42b04a627b507a0bb6/extensions/renderer/user_script_set_manager.h

Project Member

Comment 3 by bugdroid1@chromium.org, Aug 9 2016

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

commit 4ff54b3aa230cb7a5f7c9b30181ff4e19d763a46
Author: lazyboy <lazyboy@chromium.org>
Date: Tue Aug 09 20:49:08 2016

[UserScript] Do not create SubstitutionMap if not necessary.

Tiny improvement.
We won't need to call GetLocalizationMessages for most extensions as
they wouldn't be needed if those extensions only have content_script
JS but not CSS.

BUG= 622464 
Test=None, internal cleanup.

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

[modify] https://crrev.com/4ff54b3aa230cb7a5f7c9b30181ff4e19d763a46/extensions/browser/extension_user_script_loader.cc

Project Member

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

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

commit 4884705e76ea2cda9dc7a5f189ecbba23ae5fb98
Author: lazyboy <lazyboy@chromium.org>
Date: Fri Aug 12 22:37:23 2016

Rework some UserScriptLoader logic in preparation to removing UserScript copy.

Script removal does not need to store and operate on whole UserScript object,
only the scripts' id and host_id are enough. Make
UserScriptLoader::RemoveScript(s) take those ids (called UserScriptIDPair).

WebViewContentScriptManager also doesn't need the list of entire UserScript,
make them use UserScriptIDPair as well.

All the places that adds scripts usign UserScriptLoader::AddScripts, we
already have scripts with unique IDs generated. So turn those std::set
of scripts to std::vectors. Also add a dcheck that they are indeed unique.

BUG= 622464 

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

[modify] https://crrev.com/4884705e76ea2cda9dc7a5f189ecbba23ae5fb98/chrome/browser/extensions/shared_user_script_master.cc
[modify] https://crrev.com/4884705e76ea2cda9dc7a5f189ecbba23ae5fb98/chrome/browser/extensions/shared_user_script_master.h
[modify] https://crrev.com/4884705e76ea2cda9dc7a5f189ecbba23ae5fb98/extensions/browser/api/guest_view/web_view/web_view_internal_api.cc
[modify] https://crrev.com/4884705e76ea2cda9dc7a5f189ecbba23ae5fb98/extensions/browser/declarative_user_script_master.cc
[modify] https://crrev.com/4884705e76ea2cda9dc7a5f189ecbba23ae5fb98/extensions/browser/declarative_user_script_master.h
[modify] https://crrev.com/4884705e76ea2cda9dc7a5f189ecbba23ae5fb98/extensions/browser/guest_view/web_view/web_view_content_script_manager.cc
[modify] https://crrev.com/4884705e76ea2cda9dc7a5f189ecbba23ae5fb98/extensions/browser/guest_view/web_view/web_view_content_script_manager.h
[modify] https://crrev.com/4884705e76ea2cda9dc7a5f189ecbba23ae5fb98/extensions/browser/user_script_loader.cc
[modify] https://crrev.com/4884705e76ea2cda9dc7a5f189ecbba23ae5fb98/extensions/browser/user_script_loader.h
[modify] https://crrev.com/4884705e76ea2cda9dc7a5f189ecbba23ae5fb98/extensions/browser/web_ui_user_script_loader.cc
[modify] https://crrev.com/4884705e76ea2cda9dc7a5f189ecbba23ae5fb98/extensions/browser/web_ui_user_script_loader.h
[modify] https://crrev.com/4884705e76ea2cda9dc7a5f189ecbba23ae5fb98/extensions/common/user_script.cc
[modify] https://crrev.com/4884705e76ea2cda9dc7a5f189ecbba23ae5fb98/extensions/common/user_script.h

Project Member

Comment 5 by bugdroid1@chromium.org, Aug 18 2016

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

commit 49cc0b3a4e425a884a0b12639951f84b4f84f693
Author: lazyboy <lazyboy@chromium.org>
Date: Thu Aug 18 21:55:12 2016

[Extensions] Reduce string copy in renderer/ UserScript.

Before passing StringPiece script (JS/CSS) content to blink (to create
WebString or WebScriptSource), we sometimes unnecessarily created
std::string. This CL removes the intermediate state.

Greasemonkey scripts still require the intermediate std::string to
pad it with header and tail though. Use reserve and
StringPiece.appendToString() in that case to at least do that
efficiently.

BUG= 622464 
Test=No visible change.

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

[modify] https://crrev.com/49cc0b3a4e425a884a0b12639951f84b4f84f693/extensions/renderer/dispatcher.cc
[modify] https://crrev.com/49cc0b3a4e425a884a0b12639951f84b4f84f693/extensions/renderer/programmatic_script_injector.cc
[modify] https://crrev.com/49cc0b3a4e425a884a0b12639951f84b4f84f693/extensions/renderer/programmatic_script_injector.h
[modify] https://crrev.com/49cc0b3a4e425a884a0b12639951f84b4f84f693/extensions/renderer/script_injection.cc
[modify] https://crrev.com/49cc0b3a4e425a884a0b12639951f84b4f84f693/extensions/renderer/script_injector.h
[modify] https://crrev.com/49cc0b3a4e425a884a0b12639951f84b4f84f693/extensions/renderer/user_script_injector.cc
[modify] https://crrev.com/49cc0b3a4e425a884a0b12639951f84b4f84f693/extensions/renderer/user_script_injector.h

Project Member

Comment 6 by bugdroid1@chromium.org, Aug 18 2016

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

commit b81e69a88c61106c1471d9585a37787f6624e063
Author: lazyboy <lazyboy@chromium.org>
Date: Thu Aug 18 22:35:04 2016

Make FileReader return ownership of the string content.

Since FileReader reads stuff on separate thread, moving
the content around with std::string& is expensive. Make
FileReader pass the string's ownership via unique_ptr.

This reduces string copy primarily in ExecuteCodeFunction
and web_view declarative content scripts.

BUG= 622464 

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

[modify] https://crrev.com/b81e69a88c61106c1471d9585a37787f6624e063/chrome/browser/chromeos/accessibility/accessibility_manager.cc
[modify] https://crrev.com/b81e69a88c61106c1471d9585a37787f6624e063/extensions/browser/api/execute_code_function.cc
[modify] https://crrev.com/b81e69a88c61106c1471d9585a37787f6624e063/extensions/browser/api/execute_code_function.h
[modify] https://crrev.com/b81e69a88c61106c1471d9585a37787f6624e063/extensions/browser/file_reader.cc
[modify] https://crrev.com/b81e69a88c61106c1471d9585a37787f6624e063/extensions/browser/file_reader.h
[modify] https://crrev.com/b81e69a88c61106c1471d9585a37787f6624e063/extensions/browser/file_reader_unittest.cc
[modify] https://crrev.com/b81e69a88c61106c1471d9585a37787f6624e063/extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher.cc
[modify] https://crrev.com/b81e69a88c61106c1471d9585a37787f6624e063/extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher.h
[modify] https://crrev.com/b81e69a88c61106c1471d9585a37787f6624e063/extensions/browser/web_ui_user_script_loader.cc
[modify] https://crrev.com/b81e69a88c61106c1471d9585a37787f6624e063/extensions/browser/web_ui_user_script_loader.h

Project Member

Comment 7 by bugdroid1@chromium.org, Aug 19 2016

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

commit 12c77d78dd3626b4e91664cfebd293a54b3e6d81
Author: lazyboy <lazyboy@chromium.org>
Date: Fri Aug 19 20:06:09 2016

Make UserScript non-copyable.

The main purpose of this CL is to get rid of copying around script
file contents (awful!). This should also improve content_scripts's
performance, by a little bit at least.

For places we must create a copy (e.g. creating a UserScript
from manifest metadata), provide a UserScript::CopyMetadataFrom()
method. This method does not copy files' contents within the
UserScript.

*UserScriptLoader is now responsible for owning the UserScript-s
in the browser/ process. All such ownerships are done via
unique_ptr-s.

BUG= 622464 
Test=Refactoring only, no visible change should happen.
Watch out for any extensions content_scripts related regression.

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

[modify] https://crrev.com/12c77d78dd3626b4e91664cfebd293a54b3e6d81/chrome/browser/chromeos/accessibility/accessibility_manager.cc
[modify] https://crrev.com/12c77d78dd3626b4e91664cfebd293a54b3e6d81/chrome/browser/extensions/api/declarative_content/content_action.cc
[modify] https://crrev.com/12c77d78dd3626b4e91664cfebd293a54b3e6d81/chrome/browser/extensions/api/declarative_content/content_action.h
[modify] https://crrev.com/12c77d78dd3626b4e91664cfebd293a54b3e6d81/chrome/browser/extensions/convert_user_script_unittest.cc
[modify] https://crrev.com/12c77d78dd3626b4e91664cfebd293a54b3e6d81/chrome/browser/extensions/extension_service_unittest.cc
[modify] https://crrev.com/12c77d78dd3626b4e91664cfebd293a54b3e6d81/chrome/browser/extensions/extension_user_script_loader_unittest.cc
[modify] https://crrev.com/12c77d78dd3626b4e91664cfebd293a54b3e6d81/chrome/browser/extensions/shared_user_script_master.cc
[modify] https://crrev.com/12c77d78dd3626b4e91664cfebd293a54b3e6d81/chrome/browser/extensions/shared_user_script_master.h
[modify] https://crrev.com/12c77d78dd3626b4e91664cfebd293a54b3e6d81/chrome/browser/extensions/user_script_listener.cc
[modify] https://crrev.com/12c77d78dd3626b4e91664cfebd293a54b3e6d81/chrome/common/extensions/manifest_handlers/content_scripts_handler.cc
[modify] https://crrev.com/12c77d78dd3626b4e91664cfebd293a54b3e6d81/chrome/common/extensions/manifest_handlers/content_scripts_manifest_unittest.cc
[modify] https://crrev.com/12c77d78dd3626b4e91664cfebd293a54b3e6d81/extensions/browser/api/guest_view/web_view/web_view_internal_api.cc
[modify] https://crrev.com/12c77d78dd3626b4e91664cfebd293a54b3e6d81/extensions/browser/declarative_user_script_master.cc
[modify] https://crrev.com/12c77d78dd3626b4e91664cfebd293a54b3e6d81/extensions/browser/declarative_user_script_master.h
[modify] https://crrev.com/12c77d78dd3626b4e91664cfebd293a54b3e6d81/extensions/browser/extension_user_script_loader.cc
[modify] https://crrev.com/12c77d78dd3626b4e91664cfebd293a54b3e6d81/extensions/browser/guest_view/web_view/web_view_content_script_manager.cc
[modify] https://crrev.com/12c77d78dd3626b4e91664cfebd293a54b3e6d81/extensions/browser/guest_view/web_view/web_view_content_script_manager.h
[modify] https://crrev.com/12c77d78dd3626b4e91664cfebd293a54b3e6d81/extensions/browser/user_script_loader.cc
[modify] https://crrev.com/12c77d78dd3626b4e91664cfebd293a54b3e6d81/extensions/browser/user_script_loader.h
[modify] https://crrev.com/12c77d78dd3626b4e91664cfebd293a54b3e6d81/extensions/browser/web_ui_user_script_loader.cc
[modify] https://crrev.com/12c77d78dd3626b4e91664cfebd293a54b3e6d81/extensions/browser/web_ui_user_script_loader.h
[modify] https://crrev.com/12c77d78dd3626b4e91664cfebd293a54b3e6d81/extensions/common/user_script.cc
[modify] https://crrev.com/12c77d78dd3626b4e91664cfebd293a54b3e6d81/extensions/common/user_script.h
[modify] https://crrev.com/12c77d78dd3626b4e91664cfebd293a54b3e6d81/extensions/common/user_script_unittest.cc
[modify] https://crrev.com/12c77d78dd3626b4e91664cfebd293a54b3e6d81/extensions/renderer/user_script_injector.cc
[modify] https://crrev.com/12c77d78dd3626b4e91664cfebd293a54b3e6d81/extensions/renderer/user_script_injector.h
[modify] https://crrev.com/12c77d78dd3626b4e91664cfebd293a54b3e6d81/extensions/renderer/user_script_set.cc
[modify] https://crrev.com/12c77d78dd3626b4e91664cfebd293a54b3e6d81/extensions/renderer/user_script_set.h

Project Member

Comment 8 by bugdroid1@chromium.org, Aug 22 2016

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

commit de26faaca0525f21a71dfd440d113069b7fb74ba
Author: lazyboy <lazyboy@chromium.org>
Date: Mon Aug 22 18:36:28 2016

Make GreasemonkeyApiJsString truly singleton.

Before this change, ::GetSource() would create a new string from
StringPiece every time.

BUG= 622464 

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

[modify] https://crrev.com/de26faaca0525f21a71dfd440d113069b7fb74ba/extensions/renderer/user_script_injector.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Aug 22 2016

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

commit de26faaca0525f21a71dfd440d113069b7fb74ba
Author: lazyboy <lazyboy@chromium.org>
Date: Mon Aug 22 18:36:28 2016

Make GreasemonkeyApiJsString truly singleton.

Before this change, ::GetSource() would create a new string from
StringPiece every time.

BUG= 622464 

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

[modify] https://crrev.com/de26faaca0525f21a71dfd440d113069b7fb74ba/extensions/renderer/user_script_injector.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Sep 7 2016

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

commit 0f702e9e3ccd2f408b198d2b2255d363da0310b9
Author: lazyboy <lazyboy@chromium.org>
Date: Wed Sep 07 19:03:45 2016

Stop copying script contents for each RenderFrames.

Instead, after unpickling the script content, put them in a WebString
on its first use (GetJsSources) in a frame. Reuse them for subsequent
frames.

The cleanup is done when UserScriptSet releases its shared memory
connection.

A completely naive test shows that on WSJ site, with ABPlus extension
installed, this reduced (24025 * 11 + 14785 * 10) = 412125 char copies!

BUG= 622464 
Test=No visible change is expected.

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

[modify] https://crrev.com/0f702e9e3ccd2f408b198d2b2255d363da0310b9/extensions/renderer/user_script_injector.cc
[modify] https://crrev.com/0f702e9e3ccd2f408b198d2b2255d363da0310b9/extensions/renderer/user_script_injector.h
[modify] https://crrev.com/0f702e9e3ccd2f408b198d2b2255d363da0310b9/extensions/renderer/user_script_set.cc
[modify] https://crrev.com/0f702e9e3ccd2f408b198d2b2255d363da0310b9/extensions/renderer/user_script_set.h

Project Member

Comment 11 by bugdroid1@chromium.org, Sep 15 2016

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

commit b19fcfe287dbb41ba8ee1e09d660cb27b14d1590
Author: lazyboy <lazyboy@chromium.org>
Date: Thu Sep 15 22:47:57 2016

Remove some UI->FILE->UI thread hops in ExecuteCodeFunction.

There are 3 types of tasks in ExecuteCodeFunction that require FILE thread:
a) Reading the file content (for non-component extension files)
b) Retrieving the file GURL of the script file from base::FilePath
c) content l10n (if the script is CSS)

This CL does all of these in on UI->FILE hop and doesn't require
additional FILE thread hop.
Previously, the most common case, i.e. chrome.tabs.executeScript()
with {file:...}, would require UI->FILE->UI->FILE->UI. With this CL
this become UI->FILE->UI.

This CL accomplishes this with adding an optional callback to FileReader,
to specify additional tasks to be run on FILE thread.

So the thread hop changes are:
Component extension's executeScript() with file:... url would stay same.
*All* other means of executing script file:... url, e.g.
chrome.tabs.executeScript(,{file:..}) would require two (UI->FILE->UI) less
thread hops, whee!

BUG= 622464 
Test=None, internal only change.

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

[modify] https://crrev.com/b19fcfe287dbb41ba8ee1e09d660cb27b14d1590/chrome/browser/chromeos/accessibility/accessibility_extension_loader.cc
[modify] https://crrev.com/b19fcfe287dbb41ba8ee1e09d660cb27b14d1590/extensions/browser/api/execute_code_function.cc
[modify] https://crrev.com/b19fcfe287dbb41ba8ee1e09d660cb27b14d1590/extensions/browser/api/execute_code_function.h
[modify] https://crrev.com/b19fcfe287dbb41ba8ee1e09d660cb27b14d1590/extensions/browser/file_reader.cc
[modify] https://crrev.com/b19fcfe287dbb41ba8ee1e09d660cb27b14d1590/extensions/browser/file_reader.h
[modify] https://crrev.com/b19fcfe287dbb41ba8ee1e09d660cb27b14d1590/extensions/browser/file_reader_unittest.cc

Status: Fixed (was: Assigned)
I have landed all the CLs I had planned for this.
Closing the bug.

Sign in to add a comment