IPC plugins should cache well-known scriptable objects to avoid unnecessary actor creation/destruction

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

Tracking

Trunk
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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

9 years ago
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.)

Comment 9

9 years ago
I agree with cjones, this might block beta1 but doesn't block on-by-default.
Blocks: 539055
No longer blocks: 531142
Blocks: 545893
Created attachment 427883 [details] [diff] [review]
Patch, v1

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-
Created attachment 427971 [details] [diff] [review]
Patch, v1.1

(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

9 years ago
Attachment #427971 - Flags: review?(benjamin) → review+
http://hg.mozilla.org/mozilla-central/rev/7fa519501fec
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.