Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2278003002:
Stop copying script contents for each RenderFrames. (Closed)
|
Created:
4 years, 3 months ago by lazyboy Modified:
4 years, 3 months ago Reviewers:
Devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, catmullings Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionStop 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.
Committed: https://crrev.com/0f702e9e3ccd2f408b198d2b2255d363da0310b9
Cr-Commit-Position: refs/heads/master@{#417018}
Patch Set 1 #
Total comments: 6
Patch Set 2 : use map GURL->WebString #Patch Set 3 : fix WebScriptSource constructor param #
Total comments: 4
Patch Set 4 : address comments from Devlin #
Messages
Total messages: 32 (21 generated)
lazyboy@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
rdevlin.cronin@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
catmullings@ cc'd as FYI, since she's also working in this area. https://codereview.chromium.org/2278003002/diff/1/extensions/renderer/user_sc... File extensions/renderer/user_script_set.cc (right): https://codereview.chromium.org/2278003002/diff/1/extensions/renderer/user_sc... extensions/renderer/user_script_set.cc:265: std::vector<blink::WebScriptSource> UserScriptSet::GetJsSources(int script_id) { Semi-random placement: This is going to conflict with catmullings@'s patch at https://codereview.chromium.org/2213603002/ to prevent duplicate script injection. The idea there is that we avoid injecting a subset of the files if they've already been injected in the frame. For that, we need to be able to construct a vector with a subset of the script files. The easiest solution to this would be to change this method to just be a lookup for a given file's url, so that the logic for what to inject remains in the UserScriptInjector, and the UserScriptSet just stores all the cached webstrings for a given file (that also obviates the need for separate js/css containers). It means the lookup is slightly less of a win (because we don't get the instant vector), but I think that cost is reasonably small in the long run. WDYT? https://codereview.chromium.org/2278003002/diff/1/extensions/renderer/user_sc... extensions/renderer/user_script_set.cc:267: std::find_if(scripts_.begin(), scripts_.end(), In the past, I've found that I've been too gung-ho to use the new c++11 fanciness in situations like this. But at the end of the day, usually it's clearer to do something like: UserScript* script = nullptr; for (const auto& iter : scripts_) { if (script->id() == script_id) { script = iter.get(); break; } } It's one line more, but I find it significantly more readable, and I'd bet it's also faster (since it avoids calling into the lambda function). But, that's also just my opinion, so I'm fine with this too :) https://codereview.chromium.org/2278003002/diff/1/extensions/renderer/user_sc... File extensions/renderer/user_script_set.h (right): https://codereview.chromium.org/2278003002/diff/1/extensions/renderer/user_sc... extensions/renderer/user_script_set.h:101: // Map of user script id -> list of JavaScript sources. nit: not js :)
https://codereview.chromium.org/2278003002/diff/1/extensions/renderer/user_sc... File extensions/renderer/user_script_set.cc (right): https://codereview.chromium.org/2278003002/diff/1/extensions/renderer/user_sc... extensions/renderer/user_script_set.cc:265: std::vector<blink::WebScriptSource> UserScriptSet::GetJsSources(int script_id) { On 2016/08/25 17:07:35, Devlin wrote: > Semi-random placement: This is going to conflict with catmullings@'s patch at > https://codereview.chromium.org/2213603002/ to prevent duplicate script > injection. The idea there is that we avoid injecting a subset of the files if > they've already been injected in the frame. For that, we need to be able to > construct a vector with a subset of the script files. > > The easiest solution to this would be to change this method to just be a lookup > for a given file's url, so that the logic for what to inject remains in the > UserScriptInjector, and the UserScriptSet just stores all the cached webstrings > for a given file (that also obviates the need for separate js/css containers). > It means the lookup is slightly less of a win (because we don't get the instant > vector), but I think that cost is reasonably small in the long run. > > WDYT? I've changed the storage to map GURL-> WebString. https://codereview.chromium.org/2278003002/diff/1/extensions/renderer/user_sc... extensions/renderer/user_script_set.cc:267: std::find_if(scripts_.begin(), scripts_.end(), On 2016/08/25 17:07:35, Devlin wrote: > In the past, I've found that I've been too gung-ho to use the new c++11 > fanciness in situations like this. But at the end of the day, usually it's > clearer to do something like: > UserScript* script = nullptr; > for (const auto& iter : scripts_) { > if (script->id() == script_id) { > script = iter.get(); > break; > } > } > > It's one line more, but I find it significantly more readable, and I'd bet it's > also faster (since it avoids calling into the lambda function). Acknowledged, this is no longer relevant. > > But, that's also just my opinion, so I'm fine with this too :) https://codereview.chromium.org/2278003002/diff/1/extensions/renderer/user_sc... File extensions/renderer/user_script_set.h (right): https://codereview.chromium.org/2278003002/diff/1/extensions/renderer/user_sc... extensions/renderer/user_script_set.h:101: // Map of user script id -> list of JavaScript sources. On 2016/08/25 17:07:35, Devlin wrote: > nit: not js :) Acknowledged.
The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Looks like there are a few failing tests?
On 2016/09/06 20:33:33, Devlin wrote: > Looks like there are a few failing tests? Fixed (WebScriptSource ctor's file url parameter got dropped by accident).
The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm! https://codereview.chromium.org/2278003002/diff/40001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2278003002/diff/40001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:216: sources.insert(sources.begin(), g_greasemonkey_api.Get().GetSource()); nit: unrelated, but maybe put this before adding all the js scripts so we don't have to move them all? https://codereview.chromium.org/2278003002/diff/40001/extensions/renderer/use... File extensions/renderer/user_script_set.cc (right): https://codereview.chromium.org/2278003002/diff/40001/extensions/renderer/use... extensions/renderer/user_script_set.cc:26: #include "grit/extensions_renderer_resources.h" don't need these includes anymore, right?
The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run
https://codereview.chromium.org/2278003002/diff/40001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2278003002/diff/40001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:216: sources.insert(sources.begin(), g_greasemonkey_api.Get().GetSource()); On 2016/09/07 14:29:45, Devlin wrote: > nit: unrelated, but maybe put this before adding all the js scripts so we don't > have to move them all? Done. https://codereview.chromium.org/2278003002/diff/40001/extensions/renderer/use... File extensions/renderer/user_script_set.cc (right): https://codereview.chromium.org/2278003002/diff/40001/extensions/renderer/use... extensions/renderer/user_script_set.cc:26: #include "grit/extensions_renderer_resources.h" On 2016/09/07 14:29:45, Devlin wrote: > don't need these includes anymore, right? Yes. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lazyboy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2278003002/#ps60001 (title: "address comments from Devlin")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== 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. ========== to ========== 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. Committed: https://crrev.com/0f702e9e3ccd2f408b198d2b2255d363da0310b9 Cr-Commit-Position: refs/heads/master@{#417018} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/0f702e9e3ccd2f408b198d2b2255d363da0310b9 Cr-Commit-Position: refs/heads/master@{#417018}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2325053002/ by lazyboy@chromium.org. The reason for reverting is: Crashes renderer while destroying UserScriptSet via ChromeExtensionsRendererClient LazyInstance.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
