Safari uses 20ms for the "focused tab" and 250ms for the rest. We use 17ms for all plugins in all tabs in all windows. Following their lead would reduce the amount of CPU used by Firefox (incl plugins) for the common case of plugins like Flash in numerous background tabs/windows. There's apparently some controversy around the case in which a Flash animation is visible in a background window, and slows down its animation rate. We could restrict our throttling to the hidden-tab case, do visibility testing if that's feasible in our world, or just join our WebKit brothers in solidarity. (mjs also points out that we could rev the Plugin API to let plugins disable the dispatch of idleEvents, and then phase it out over time.)

Simon Fraser and I were talking about adjusting the idleEvent timer about a year ago but we never really decided on a good plan because of how complex this issue is. I would be happy to look into this when I look into adding support for the GetEntryPoints API, which is another area in which we differ from Safari. We should run any plans we come up with by people on the macplugin-dev mailing list.

This uses 17ms for plugins in the foremost tab of every window and 100ms in tabs that are hidden. Using 250ms for those causes audio slowdowns. Safari must be doing some other weird stuff because some plugins that always play audio for me in Firefox don't play audio when they are hidden behind a tab in Safari. I'm just posting this patch for people to play with if they want, I'm not sure its what I want to do yet. Another weird thing about this patch is that we don't appear to be saving much CPU, even with 10 plugin-full tabs open. I'm going to try some more tests and maybe another monitoring tool (I am using Activity Monitor now).

I figured out what was wrong with my tests. The Flash plugin loaded into a particular browser instance will max out at 100% of a single CPU. In my tests I was maxing it out constantly so nothing I did made a difference because it really wanted to use about 400% of the CPU but couldn't. I made a new test that works much better. I open a single window with a single tab showing yugop.com, a page with a large Flash plugin. At the point Firefox is taking up about 75% CPU constantly. Without my patch applied, when I open a new tab to cover up yugop.com the browser uses about 65% CPU. A small drop that happened most likely because the plugin is not blitting to screen. With my patch applied when you cover up yugop.com with a blank tab browser CPU usage drops to about 30%, at 50% improvement over the unpatched case.

Comment on attachment 235770 [details] [diff] [review] fix v1.0 requesting review, some tabs slipped into the patch, I know about that already

Here is what I noticed from my testing (granted this is from a trunk build from 8/21, sorry if it's a little stale) 1. I opened Minefield and pointed the browser to www.google.com, page load dropped to around 0% once the page loaded. 2. I opened a new tab with www.ign.com (has a resource heavy flash component), then refocused the google tab. I watched the CPU ping around 20-30% while that page was in the background. 3. I closed the IGN tab (which left only the google tab open) and cpu usage dropped back down to around 0%. Hope this helps a little.

(In reply to comment #5) > Here is what I noticed from my testing (granted this is from a trunk build from > 8/21, sorry if it's a little stale) > > 1. I opened Minefield and pointed the browser to www.google.com, page load > dropped to around 0% once the page loaded. > > 2. I opened a new tab with www.ign.com (has a resource heavy flash component), > then refocused the google tab. I watched the CPU ping around 20-30% while that > page was in the background. > > 3. I closed the IGN tab (which left only the google tab open) and cpu usage > dropped back down to around 0%. > > Hope this helps a little. > Trying the same test w/o the patch resulted this: 1. With IGN in the background: 30-55% 2. With IGN in the foreground: 45-65%

Comment on attachment 235770 [details] [diff] [review] fix v1.0 >Index: nsObjectFrame.cpp >+// 1020 / 60 >+#define NORMAL_PLUGIN_DELAY 17 >+// low enough to avoid audio skipping/delays >+#define HIDDEN_PLUGIN_DELAY 100 If you know what's significant about 1020/60, add it to the comment. I didn't find anything going back a couple of revisions in cvs, though. Could we get a blank line between the two #defines so that it's clear which comment applies to which #define? >+void nsPluginInstanceOwner::StartTimer(unsigned int aDelay) > { > #ifdef XP_MACOSX > nsresult rv; > > // start a periodic timer to provide null events to the plugin instance. > if (!mPluginTimer) { > mPluginTimer = do_CreateInstance("@mozilla.org/timer;1", &rv); > if (NS_SUCCEEDED(rv)) >- rv = mPluginTimer->InitWithCallback(this, 1020 / 60, nsITimer::TYPE_REPEATING_SLACK); >+ rv = mPluginTimer->InitWithCallback(this, aDelay, nsITimer::TYPE_REPEATING_SLACK); We don't care about this last rv, you can get rid of the assignment. > } > #endif > } >+ // if the clipRect is of size 0, make the null timer fire less often You've mixed tabs and spaces throughout this block. >+ if (mPluginWindow->clipRect.left == mPluginWindow->clipRect.right || >+ mPluginWindow->clipRect.top == mPluginWindow->clipRect.bottom) { >+ CancelTimer(); Move the CancelTimer()s above the if/else. >+ StartTimer(HIDDEN_PLUGIN_DELAY); >+ } >+ else { >+ CancelTimer(); >+ StartTimer(NORMAL_PLUGIN_DELAY); >+ } >+ }

Comment on attachment 235770 [details] [diff] [review] fix v1.0 sr=jst

landed on trunk

(In reply to comment #7) > (From update of attachment 235770 [details] [diff] [review] [edit]) > >Index: nsObjectFrame.cpp > >+// 1020 / 60 > >+#define NORMAL_PLUGIN_DELAY 17 > >+// low enough to avoid audio skipping/delays > >+#define HIDDEN_PLUGIN_DELAY 100 > > If you know what's significant about 1020/60, add it to the comment. I didn't > find anything going back a couple of revisions in cvs, though. I believe this harks back to the frequency of idle events you get from WaitNextEvent(), back when plugins were fed idle events directly form the main event loop in 4.x.

as raised in todays meeting with Brad Fitzpatrick, this should help. note, this fix is on the trunk and not on the MOZILLA_1_8_BRANCH (so it is not part of Firefox 2.0.0.x)

josh / drivers, what do you think about about taking this on the MOZILLA_1_8_BRANCH?

probably not too hard to port to the 1.8 branch, but why do we want this now?

If it's safe it will address a common complaint from Mac users. Firefox 2 is our main supported platform for the next 4-6 months until FF3 ships. FF3 betas don't count for the vast majority of people.

I don't think it is clear that this would be very safe, and we've had this problem forever until my trunk fix. It isn't like it is new with Firefox 2. I'd totally be willing to try it though and see if it works out, however I don't think I can make this a priority over 1.9 blockers right now.

Too close to the end of 1.8.1.8 to take this time.

Shaver: do you want to fight for this one on the 1.8 branch? or just leave it as a Firefox 3 improvement? There have been claims of improvement, but Josh appears to think this is risky.

Have regressions been noted on the trunk since this landed?

I don't know of any, and given that we're following Safari's lead here I'm personally pretty comfortable with it. I'd certainly want to hear Josh's concerns in more detail, though, before deciding that I didn't share them! :)

I can't recall any regressions from this, my concern is mainly just that this is a very touchy part of our code. Messing around in there is dangerous pretty much no matter what you are doing. To ship a patch like this on the 1.8 branch we have to really be sure it is worth that risk. If you think it is, then I'm not opposed.

Not blocking 1.8.1.x, we'll leave this as a FF3 improvement.