avoid duplication of source strings #2243
Conversation
| if(old == this.textContent) | ||
| this.textContent = old | ||
| else | ||
| instances.set(this.uuid, this.textContent) |
There was a problem hiding this comment.
When are instances unset?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Please don't skip optional braces or semicolons. |
|
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 Dependency loading also seems to work. What still needs testing is resource-loading in e10s, none of my scripts use that. |
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
|
resource loading should work now |
|
Started looking into this, I'm not happy with it. The whole concept of |
|
This page states that I think mozilla wants to prevent direct system calls to file handles ( So it doesn't seem obvious to me that they'll remove |
|
further reading: https://bugzilla.mozilla.org/show_bug.cgi?id=922481 |
|
also asked one of the devs: |
|
Is this implicit in #2259 and safe to ignore if that's merged? |
|
Yes |
Currently script sources get cloned into every sandbox in which a script runs via the
GM_infostructure. 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_infoinstances are created.I've solved that by de-duping identical string instances in
IPCScriptand by installing a cross-compartment getter for theGM_info.scriptSourceproperty.Should lead to significant memory reductions, especially for large scripts and/or scripts that get loaded into every tab and frame.