Closed Bug 547359 Opened 12 years ago Closed 11 years ago

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

Categories

(Core :: IPC, defect)

x86
Windows XP
defect
Not set
major

Tracking

()

VERIFIED FIXED
Tracking Status
status1.9.2 --- .4-fixed

People

(Reporter: fehe, Assigned: bent.mozilla)

References

()

Details

(Keywords: perf, Whiteboard: [fixed-lorentz])

Attachments

(3 files, 2 obsolete files)

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.
Keywords: perf
Version: unspecified → Trunk
Component: General → IPC
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
You would have to enable the script to continue, because it will load eventually.  It's just being bogged down.
QA Contact: general → ipc
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).
Assignee: nobody → bent.mozilla
Summary: [OOPP] Silverlight plugin takes 6X longer to load with IPC plugins than without → [OOPP] Silverlight plugin takes 6X longer to load with IPC plugins than without - make NPIdentifier handler smarter/faster
Attached patch Patch, v1 (obsolete) — Splinter Review
This caches identifiers so that we only have to make an RPC call once per identifier.
Attachment #433071 - Flags: review?(benjamin)
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.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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.
Attachment #433071 - Flags: review?(benjamin) → review-
Attached patch Identifiers as Actors, v1 (obsolete) — Splinter Review
We had to go whole hog and make them actors. What do you think?
Attachment #433071 - Attachment is obsolete: true
Attachment #433592 - Flags: review?(benjamin)
Now resolves races, too.
Attachment #433592 - Attachment is obsolete: true
Attachment #433633 - Flags: review?(benjamin)
Attachment #433592 - Flags: review?(benjamin)
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.
Attachment #433633 - Flags: review?(benjamin) → review+
(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.
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
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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.
Doesn't work with nightly either... back to the drawing board :(
Re-installed latest hourly, and now it works - go figure... 
I think we need a better test site for Silverlight...
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
For comparison, memory consumption without IPC is only 98 MB
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...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
We'll need a new bug for the mem usage stuff. I can reproduce it and it's something in RPC code...
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
(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?
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
Status: RESOLVED → VERIFIED
Turns out this patch falls far short of addressing the performance hit; therefore, I've filed bug 555500.
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
Depends on: 653083
You need to log in before you can comment on or make changes to this bug.