Drop support for using DOM objects as prototypes of random objects and having DOM operations on the random object work on the DOM object instead

RESOLVED FIXED in mozilla14

Status

()

RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: bzbarsky, Assigned: bholley)

Tracking

Trunk
mozilla14
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox13 wontfix)

Details

Attachments

(2 attachments, 2 obsolete attachments)

It's not the way prototypes are supposed to work in ES, and overcomplicates code significantly.  A good first step would be to stop looking up the proto chain for the XPCWrappedNative or slimwrapper in quickstubs; this should make the unwrapping code much simpler.
(Reporter)

Comment 1

8 years ago
One question here.  Right now, quickstub unwrapping claims that it "sees through XOWs, XPCNativeWrappers, and SafeJSObjectWrappers".

Does it need to continue to do this?
Yes it does. Hopefully that's not too expensive though.

(For the record, this is implemented by the IsSecurityWrapper and then Unwrap code in xpcquickstubs.cpp:getWrapper.)
(Reporter)

Comment 3

7 years ago
Bobby is throwing himself on this machine gun nest.
Assignee: nobody → bobbyholley+bmo
(Assignee)

Comment 4

7 years ago
I've got some patches for this, though I think even more simplification can be done.

github branch: https://github.com/bholley/mozilla-central/commits/nonativeprotos
try push: https://tbpl.mozilla.org/?tree=Try&rev=d6326a7296ed
(Assignee)

Updated

7 years ago
Depends on: 713747
(Assignee)

Comment 5

7 years ago
Created attachment 585504 [details] [diff] [review]
Part 1 - Don't use XPCWrappedNative::GetWrappedNativeOfJSObject in quickstub unwrapping. v2

So I've been trying to figure out a way to make this work for all of XPConnect, rather than just QuickStubs. But there's some trickiness with Xray XOWs, and after talking with Blake, it sounds like it makes the most sense to do that as a following bug.

In the mean time, here's the quickstubs version. Flagging mrbkap for review.
Attachment #585504 - Flags: review?(mrbkap)
(Assignee)

Updated

7 years ago
Attachment #585504 - Attachment description: Bug 622301 - Don't use XPCWrappedNative::GetWrappedNativeOfJSObject in quickstub unwrapping. v2 → Part 1 - Don't use XPCWrappedNative::GetWrappedNativeOfJSObject in quickstub unwrapping. v2
(Assignee)

Comment 6

7 years ago
Created attachment 585505 [details] [diff] [review]
Part 2 - Remove now-obsolete 'callee' parameter. v1
Attachment #585505 - Flags: review?(mrbkap)
Comment on attachment 585504 [details] [diff] [review]
Part 1 - Don't use XPCWrappedNative::GetWrappedNativeOfJSObject in quickstub unwrapping. v2

Review of attachment 585504 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCQuickStubs.cpp
@@ +789,5 @@
> +    // get something tangible.
> +    while (js::IsWrapper(obj)) {
> +        obj = XPCWrapper::Unwrap(cx, obj);
> +        if (!obj)
> +            return NS_ERROR_XPC_SECURITY_MANAGER_VETO;

Instead of a while loop, this could be two nested if statements (and then an assertion). We can have at most 1 security wrapper followed by (at most) an outer window. After that, we have to find a wrapped native.

Note that in the case of a CrossOrigin or NoWaiver wrapper, the "double wrapping" is taken care of in XPCWrapper::Unwrap.
Attachment #585504 - Flags: review?(mrbkap) → review+

Updated

7 years ago
Attachment #585505 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 8

7 years ago
(In reply to Blake Kaplan (:mrbkap) from comment #7)
> Note that in the case of a CrossOrigin or NoWaiver wrapper, the "double
> wrapping" is taken care of in XPCWrapper::Unwrap.

Is it? It looks to me like we only do one layer of unwrapping there...
(Assignee)

Comment 9

7 years ago
(In reply to Bobby Holley (:bholley) from comment #8)

> Is it? It looks to me like we only do one layer of unwrapping there...

Oh, I see. js::UnwrapObject actually recursively unwraps everything except for outer objects (in which case it only does one layer of unwrapping). I hadn't realized that.
(Assignee)

Comment 10

7 years ago
Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=4077fdcc109e

Blake, in case you're interested, the new version of the code we were discussing is as follows:

+    // We can have at most three layers in need of unwrapping here:
+    // * A (possible) security wrapper
+    // * A (possible) Xray waiver
+    // * A (possible) outer window
+    //
+    // The underlying call to js::Unwrap recursively unwraps, but stops if it
+    // hits an outer object. Thus, we need to make at most two unwrapping
+    // calls: one to handle security wrappers and waivers, and one to handle
+    // outer objects. 
+    if (js::IsWrapper(obj))
+        obj = XPCWrapper::Unwrap(cx, obj);
+    if (obj && js::IsWrapper(obj)) {
+        MOZ_ASSERT(js::Wrapper::wrapperHandler(obj)->isOuterWindow());
+        obj = XPCWrapper::Unwrap(cx, obj);
     }

-    *cur = obj;
+    // The safe unwrap might have failed for SCRIPT_ACCESS_ONLY objects. If it
+    // didn't fail though, we should be done with wrappers.
+    if (!obj)
+        return NS_ERROR_XPC_SECURITY_MANAGER_VETO;
+    MOZ_ASSERT(!js::IsWrapper(obj));
Backed out for now so that the Dromaeo results over the coming hours can confirm either way :-)

https://hg.mozilla.org/integration/mozilla-inbound/rev/48da43d7924a
Target Milestone: mozilla12 → ---
(Assignee)

Comment 15

7 years ago
I've been investigating this issue this week.

It looks like the actually dromaeo numbers are scattered, and there are some wins in some places. However we lose pretty hard on dom-traverse, where we go from ~460 to ~370.

With profiling, I noticed that the calls to js::IsWrapper are really hot, and that I actually introduced several more of them. The old hot path actually had no calls to js::IsWrapper, and my patch added 2 unconditional calls (the second one was silly, and should have been conditional on the first). So I filed bug 717113, and started to do optimization and inlining there.

But the numbers still weren't adding up. I looked into it again, and realized that it was actually my _second_ patch that was causing the major perf hit. The second patch only does one non-trivial thing, which is here: hg.mozilla.org/integration/mozilla-inbound/rev/b1612e3ba9b9#l4.68

Basically, the castNativeFromWrapper code just copy-pastes the central part of getWrapper without any of the checks, and bails out to getWrapper if that fails. I'm pretty flabbergasted that this was so hot, but it was.

I'm guessing it might have a lot to do with the function call overhead, so I'm going to try inlining getWrapper and see if that helps.
(In reply to Bobby Holley (:bholley) from comment #15)
> Basically, the castNativeFromWrapper code just copy-pastes the central part
> of getWrapper without any of the checks, and bails out to getWrapper if that
> fails. I'm pretty flabbergasted that this was so hot, but it was.

I don't think I'd write the code like that if it didn't matter for performance.
Inlining getWrapper is worth trying.  If we do that, we should also reorder it so that it checks for the common cases first, imo.
(Assignee)

Comment 18

7 years ago
So by cleaning up a few things in the first patch and leaving the manual inlining in the second, I've got this to be more or less dromaeo-neutral: http://dromaeo.com/?id=160322,160547

So I'm going to try to land this as-is, and handle everything else in followup bugs. Patches coming right up.
(Assignee)

Comment 19

7 years ago
Created attachment 587955 [details] [diff] [review]
Part 1 - Don't use XPCWrappedNative::GetWrappedNativeOfJSObject in quickstub unwrapping. v5
(Assignee)

Comment 20

7 years ago
Created attachment 587956 [details] [diff] [review]
Part 2 - Remove now-obsolete 'callee' parameter. v2
(Assignee)

Updated

7 years ago
Attachment #587955 - Attachment description: Bug 622301 - Don't use XPCWrappedNative::GetWrappedNativeOfJSObject in quickstub unwrapping. v5 → Part 1 - Don't use XPCWrappedNative::GetWrappedNativeOfJSObject in quickstub unwrapping. v5
(Assignee)

Updated

7 years ago
Attachment #587956 - Attachment description: Bug 622301 - Remove now-obsolete 'callee' parameter. v2 → Part 2 - Remove now-obsolete 'callee' parameter. v2
Attachment #587956 - Flags: review?(mrbkap)
(Assignee)

Updated

7 years ago
Attachment #587955 - Flags: review?(mrbkap)
(Assignee)

Updated

7 years ago
Attachment #585504 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #585505 - Attachment is obsolete: true

Updated

7 years ago
Attachment #587955 - Flags: review?(mrbkap) → review+

Updated

7 years ago
Attachment #587956 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 21

7 years ago
Gave this one last push to try to double-check: https://tbpl.mozilla.org/?tree=Try&rev=6bb5a094065b
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b641c65643b9 for utterly opaque crashes in the Windows PGO profiling run like https://tbpl.mozilla.org/php/getParsedLog.php?id=8557108&tree=Mozilla-Inbound - I don't envy you the debugging of that, but I'm looking forward to how you'll probably have to leave automation.py/profileserver.py in better shape for the next person who hits a crash there ;)
(Assignee)

Comment 25

7 years ago
khuey fixed the PGO bug, and I believe the fix has landed on trunk.

Rebased the patches and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=082cbe8903da
(Assignee)

Comment 26

7 years ago
Pushed to m-i:

http://hg.mozilla.org/integration/mozilla-inbound/rev/9dde014a4b3f
http://hg.mozilla.org/integration/mozilla-inbound/rev/b89b725e10fb

In theory the windows PGO build should be green this time. Let's see.
Target Milestone: mozilla12 → mozilla13
https://hg.mozilla.org/mozilla-central/rev/9dde014a4b3f
https://hg.mozilla.org/mozilla-central/rev/b89b725e10fb

Green PGO \o/
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 28

7 years ago
bz pointed out that this was a very modest dromaeo win:

Improvement! Dromaeo (CSS) increase 2.52% on Linux Mozilla-Inbound-Non-PGO
http://mzl.la/xYKHEp

Updated

7 years ago
Depends on: 726949
(Reporter)

Updated

7 years ago
status-firefox13: --- → wontfix
Target Milestone: mozilla13 → mozilla14

Updated

6 years ago
Depends on: 785822
Depends on: 788966

Updated

6 years ago
Depends on: 790303
You need to log in before you can comment on or make changes to this bug.