Closed
Bug 547359
Opened 14 years ago
Closed 14 years ago
[OOPP] Silverlight plugin takes 6X longer to load with IPC plugins than without - make NPIdentifier handler smarter/faster
Categories
(Core :: IPC, defect)
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)
233.77 KB,
application/zip
|
Details | |
80.57 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
8.12 KB,
patch
|
Details | Diff | Splinter Review |
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.
Blocks: OOPP, LorentzBeta1
Comment 2•14 years ago
|
||
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.
Updated•14 years ago
|
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).
Updated•14 years ago
|
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
Assignee | ||
Comment 5•14 years ago
|
||
This caches identifiers so that we only have to make an RPC call once per identifier.
Attachment #433071 -
Flags: review?(benjamin)
Assignee | ||
Comment 6•14 years ago
|
||
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.
Assignee | ||
Updated•14 years ago
|
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment 7•14 years ago
|
||
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-
Assignee | ||
Comment 8•14 years ago
|
||
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)
Assignee | ||
Comment 9•14 years ago
|
||
Now resolves races, too.
Attachment #433592 -
Attachment is obsolete: true
Attachment #433633 -
Flags: review?(benjamin)
Attachment #433592 -
Flags: review?(benjamin)
Comment 10•14 years ago
|
||
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+
Assignee | ||
Comment 11•14 years ago
|
||
(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.
Assignee | ||
Comment 12•14 years ago
|
||
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: 14 years ago
Resolution: --- → FIXED
Comment 13•14 years ago
|
||
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.
Assignee | ||
Comment 14•14 years ago
|
||
Hm... WFM.
Comment 15•14 years ago
|
||
Doesn't work with nightly either... back to the drawing board :(
Comment 16•14 years ago
|
||
Re-installed latest hourly, and now it works - go figure... I think we need a better test site for Silverlight...
Reporter | ||
Comment 17•14 years ago
|
||
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
Reporter | ||
Comment 18•14 years ago
|
||
For comparison, memory consumption without IPC is only 98 MB
Reporter | ||
Comment 19•14 years ago
|
||
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 → ---
Reporter | ||
Comment 20•14 years ago
|
||
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.
Assignee | ||
Comment 21•14 years ago
|
||
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: 14 years ago → 14 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 22•14 years ago
|
||
(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?
Assignee | ||
Comment 23•14 years ago
|
||
bug 554466.
Reporter | ||
Comment 24•14 years ago
|
||
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
Comment 25•14 years ago
|
||
http://hg.mozilla.org/projects/firefox-lorentz/rev/157f0c47b447
Whiteboard: [fixed-lorentz]
Reporter | ||
Comment 26•14 years ago
|
||
Turns out this patch falls far short of addressing the performance hit; therefore, I've filed bug 555500.
Comment 27•14 years ago
|
||
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•14 years ago
|
||
Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430
status1.9.2:
--- → .4-fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•