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

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Any idea what the perf impact of this would be?

Comment 2

[Benjamin Smedberg]

Probably "very good". We can just cache the actors the first time they are requested, right?

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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).

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(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.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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

[Benjamin Smedberg]

I agree with cjones, this might block beta1 but doesn't block on-by-default.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: 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.

Comment 11

[Benjamin Smedberg]

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.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: 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.

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

http://hg.mozilla.org/mozilla-central/rev/7fa519501fec