547359 - [OOPP] Silverlight plugin takes 6X longer to load with IPC plugins than without - make NPIdentifier handler smarter/faster

Status: VERIFIED FIXED

(Core :: IPC, defect)

Description

[IU]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a2pre) Gecko/20100219 Minefield/3.7a2pre (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a2pre) Gecko/20100219 Minefield/3.7a2pre

With IPC plugins enabled, the Silverlight Showcase takes 6 times longer to load than with IPC plugins disabled (90 seconds vs. 15 seconds) on my P3 system.

I tested with Silverlight 4 Beta; however, I doubt the effect with Silverlight 3 should be much different.


Reproducible: Always

Steps to Reproduce:
1. With IPC plugins enabled, visit the linked URL and time how long it takes for the Silverlight Showcase to become usable
2. Disable IPC plugins, restart the browser, and test again
3. Notice the significant performance difference.

Comment 1

[IU]

Attachment: IPC plugins log (loading http://www.silverlight.net/showcase/)

Comment 2

[[not reading bugmail]]

I see this message come up when it take a while: ask to Stop Script or continue:
Script: http://blogs.silverlight.net/showcase/resources/js/main.js?fd=-8589467390013877770:2539

Comment 3

[IU]

You would have to enable the script to continue, because it will load eventually.  It's just being bogged down.

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

The Silverlight Showcase takes forever to start up with IPC enabled for me as well. While it's loading, Firefox's UI locks up (can't change tabs, clicks within the document aren't registered).

I don't see the Stop Script dialog, though.

I'm on the latest Minefield nightly (20100227).

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

This caches identifiers so that we only have to make an RPC call once per identifier.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I also noticed that silverlight does a bunch of HasProperty/GetProperty calls, so I wonder if we couldn't get a bit more speed out of actually calling GetProperty from HasProperty and then caching the result.

Comment 7

[Benjamin Smedberg]

Comment on attachment 433071 [details] [diff] [review]
Patch, v1

Discussion from IRC, we at least want separate maps (int->identifier and string->identifier) and a local object so that we don't have to enumerate a hash to do the lookup.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Identifiers as Actors, v1

We had to go whole hog and make them actors. What do you think?

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Identifiers as Actors, v1.1

Now resolves races, too.

Comment 10

[Benjamin Smedberg]

Comment on attachment 433633 [details] [diff] [review]
Identifiers as Actors, v1.1

>diff --git a/dom/plugins/PPluginIdentifier.ipdl b/dom/plugins/PPluginIdentifier.ipdl

>+async protocol PPluginIdentifier
>+{
>+  manager PPluginModule;

Wants doc-comment "this protocol represents an NPIdentifier".

Is this protocol required by IPDL to have a __delete__ message, even thought we never call it or intend to call it?

>diff --git a/dom/plugins/PPluginModule.ipdl b/dom/plugins/PPluginModule.ipdl

>+both:
>+  // Sending a void string to this constructor creates an int identifier
>+  // whereas sending a non-void string will create a string identifier.
>+  async PPluginIdentifier(nsCString aString,
>+                          int32_t aInt);

Use doc-comment styling. Also, please explain here how it deals with racing constructors for the same logical entity.

>diff --git a/dom/plugins/PluginIdentifierChild.h b/dom/plugins/PluginIdentifierChild.h

>+class PluginIdentifierChild : public PPluginIdentifierChild

It would be smaller to have PluginIdentifierChildString and PluginIdentifierChildInt. Do you strongly prefer having one class for both types?

>+  NPIdentifier ToNPIdentifier()
>+  {
>+    return mCanonicalIdentifier ? mCanonicalIdentifier : this;
>+  }

You could also just initialize mCanonicalIdentifier = this and always return mCanonicalIdentifier.

>diff --git a/dom/plugins/PluginModuleChild.cpp b/dom/plugins/PluginModuleChild.cpp


>+NPUTF8* NP_CALLBACK
>+PluginModuleChild::NPN_UTF8FromIdentifier(NPIdentifier aIdentifier)
>+{
>+    PLUGIN_LOG_DEBUG_FUNCTION;
>+    AssertPluginThread();
>+
>+    PluginIdentifierChild* ident =
>+        static_cast<PluginIdentifierChild*>(aIdentifier);
>+    return ident->ToString();

The mozilla plugin host checks the identifier type:
1507   if (!JSVAL_IS_STRING(v)) {
1508     return nsnull;
1509   }

I think we should probably do the same thing here.

>+int32_t NP_CALLBACK
>+PluginModuleChild::NPN_IntFromIdentifier(NPIdentifier aIdentifier)


1526   if (!JSVAL_IS_INT(v)) {
1527     return PR_INT32_MIN;
1528   }

r=me with changes mentioned. PluginIdentifierChildString is optional but seems like it would be pretty simple.

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Address review comments

(In reply to comment #10)
> Is this protocol required by IPDL to have a __delete__ message, even thought we
> never call it or intend to call it?

Yes.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I went a little further and got rid of the mType member and used a bit on mCanonicalIdentifier as the type flag instead.

http://hg.mozilla.org/mozilla-central/rev/a7c62300bbbb

Comment 13

[Jim Jeffery not reading bug-mail 1/2/11]

The test url given does not load the whole page, only the background.  Prior to the patch I did get slow-script warnings, but still page would not load.

Tested using latest hourly with patch.

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Hm... WFM.

Comment 15

[Jim Jeffery not reading bug-mail 1/2/11]

Doesn't work with nightly either... back to the drawing board :(

Comment 16

[Jim Jeffery not reading bug-mail 1/2/11]

Re-installed latest hourly, and now it works - go figure... 
I think we need a better test site for Silverlight...

Comment 17

[IU]

Bug is not fixed for me using latest hourly.

While the tiles appearing is now only 3X slower, they are still not clickable (no orange hover effect) until 6X later.

Add to that, memory utilization now goes races up to over 300 MB

Comment 18

[IU]

For comparison, memory consumption without IPC is only 98 MB

Comment 19

[IU]

This patch causes a huge memory leak.  Tested again and this time memory kept increasing over 800 MB.  The only reason memory consumption didn't keep increasing beyond that is that I killed the browser.

I verified that there are no such problems before this patch landed.

Now trying to determine steps to reproduce the runaway memory consumption issue...

Comment 20

[IU]

I'm having a hard time with the STR, because it's hard to time my clicks properly.  It appears it may be triggered by a race condition.

The gist of it is that I click to load the Sliverlight Showcase page, but before the tiles are completely populated, I take focus off the browser window.  Once the tiles have been fully populated, I move my mouse pointer over the tiles.  At this point, CPU utilization goes way up and memory utilization increases out of control.  Killing mozilla-runtime.exe has no effect and the firefox.exe process continues to consume memory indefinitely -- until the process is killed.

Even if I could come up with an STR, people with fast systems would have to run it on a virtual box, in order to slow things down enough to allow timing the clicks.

I suppose you'll have to manually inspect the path to determine the likely source of the race condition, and thus the memory leak.

Comment 21

[Ben Turner (not reading bugmail, use the needinfo flag!)]

We'll need a new bug for the mem usage stuff. I can reproduce it and it's something in RPC code...

Comment 22

[IU]

(In reply to comment #21)
> We'll need a new bug for the mem usage stuff. I can reproduce it and it's
> something in RPC code...

Great.  I assume you've got that covered and I don't need to file anything?

Comment 23

[Ben Turner (not reading bugmail, use the needinfo flag!)]

bug 554466.

Comment 24

[IU]

With bug 554466 fixed, slowdown is now down to only about 2X, and there's no more runaway memory consumption.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a4pre) Gecko/20100324 Minefield/3.7a4pre ID:20100324040325
http://hg.mozilla.org/mozilla-central/rev/e9312d05488f

Comment 25

[Benjamin Smedberg]

http://hg.mozilla.org/projects/firefox-lorentz/rev/157f0c47b447

Comment 26

[IU]

Turns out this patch falls far short of addressing the performance hit; therefore, I've filed bug 555500.

Comment 27

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

Blanket approval for Lorentz merge to mozilla-1.9.2
a=beltzner for 1.9.2.4 - please make sure to mark status1.9.2:.4-fixed

Comment 28

[Benjamin Smedberg]

Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430