500925 - don't unload plugins as soon as possible by default

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Josh Aas]

Right now we unload plugins as soon as their last instance goes away, then load them again when a new instance starts. This, for example, means that if you go from one page with a Flash instance to another page with a Flash instance, the plugin is shut down and unloaded upon leaving the first page and restarted when loading the second. I believe this is a big part of why people report so much plugin i/o and loading/unloading activity, and why we have problems like bug 397053 while other browser don't.

Other sources of load/unload activity at unexpected times are upon launch, where we potentially scan, and when loading/unloading happens on timers for various reasons.

This bug is about improving our unloading behavior so that we don't load/unload so often.

Comment 1

[Josh Aas]

Attachment: fix v1.0

This patch changes our default behavior to leaving plugins loaded once they are used for the first time. I've added a pref, "plugins.unloadASAP', to revert to our current behavior of unloading plugins as soon as they have no active instances.

This is probably a good start, memory-pressured platforms such as mobile may want to set "plugins.unloadASAP" to "true".

Another thing we could do is unload plugins only after they have had no instances for a certain period of time.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #0)
> Right now we unload plugins as soon as their last instance goes away, then load
> them again when a new instance starts. This, for example, means that if you go
> from one page with a Flash instance to another page with a Flash instance, the
> plugin is shut down and unloaded upon leaving the first page and restarted when
> loading the second.

This wouldn't happen every time, because if the plugin's page lands in the bfcache then we'd keep the plugin loaded. But I agree that this is definitely something we should fix.

I think the best approach would be to unload a plugin after it hasn't been used for, say, ten minutes.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I thought we tore down plugin instances when going into bfcache.  At least last I checked....

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm. You're right, of course.

Comment 5

[Josh Aas]

Attachment: fix v1.1

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 385907 [details] [diff] [review]
fix v1.1

I assume the only difference from the first patch is that you removed the unused boolean arg?

If so, this looks ok, but jst should look at this too.  And please do file a bug on unloading at some point; I do think we want to do this.

Comment 7

[Josh Aas]

(In reply to comment #6)
> (From update of attachment 385907 [details] [diff] [review])
> I assume the only difference from the first patch is that you removed the
> unused boolean arg?

Right.

> If so, this looks ok, but jst should look at this too.  And please do file a
> bug on unloading at some point; I do think we want to do this.

Yup.

Comment 8

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/61a70b55686d

Comment 9

[Josh Aas]

I filed bug 501485 about finding a new unloading strategy.

Comment 11

[Johnny Stenback (:jst)]

Attachment: Fix for 1.9.1

This is the same thing as the fix we took on the trunk but ported to 1.9.1. The hope is that this might fix bug 501429, and we should take this for 3.5.5 to find out (no other way to find out it seems, nobody can seem to reproduce this crash).

Comment 12

[Daniel Veditz [:dveditz]]

Would this have fixed bug 493601?

Comment 13

[Johnny Stenback (:jst)]

I don't think so, no. This bug is about us never unloading the actual DLL (which is the behavior we want), but we still destroy plugin instances. I think bug 493601 really talks about plugin instance destruction rather than plugin library unloading.

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 407617 [details] [diff] [review]
Fix for 1.9.1

Approved for 1.9.1.5, a=dveditz for release-drivers

Shouldn't we have a default pref value in all.js for the new pref? Or do you mean  plugins.unloadASAP to be not only hidden but stealth?

Comment 15

[Johnny Stenback (:jst)]

Landed on 1.9.1. dveditz, the pref is stealth everywhere else as well...

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0be7c0bfe443

Comment 16

[Serge Gautherie (:sgautherie)]

(In reply to comment #15)
> Landed on 1.9.1.

This made reftest and mochitest-plain leaks! Firefox and SeaMonkey!
Please back out or fix...

Comment 17

[Serge Gautherie (:sgautherie)]

(In reply to comment #16)
> This made reftest and mochitest-plain leaks! Firefox and SeaMonkey!

On MacOSX (only).

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Also implicated in a Tp4 RSS and Tp4 Private Bytes increase on Linux, see dev-tree-management:

http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/ce2a4e29550f6673#

http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/f1305e1d7f268005#

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

And ~10% memory regressions on Windows as well:

http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/66da50ef00185968#

http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/0b152e23d559b005#

From the title of this patch, I don't know if this was unexpected (I mean, we're unloading later). It's possible that this is an artifact of how we measure memory with Talos, but we need to make sure we're not actually regressing memory usage in a way that users will feel without first discussing the pros and cons.

Comment 20

[Mike Shaver (:shaver emeritus)]

We should probably unload eagerly after initial-startup plugin scan.  I suspect that that would give us back the memory here, since I don't think our test actually trigger plugin load, and would keep users from having all the plugins on their system loaded for 10 mins or whatever.

(We should maybe stop scanning plugins on startup too, for perf reasons, but that's another bug that I can't remember the number of right now.)

Comment 21

[Benjamin Smedberg]

Tp4 does load Flash, I'm told.

We shouldn't be scanning plugins until/unless we have to handle an unknown MIME type and we haven't got saved plugin information from a previous run.

Comment 22

[Jesse Ruderman]

I think we already don't scan plugins at startup (bug 506472?).

Comment 23

[Johnny Stenback (:jst)]

(In reply to comment #16)
> From the title of this patch, I don't know if this was unexpected (I mean,
> we're unloading later). It's possible that this is an artifact of how we
> measure memory with Talos, but we need to make sure we're not actually
> regressing memory usage in a way that users will feel without first discussing
> the pros and cons.

I don't know what the numbers were like when this change landed on the trunk, but they should've been the same. We're in fact *not* unloading these plugin libraries later (until the OS unloads them on application exit), which is why we see the increase in memory usage when measured as our graphs show. I believe there's a bug on doing something like unloading plugins after n minutes of the last plugin instance being destroyed, but I don't have that bug number handy, and I don't expect that to ever be backported to 1.9.1, or even 1.9.2. And just to be clear here, once a user leaves a page that used plugin x, the memory that plugin x allocated, and the memory we allocated for the plugin instance does get freed (modulo actual memory leaks) like before, we just don't unload the library since we believe we're likely to use it again soon, which is the common case by a long stretch.

Comment 24

[Johnny Stenback (:jst)]

(In reply to comment #20)
> We should probably unload eagerly after initial-startup plugin scan.  I suspect
> that that would give us back the memory here, since I don't think our test
> actually trigger plugin load, and would keep users from having all the plugins
> on their system loaded for 10 mins or whatever.

AFAIK, the only system where we actually load plugins on startup is on unix systems, and there we only load any plugins that we find and haven't seen before (per pluginreg.dat in the profile), so for regular users we basically never load all plugins, unless they visit pages that acutally use all their plugins. On other systems we extract the info we need about plugins w/o actually loading the plugin library, so no unloading ever needed due to startup.

> (We should maybe stop scanning plugins on startup too, for perf reasons, but
> that's another bug that I can't remember the number of right now.)

Sure.

Comment 25

[Jo Hermans]

(In reply to comment #23) 
> I don't know what the numbers were like when this change landed on the trunk,
> but they should've been the same. We're in fact *not* unloading these plugin
> libraries later (until the OS unloads them on application exit), which is why
> we see the increase in memory usage when measured as our graphs show. I believe
> there's a bug on doing something like unloading plugins after n minutes of the
> last plugin instance being destroyed, but I don't have that bug number handy,

bug 501485

> and I don't expect that to ever be backported to 1.9.1, or even 1.9.2. And just
> to be clear here, once a user leaves a page that used plugin x, the memory that
> plugin x allocated, and the memory we allocated for the plugin instance does
> get freed (modulo actual memory leaks) like before, we just don't unload the
> library since we believe we're likely to use it again soon, which is the common
> case by a long stretch.

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 407617 [details] [diff] [review]
Fix for 1.9.1

>-void nsPluginTag::TryUnloadPlugin(PRBool aForceShutdown)
>+void nsPluginTag::TryUnloadPlugin()
> {
>   PRBool isXPCOM = PR_FALSE;
>   if (!(mFlags & NS_PLUGIN_FLAG_NPAPI))
>     isXPCOM = PR_TRUE;
> 
>-  if (isXPCOM && !aForceShutdown) return;
>+  if (isXPCOM) return;

jst, when this parameter was removed on trunk, it was unused. On the 1.9.1 branch, though, it was actually used so that we knew when we could release (amongst other XPCOM plugins) Java. I think this is responsible for the leaks found in <http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.5-Unittest/1257211455.1257214299.2063.gz>.

Comment 27

[Johnny Stenback (:jst)]

Yes, I have a half baked patch to deal with the leaks, just haven't had time to make sure it's exactly what I want here.

Comment 28

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1-SM) Add leak threshold ftb [Superseded by bug 526277]

2 weeks of orange is enough :-(

Comment 29

[Reed Loden [:reed]]

Comment on attachment 410716 [details] [diff] [review]
(Bv1-SM) Add leak threshold ftb [Superseded by bug 526277]

Just go land bug 526277.

Comment 30

[Serge Gautherie (:sgautherie)]

(In reply to comment #29)
> (From update of attachment 410716 [details] [diff] [review])
> Just go land bug 526277.

Thanks for the hint!
(How are we supposed to find out about such bugs not linked from anywhere? :-<)