avoid duplication of source strings #2243

Closed
wants to merge 2 commits into
from

Projects

None yet

2 participants

@the8472
Contributor
the8472 commented Aug 18, 2015

Currently script sources get cloned into every sandbox in which a script runs via the GM_info structure. Since script sources can be fairly large this results in quite some memory overhead.

Additionally in e10s the scripts are passed down via IPC as new string instances every time they're needed, thus resulting in duplication even before the GM_info instances are created.

I've solved that by de-duping identical string instances in IPCScript and by installing a cross-compartment getter for the GM_info.scriptSource property.

Should lead to significant memory reductions, especially for large scripts and/or scripts that get loaded into every tab and frame.

@arantius arantius added this to the 3.4 milestone Aug 19, 2015
@arantius arantius and 1 other commented on an outdated diff Aug 19, 2015
modules/ipcscript.js
@@ -48,6 +50,21 @@ function IPCScript_getMetaStr() {
return this._metaStr;
});
+IPCScript.prototype.dedup = function() {
+ // scripts sent via e10s message manager contain new string instances every time a new one is created -> dedup them to reduce memory footprint
+
+ // alternative approaches:
+ // a) beam down IPC script instances only once via the parent process manager every time they change.
+ // b) creating resource URIs and loading user scripts straight from URIs might also be more elegant
+ // see: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Using#Programmatically_adding_aliases
+ var old = instances.get(this.uuid);
+
+ if(old == this.textContent)
+ this.textContent = old
+ else
+ instances.set(this.uuid, this.textContent)
@arantius
arantius Aug 19, 2015 Collaborator

When are instances unset?

@the8472
the8472 Aug 19, 2015 Contributor

on the assumption that uninstalling scripts is a rather rare use-case and updating/modifying them is more likely it simply overwrites them based on UUID. removal would require additional communication with the parent process since the child process otherwise has no way of knowing.

@arantius
arantius Aug 19, 2015 Collaborator

So before this change, the script string is in memory only while the script is executing/is in scope. It is garbage collected once it goes out of scope (i.e. navigation to a different page, close the tab, or as soon as it finishes executing if it doesn't expose references to itself to content).

After this change, every script that has ever executed has a copy of its source stored permanently in memory. When there are several children processes (there is only one today AIUI) there will be several copies, and none of them will ever go away until the browser is completely closed.

It's not clear to me whether this new approach is better overall in terms of memory usage.

@the8472
the8472 Aug 19, 2015 Contributor

Mhh, good point, I was basing things on behavior I observed in my profile, where everything is fairly long-lived anyway.

The scenario you outline seems plausible for other types of script. I'll see If i can implement option a)

But for that one bit of information would be useful: Can I rely on all changes to Script instances notifying service.config.addObserver()

@arantius
arantius Aug 19, 2015 Collaborator

Changes to scripts should always be notifying the Config.

But option A sounds a lot like "store the content of every script in memory forever" again, in a slightly different arrangement. I'm curious what we can do with lazy getters and either sync messages or however that resource URI thing would work.

@the8472
the8472 Aug 19, 2015 Contributor

Hrrm, wait... you want scripts to be GCed once they're not used anymore by a child process.

I guess some weakmap magic + keeping instances alive as long as they're referenced by the respective sandboxes might work.

@arantius
Collaborator

Please don't skip optional braces or semicolons.

@the8472
Contributor
the8472 commented Aug 19, 2015

pushed new approach using lazy getters and loading via file:// uris. I didn't actually have to do any resource:// registration, mozilla seems to be doing about the right thing, sharing instances from the same path.

Scripts touching textContent will lead to duplicate strings for the lifetime of the sandboxes, but as long as they don't it saves memory.

Dependency loading also seems to work.

What still needs testing is resource-loading in e10s, none of my scripts use that.

@the8472 the8472 avoid holding duplicate script source strings in memory
old: strings sent via e10s message manager are duplicated and the
sandbox holds onto them as source code, this causes unnecessary memory
overhead.

new: load scripts from file:// URIs and use lazy getters for
scriptSource
70a16c4
@the8472
Contributor
the8472 commented Aug 21, 2015

resource loading should work now

@arantius arantius modified the milestone: 3.5, 3.4 Aug 24, 2015
@arantius
Collaborator

Started looking into this, I'm not happy with it. The whole concept of fileXhr seems brittle to me. Mozilla has explained that they plan to continue e10s by making it impossible for the child process to (e.g.) access local files at all. The fact that XHR happens to be a workaround to blockage of nsIFile doesn't stop this. One day this change will (probably?) fail to work correctly, and we'll be left scrambling.

@the8472
Contributor
the8472 commented Aug 24, 2015

This page states that file:// will always load in content, so I would expect this to work in the future too.

I think mozilla wants to prevent direct system calls to file handles (open() and the like), using XHR just passes the input stream to the content process AIUI, not the file handle itself.

So it doesn't seem obvious to me that they'll remove file:// XHR

@the8472
Contributor
the8472 commented Aug 25, 2015

also asked one of the devs:

<The_8472> billm, file: URIs will continue to work though, yes?
<billm> The_8472: sure
@arantius
Collaborator

Is this implicit in #2259 and safe to ignore if that's merged?

@the8472
Contributor
the8472 commented Sep 24, 2015

Yes

@the8472 the8472 closed this Sep 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment