Closed
Bug 533639
Opened 15 years ago
Closed 13 years ago
Consider better jit codegen for DOM bindings (dromaeo)
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: bzbarsky, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(6 files, 1 obsolete file)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/html
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
We (me, Peter, Blake) were looking at some dromaeo profiles today, and we seem to be spending enough time on wrapping/unwrapping even now that there's no way we can get to chrome's performance on those tests without some radical change.
So a halfway-radical change proposal: bake the QI result into the trace. More precisely, since we guard on shape before making the quickstub call (we do, right?) and since shape includes JSClass (right?) we are in fact guaranteed that if we had a certain DOM class before we still have the same one. At least for nodes. Not quite as sure about nodelists and such, given our classinfo-name stuff, but I think it's true there too.
Given that, the idea is to bake into trace the offset from nsISupports* to the interface the quickstub actually wants. Then we need "only" check whether we have a security wrapper, and if not get our nsISupports* and add the offset. This works as long as the interface can be reached via the element's offset table.
Andreas' suggestion was that the trace allocate a 4-word scratch area when it traces a quickstub call, and pass the pointer to the call. We can then stash the offset there the first time we call the quickstub, if the interface is reached via the offset table, and get it from there after that.
Thoughts? Is there some way we can make this even thinner somehow?
Reporter | ||
Updated•15 years ago
|
Assignee: nobody → general
Component: DOM → JavaScript Engine
QA Contact: general → general
Comment 1•15 years ago
|
||
Reporter | ||
Comment 2•15 years ago
|
||
Comment 3•15 years ago
|
||
Can you post the shark profile as ascii?? just so we know what callstack we talk about.
Reporter | ||
Comment 4•15 years ago
|
||
Reporter | ||
Comment 5•15 years ago
|
||
Reporter | ||
Comment 6•15 years ago
|
||
Reporter | ||
Comment 7•15 years ago
|
||
This speeds up the second microbenchmark some; in particular unwrapping drops to 28%. That's a 40% speedup on the testcase overall; a 70% speedup of the unwrapping itself.
I also tried putting the pic use before the security wrapper thing, and that moved unwrapping down to 24%. So a 44% overall speedup; a 77% speedup of the unwrapping.
If we're ok doing the pic check before the unwrap, I bet we can win even bigger by simply hoisting it out of the unwrap function altogether (and therefore saving the function call overhead).
Remaining issues:
1) Decide what to do with that security stuff.
2) Figure out how to make getters also work; we can't use the traceable native
stuff there.
3) Figure out whether we can enter this code sometimes with slimwrappers and
sometimes with XPCNativeWrapper on the same trace, and if so how we can
handle the situation. I suspect it cam, in fact.
Reporter | ||
Comment 8•15 years ago
|
||
For issue #3, which we are in fact hitting, and crashing as a result, can we just cheat and if the nsISupports has the vtable pointer to the XPCNativeWrapper vtable grab it's mIdentity? Or something?
Issue 4:
4) In a debug build, we die trying to call getElementsByTagName:
0x003b5f42 in JS_Assert (s=0x463e94 "prefixc <= 3", file=0x4605a8
"/Users/bzbarsky/mozilla/tracemonkey/mozilla/js/src/jstracer.cpp", ln=10566)
at /Users/bzbarsky/mozilla/tracemonkey/mozilla/js/src/jsutil.cpp:71
The relevant part is:
p *trcinfo->specializations
$6 = {
builtin = 0x118ed6f8,
prefix = 0x118e3d55 "IfTC",
argtypes = 0x118da664 "s",
flags = 9
}
Is there something that actually fails for the case of prefixc == 4, or do we just need to bump the assert?
Reporter | ||
Comment 9•15 years ago
|
||
mrbkap pointed out that for method calls we don't guard on the shape of |this| or anything (in particular not if coming through JSOP_CALL). So I'm going to back out the method part of this for now, and make the property part, where we do have shape guards, work.
Comment 10•15 years ago
|
||
Just put the shape into the PIC.
Comment 11•15 years ago
|
||
The assert seems bogus.
Comment 12•15 years ago
|
||
(In reply to comment #9)
> mrbkap pointed out that for method calls we don't guard on the shape of |this|
> or anything (in particular not if coming through JSOP_CALL).
It's there, under TR::record_JSOP_CALLPROP -> test_property_cache etc. The shape of the receiver guards the method (function-valued property) value. This is a big and important optimization, you can count on it.
/be
Reporter | ||
Comment 13•15 years ago
|
||
> Just put the shape into the PIC.
Good idea.
I tested putting the "is this a security wrapper?" check in the pic, and it didn't seem to help that much. Still doing some profiling to see what else is up.
Peterv is worried that we might have objects with different vtable layouts sharing classinfo... Not quite sure what to do about that yet. :(
Reporter | ||
Comment 14•15 years ago
|
||
What does help a lot is inlining the pic codepath and saving the call overhead. On the second microbenchmark in this bug, that drops the runtime to about 30ms (from 68 initially). That's compared to 42ms without inlining.
Now the caveat is that it's not exactly a tiny bit of code to inline...
Reporter | ||
Comment 15•15 years ago
|
||
Still need a solution for getters/setters. Andreas thinks we should just use a member of the JSContext as a secret handshake. Brendan didn't sound happy with it.
And of course still need to sort out the offset table situation.
Attachment #416734 -
Attachment is obsolete: true
Reporter | ||
Updated•15 years ago
|
blocking2.0: --- → ?
Updated•15 years ago
|
blocking2.0: ? → beta1
Reporter | ||
Comment 16•15 years ago
|
||
This probably needs to be remeasured with peterv's new fast paths.
Blocks: domperf
Updated•14 years ago
|
blocking2.0: beta1+ → final+
Updated•14 years ago
|
Comment 17•13 years ago
|
||
bz, is this bug still relevant or has DOM perf improvement moved on to other bugs?
Reporter | ||
Comment 18•13 years ago
|
||
That's a good question. I _think_ we have other plans for doing this now, and the approach from this bug is not going to get used.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•