Closed
Bug 535017
Opened 14 years ago
Closed 14 years ago
IPC plugins should cache well-known scriptable objects to avoid unnecessary actor creation/destruction
Categories
(Core Graveyard :: Plug-ins, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: bent.mozilla, Assigned: bent.mozilla)
References
Details
Attachments
(1 file, 1 obsolete file)
8.51 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
We should hold on to the window, plugin element, and plugin scriptable object wrappers to keep from creating and destroying actors all the live long day.
Any idea what the perf impact of this would be?
Comment 2•14 years ago
|
||
Probably "very good". We can just cache the actors the first time they are requested, right?
Assignee | ||
Comment 3•14 years ago
|
||
Yeah, flash gets and then releases the window object about a billion times just starting up. I just want to make sure the actor stuff is all bullet-proofed before we do this as we could hide bugs.
Should this really block enabling OOPP by default? We don't have a guess at the performance impact of this change, or even a metric by which to measure it.
More quantitatively: An IPC round trip takes about 200us. That means that flash would need to get the window NPObject about 500 times during startup for there to be a user-visible perf hit from the IPC overhead (assuming that IPC time >> parent-side processing time).
Assignee | ||
Comment 6•14 years ago
|
||
A few things: 1. We destroy the NPObject immediately when the actor is done with it. In the in-process case JS holds a reference to most objects as well so we don't destroy until a GC. 2. Each create or destroy call is actually a series of 4 IPC calls: (Send)NPN_GetObject, (Send)NPN_CreateObject, (Return)NPN_CreateObject, (Return)NPN_GetObject. 3. We also have to do all the memory stuff and hash table modifications in addition to the IPC calls and JS goop. So yeah, I think we should do this before we turn it on by default. It should be a simple enough patch and we know it's a source of near-constant churn.
Assignee | ||
Comment 7•14 years ago
|
||
(In reply to comment #6) > 1. We destroy the NPObject immediately when the actor is done with it Actually, that's backwards. We destroy the actor immediately when the NPObject's refcount goes to 0.
(In reply to comment #6) > So yeah, I think we should do this before we turn it on by default. It should > be a simple enough patch and we know it's a source of near-constant churn. I disagree. The other items blocking OOPP by default are crashes and crash reporting. Would you really block enabling OOPP by default on an optimization that may or may not be user visible? (To be clear, I'm not arguing that we shouldn't do this optimization, I'm arguing that it shouldn't block enabling OOPP by default.)
Comment 9•14 years ago
|
||
I agree with cjones, this might block beta1 but doesn't block on-by-default.
Updated•14 years ago
|
Blocks: LorentzAlpha
Assignee | ||
Comment 10•14 years ago
|
||
Caches the window object and plugin element object. Since the plugin element object uses the plugin scriptable object as its prototype I think this is sufficient to prevent most IPC churn.
Comment 11•14 years ago
|
||
Comment on attachment 427883 [details] [diff] [review] Patch, v1 + NPObject* object = actor->GetObject(false); Shouldn't that be `true`? Also, I think we should clear the cached actors after NPP_Destroy before we call invalidate/dealloc on them.
Attachment #427883 -
Flags: review?(benjamin) → review-
Assignee | ||
Comment 12•14 years ago
|
||
(In reply to comment #11) > + NPObject* object = actor->GetObject(false); > > Shouldn't that be `true`? In this case it doesn't matter because the object can't be dropped as long as we hold the extra ref for the cache. I think leaving it false here is better.
Attachment #427883 -
Attachment is obsolete: true
Attachment #427971 -
Flags: review?(benjamin)
Updated•14 years ago
|
Attachment #427971 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 13•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/7fa519501fec
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•