Closed Bug 535017 Opened 12 years ago Closed 12 years ago
IPC plugins should cache well-known scriptable objects to avoid unnecessary actor creation/destruction
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?
Probably "very good". We can just cache the actors the first time they are requested, right?
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).
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.
(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.)
I agree with cjones, this might block beta1 but doesn't block on-by-default.
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.
Assignee: nobody → bent.mozilla
Status: NEW → ASSIGNED
Attachment #427883 - Flags: review?(benjamin)
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-
(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.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.