Closed Bug 556849 Opened 14 years ago Closed 14 years ago

[OOPP] Reduce unnecessary HasProperty calls for plugin scriptable objects

Categories

(Core Graveyard :: Plug-ins, defect)

Product:
Core Graveyard
Component:
Plug-ins
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(status1.9.2 .4-fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
status1.9.2 --- .4-fixed

People

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

Details

Attachments

(3 files, 1 obsolete file)

Patch, v1, with tests
34.34 KB, patch
jst
: review+
jaas
: review+
benjamin
: review+
Details | Diff | Splinter Review
Additional patch
736 bytes, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
Patch for 1.9.2
35.01 KB, patch
beltzner
: approval1.9.2.4+
Details | Diff | Splinter Review
Attached patch Patch, v1 (obsolete) — — Splinter Review 
nsJSNPRuntime calls HasProperty on plugin objects before calling GetProperty, SetProperty, and RemoveProperty. For plugin scriptable objects that exist in a child process this results in many unnecessary IPC calls, each causing a slowdown and providing an opportunity for another process to race with the parent.

Attached patch prevents those unnecessary calls while preserving the same behavior by performing the HasProperty calls in the plugin process. Also, we weren't handling java's funny property+method combination properly and I think that I've fixed that here as well.

Using the attached patch I recorded the number of IPC messages sent while loading the silverlight showcase (http://www.silverlight.net/showcase) and got the following results showing a significant reduction in IPC traffic:

patched.log:
  Sent:  107018 messages
  Total: 214627 messages (includes replies)

unpatched.log:
  Sent:  185948 messages
  Total: 372962 messages (includes replies)

This may fix bug 555500.

I'm not sure if josh or jst would be better to review the pluginhost changes, but I'd like bsmedberg to look at the OOPP stuff.
Attachment #436743 - Flags: review?(jst)
Attachment #436743 - Flags: review?(joshmoz)
Attachment #436743 - Flags: review?(benjamin)
Comment on attachment 436743 [details] [diff] [review]
Patch, v1

No tests. If it's important to get distinct property/method correct, we should have a test for that case.
Attachment #436743 - Flags: review?(benjamin) → review-
Now with tests.
Attachment #436743 - Attachment is obsolete: true
Attachment #437167 - Flags: review?(jst)
Attachment #437167 - Flags: review?(joshmoz)
Attachment #436743 - Flags: review?(jst)
Attachment #436743 - Flags: review?(joshmoz)
Attachment #437167 - Flags: review?(benjamin)
Comment on attachment 437167 [details] [diff] [review]
Patch, v1, with tests

+  for (int i = 0; i < ARRAY_LENGTH(sPluginPropertyValues); i++) {

...

+  for (int i = 0; i < int(ARRAY_LENGTH(sPluginPropertyIdentifiers)); i++) {

Why int(ARRAY_LENGTH(...)) in the second case and not the first?

There are a bunch of Windows line breaks in the patch.
Attachment #437167 - Flags: review?(joshmoz) → review+
Comment on attachment 437167 [details] [diff] [review]
Patch, v1, with tests

>diff --git a/modules/plugin/test/mochitest/test_propertyAndMethod.html b/modules/plugin/test/mochitest/test_propertyAndMethod.html

>+<head>

Are you using notepad? ;-) audit this for CRLF and BOM marks.

As we discussed on IRC, doing this using __proto__ is a little hacky. Could you maybe run this test using kTestSharedNPClass (.getObjectValue) instead of the plugin element proto class?
Attachment #437167 - Flags: review?(benjamin) → review+
Out of curiosity, what are the chances this could make the April 12 code freeze for 3.6.4 (or is that unlikely)?
Attachment #437167 - Flags: review?(jst) → review+
Yes, although I'm not sure about the risk/reward factor for this patch yet. Need to get it landed on m-c ASAP.
http://hg.mozilla.org/mozilla-central/rev/0ed675647700

Will file a followup to move the test for removeprop to the other plugin scriptable object.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment on attachment 437167 [details] [diff] [review]
Patch, v1, with tests

I'd like to get this for Lorentz. It's a big reduction in the number of places where we can race the other process in RPC calls. Baking now, has tests.
Attachment #437167 - Flags: approval1.9.2.4?
Hm, this bounced. Test is timing out on some linux and os x machines. I'm looking into it.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Did you fix the line breaks mentioned in comment 3 and 4?(In reply to comment #9)
> Hm, this bounced. Test is timing out on some linux and os x machines. I'm
> looking into it.

Did you fix the line breaks mentioned in comment 3 and 4?
Yes. Though I guess it's possible vstudio slipped something by me...
Attached patch Additional patch — — Splinter Review 
This has r=josh, this was an old pluginhost bug. Tests were failing on non-OOP boxes because our inprocess behavior for this has been broken for a while now.
Attachment #437717 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/f292d7bd2e93
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
Comment on attachment 437717 [details] [diff] [review]
Additional patch

This was apparently introduced in bug 474157.
Attached patch Patch for 1.9.2 — — Splinter Review 
Here's a rollup for 1.9.2/Lorentz. I'd like to get this in as it reduces the number of places where the plugin process can race with the browser and it also reduces a ton of IPC messages. It has tests.
Attachment #437719 - Flags: approval1.9.2.4?
Comment on attachment 437719 [details] [diff] [review]
Patch for 1.9.2

a=beltzner for 1.9.2.4 - this is a big patch, but with tests.

Ben: can you help QA understand how they can test the effects of this patch? Just paying attention to perf? Multiple plugin instances of the same / different type?
Attachment #437719 - Flags: approval1.9.2.4? → approval1.9.2.4+
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4569f7f41331

As for QA verification... See bug 555500 comment 8, perhaps? He was just timing silverlight load time I believe. Not much more to do to verify this I don't think.
status1.9.2: --- → .4-fixed
Did this cause bug 563891?
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: