Closed
Bug 622301
Opened 14 years ago
Closed 13 years ago
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
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla14
Tracking | Status | |
---|---|---|
firefox13 | --- | wontfix |
People
(Reporter: bzbarsky, Assigned: bholley)
References
Details
Attachments
(2 files, 2 obsolete files)
2.95 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
19.52 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
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•14 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?
Comment 2•14 years ago
|
||
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•13 years ago
|
||
Bobby is throwing himself on this machine gun nest.
Assignee: nobody → bobbyholley+bmo
Assignee | ||
Comment 4•13 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 | ||
Comment 5•13 years ago
|
||
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•13 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•13 years ago
|
||
Attachment #585505 -
Flags: review?(mrbkap)
Comment 7•13 years ago
|
||
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•13 years ago
|
Attachment #585505 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 8•13 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•13 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•13 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));
Assignee | ||
Comment 11•13 years ago
|
||
Landed to mozilla-inbound:
* http://hg.mozilla.org/integration/mozilla-inbound/rev/668a56be0eef
* http://hg.mozilla.org/integration/mozilla-inbound/rev/b1612e3ba9b9
Target Milestone: --- → mozilla12
Comment 12•13 years ago
|
||
Could this be the cause of https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/-7aQ03BTo4k / was this expected? :-)
Comment 13•13 years ago
|
||
Manual inspection of both:
http://graphs-new.mozilla.org/graph.html#tests=[[72,131,15]]&sel=1325815778000,1325988578000&displayrange=7&datatype=running
and
http://graphs-new.mozilla.org/graph.html#tests=[[73,131,15]]&sel=1325815778000,1325988578000&displayrange=7&datatype=running
...confirms the regression range as:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9a1a4157bf49&tochange=a761ff40306b
Comment 14•13 years ago
|
||
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•13 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.
Comment 16•13 years ago
|
||
(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.
Reporter | ||
Comment 17•13 years ago
|
||
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•13 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•13 years ago
|
||
Assignee | ||
Comment 20•13 years ago
|
||
Assignee | ||
Updated•13 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•13 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•13 years ago
|
Attachment #587955 -
Flags: review?(mrbkap)
Assignee | ||
Updated•13 years ago
|
Attachment #585504 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #585505 -
Attachment is obsolete: true
Updated•13 years ago
|
Attachment #587955 -
Flags: review?(mrbkap) → review+
Updated•13 years ago
|
Attachment #587956 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 21•13 years ago
|
||
Gave this one last push to try to double-check: https://tbpl.mozilla.org/?tree=Try&rev=6bb5a094065b
Comment 22•13 years ago
|
||
Caught a bad parent, so I pushed again:
https://tbpl.mozilla.org/?tree=Try&rev=c5294fcc3baf
Assignee | ||
Comment 23•13 years ago
|
||
Pushed this to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/46ae4bf4aaf4
http://hg.mozilla.org/integration/mozilla-inbound/rev/3ab1dcfb2218
Target Milestone: --- → mozilla12
Comment 24•13 years ago
|
||
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•13 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•13 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
Comment 27•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9dde014a4b3f
https://hg.mozilla.org/mozilla-central/rev/b89b725e10fb
Green PGO \o/
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 28•13 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
Assignee | ||
Comment 29•13 years ago
|
||
Backed out on aurora until we can get a patch together for bug 726949:
http://hg.mozilla.org/releases/mozilla-aurora/rev/1f489405617a
http://hg.mozilla.org/releases/mozilla-aurora/rev/98c38622905f
Reporter | ||
Updated•13 years ago
|
status-firefox13:
--- → wontfix
Target Milestone: mozilla13 → mozilla14
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•