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

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Core
Plug-ins
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: bz, Assigned: Josh Aas)

Tracking

(Blocks: 1 bug, {perf})

Trunk
mozilla1.9.3a1
All
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
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.)
If you can send me instructions on how one would go about trying that plug-in, I'd be happy to do so.
(Assignee)

Comment 3

9 years ago
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.
Assignee: nobody → joshmoz
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...
(Assignee)

Comment 5

9 years ago
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.
(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.
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.
(Assignee)

Comment 8

9 years ago
(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.
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%.
> 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").
Actually, in case you're interested, here's a very heavyweight Flash "movie" to test with -- http://rogerdean.com.
> "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....
(Assignee)

Comment 13

9 years ago
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.
(Assignee)

Comment 14

9 years ago
Created attachment 415073 [details] [diff] [review]
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.
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.
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.
(Assignee)

Comment 17

9 years ago
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.
(Assignee)

Updated

9 years ago
Attachment #415073 - Flags: review?(roc)
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?
(Assignee)

Comment 19

9 years ago
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.
bz, we need to send idle events to hidden plugins in case they're playing sound.
Why didn't you use nsTObserverArray here?
(Assignee)

Comment 22

9 years ago
Created attachment 415265 [details] [diff] [review]
fix v1.1

I didn't know we had such an array type, perfect.
Attachment #415073 - Attachment is obsolete: true
Attachment #415265 - Flags: review?(roc)
Attachment #415073 - Flags: review?(roc)
+    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.
(Assignee)

Comment 24

9 years ago
Created attachment 415357 [details] [diff] [review]
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.
Attachment #415265 - Attachment is obsolete: true
Attachment #415357 - Flags: review?(roc)
Attachment #415265 - Flags: review?(roc)
(Assignee)

Comment 25

9 years ago
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/878545a1236a
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Josh, should this be all/all?
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.3a1
(Assignee)

Comment 27

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

Updated

8 years ago
Duplicate of this bug: 217360
You need to log in before you can comment on or make changes to this bug.