519598 - 15% of our time on gmail is spent firing the plugin instance timer

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Boris Zbarsky [:bzbarsky]]

If I load gmail in 4 tabs in a mac build and profile our 10% or so resulting cpu usage, 15% of that is under nsPluginInstanceOwner::Notify(nsITimer*).  

It looks like the method only does something on mac, and it has a "// Here's how we give idle time to plugins." comment.  If we really need this timer, we should either make this method extremely fast in the hopefully usual no-op case or not fire it as often.  Right now we fire it every 17ms (the comment for this is "1020 / 60"; presumably the idea is to fire at close to 60Hz) for "non-hidden" plug-ins and every 100ms otherwise.  The gmail flash plug-ins are getting the 100ms treatment (verified in gdb), but there are 4 of them in each gmail instance.  So we end up doing Notify 40 times a second per gmail instance; 160 times a second total for the steps to reproduce.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

It'd be interesting to see the load from this timer in the context of an application less heavyweight than Gmail.

Could you try my DebugEventsPlugin from bug 441880 in 4 tabs at once?

(If I remember right, ages ago I proposed a patch to get rid of this timer.  I'll try to find it again.)

Comment 2

[Boris Zbarsky [:bzbarsky]]

If you can send me instructions on how one would go about trying that plug-in, I'd be happy to do so.

Comment 3

[Josh Aas]

We can't stop sending idle time events to Carbon-based plugins but I have some ideas for making it more efficient. Idle time has been done away with in the Cocoa NPAPI event model but we haven't shipped that yet.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Once we ship it, does Flash automatically end up with it?  If so, and if we're planning to ship it soon-ish (1.9.3?), I wouldn't worry about spending time optimizing the carbon code...

Comment 5

[Josh Aas]

Adobe will have to release a Cocoa events based port of Flash in order for Flash to stop getting idle time. The optimizations I'm thinking of will either be fairly simple or I'll know quickly that they aren't possible.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #2)

> Could you try my DebugEventsPlugin from bug 441880 in 4 tabs at
> once?

> If you can send me instructions on how one would go about trying
> that plug-in, I'd be happy to do so.

Just download the latest version's "distro" from bug 441880 and copy
its binary (DebugEventsPlugin.plugin) from the distro's build/Debug
directory to /Library/Internet Plug-Ins/.  Then open the distro's
test.html in four different tabs.

Comment 7

[Mike Shaver (:shaver emeritus)]

How often does Safari fire the idle timers to those plugins?  They seem to use a lot less ambient CPU in this case, would be good to see if they have a trick we can copy.

Comment 8

[Josh Aas]

(In reply to comment #7)
> How often does Safari fire the idle timers to those plugins?  They seem to use
> a lot less ambient CPU in this case, would be good to see if they have a trick
> we can copy.

I'm looking into it but we already copied that particular trick from them once. Maybe they adjusted timing to be even lower.

The things I alluded to in in comment #3 are timing adjustments and using only 1-2 timers to drive all instances.

Comment 9

[Boris Zbarsky [:bzbarsky]]

OK, with 4 copies of that test plugin test.html, I get about 5% cpu usage.  20% of that is under the plugin notify, and 30% under nsAppShell::OnProcessNextEvent; a bunch more under various event-processy and painty stuff.  If I close all the tabs cpu usage goes to 0. If I have one tab open showing about:blank, cpu usage is about 0.1%.  If I have one tab showing the test plugin, cpu usage is about 4%.  If I have two tabs, with the test plugin in the background one, cpu usage is about 1.5%.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

> 20% of that is under the plugin notify, and 30% under
> nsAppShell::OnProcessNextEvent;

Interesting that these percentages are roughly the same under a
"light" load (my DebugEventsPlugin) as under a "heavy" one (Gmail plus
whatever plugin(s) it loads).  Though neither what happens under the
timer nor what happens under nsAppShell::OnProcessNextEvent() is a
constant (they go up or down with the rest of the "load").

Comment 11

[Steven Michaud [:smichaud] (Retired)]

Actually, in case you're interested, here's a very heavyweight Flash "movie" to test with -- http://rogerdean.com.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> "light" load (my DebugEventsPlugin) as under a "heavy" one 

Your plug-in is visible, so it get about 60 timer calls a second in the foreground tab.  10 per second in the background tabs, so 90 total with 4 tabs.  The gmail plugins are all invisible so at 4 per tab that's 160 per seconds.  Not a huge difference in some ways.

> either what happens under the timer nor what happens under
> nsAppShell::OnProcessNextEvent() is a constant 

I think all that's going on with OnProcessNextEvent is that we do a bunch of (somewhat) expensive stuff for every single timer event we process or something....

Comment 13

[Josh Aas]

Note - I tweaked idle time in bug 525533. Now we only fire the timer 50 times per second for active instances and 4 times per second for hidden instances.

We should still try getting all instances to share 2 timers for a bigger improvement.

Comment 14

[Josh Aas]

Attachment: fix v1.0

This patch makes the number of idle event timer notifications per second O(1) instead of O(n), where n is the number of plugin instances. It has all Mac OS X Carbon plugin instances operating off of the same two timers (one for visible, one for hidden) instead of having a timer per instance.

If you have four visible plugins, for example, without this patch you'd have 200 (4*50) timer notifications per second. With this patch you'd only have 50.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Would it make more sense to move plugins to using nsRefreshDriver?  That fires at exactly the 20ms interval visible plug-ins want, and the invisible ones could fire once every 6 notifications (so at 120ms, compared to the current 125)...  And in background tabs we could disable the refresh driver altogether, and not send these notifications to plug-ins at all, perhaps.

Comment 16

[Boris Zbarsky [:bzbarsky]]

I guess the key is that we get a tradeoff between having more timers around if we have multiple presshells with plug-ins in them and being able to not fire idle events at all to plug-ins in background tabs, or throttle them back to a rate of our choosing, or whatever.

Comment 17

[Josh Aas]

Hooking into nsRefreshDriver sounds like future pain to me. It doesn't require a lot of code to do it the way my patch does it, it is more flexible, and it does exactly what we want with no compromises.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Does it do exactly what we want, though?  Do we want to be sending idle events at 20ms intervals to plug-ins in backgrounds tabs?  In minimized windows?

Comment 19

[Josh Aas]

We want to be sending idle events all the time, and we want to be in hidden mode (lower rate) whenever the plugin is not visible (including when it is in a background tab or a minimized window). The opt-in timer situation for Cocoa plugins is totally different in case there is any confusion about that.

I am not interested in changing anything but the number of timers in this bug, which should not affect plugin behavior in any significant way. If there are times when the rate is high and it should be low then we should file bugs about that separately.

Comment 20

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

bz, we need to send idle events to hidden plugins in case they're playing sound.

Comment 21

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

Why didn't you use nsTObserverArray here?

Comment 22

[Josh Aas]

Attachment: fix v1.1

I didn't know we had such an array type, perfect.

Comment 23

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

+    while (iter.HasMore())
+      iter.GetNext()->SendIdleEvent();

+    while (iter.HasMore())
+      iter.GetNext()->SendIdleEvent();

+  if (hiddenRemoved && mHiddenTimerTargets.IsEmpty())
+    mHiddenPluginTimer->Cancel();

+  if (visibleRemoved && mVisibleTimerTargets.IsEmpty())
+    mVisiblePluginTimer->Cancel();

{} around the body

Should the new code be #ifdef CARBON or something?

It might be worth defining an enum { PLUGIN_VISIBLE = 0, PLUGIN_HIDDEN = 1 } and passing that to nsPluginInstanceOwner::StartTimer, and using an array of two elements for the timers, so you can use a loop instead of duplicating code.

Comment 24

[Josh Aas]

Attachment: fix v1.2

This adds ifdefs and braces. I didn't do the enum thing, there are only a few simple duplicate lines and the alternative probably means more code and complexity.

Comment 25

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/878545a1236a

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Josh, should this be all/all?

Comment 27

[Josh Aas]

This applies to x86 and PPC Mac OS X, but not x86-64.