Closed
Bug 569766
Opened 15 years ago
Closed 15 years ago
JM: Get tracing working with fat values
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: dmandelin, Assigned: dmandelin)
References
Details
Attachments
(1 file, 9 obsolete files)
406.04 KB,
patch
|
Details | Diff | Splinter Review |
Fat values is almost done for the interpreter. It's time to make it work with tracing too.
Assignee | ||
Comment 1•15 years ago
|
||
I don't have too much idea what needs to be done for this yet, but I will offer a rough ETA of Friday, June 11. That is for a patch that passes trace-tests and goes in the fatval branch.
Assignee | ||
Comment 2•15 years ago
|
||
Most build errors are fixed with this patch, but I punted on some important stuff, esp. box_value and unbox_value.
Assignee | ||
Comment 3•15 years ago
|
||
This version compiles, but there are 40-50 spots that I commented out or otherwise punted on that I have to go through. A lot of those are things I don't understand, so I want to try reaching them in the debugger to see what happens.
Attachment #449165 -
Attachment is obsolete: true
Assignee | ||
Comment 4•15 years ago
|
||
This passes a few simple tests. There are still a lot of JS_ASSERT(0) areas to be replaced with working code. So far, I haven't run into anything truly hard (but many things that required a bit of thought or some help), and the code seems to get a little simpler with the new values.
Attachment #449375 -
Attachment is obsolete: true
Assignee | ||
Comment 5•15 years ago
|
||
I haven't rerun yet to see if my recent fixes introduced more bugs, but basically I think I'm down to these trace-test failures:
recursion not supported yet
c:\sources\fatval\js\src\trace-test\tests\basic\bug551705.js
c:\sources\fatval\js\src\trace-test\tests\basic\bug552196.js
c:\sources\fatval\js\src\trace-test\tests\basic\testBug520503-1.js
c:\sources\fatval\js\src\trace-test\tests\basic\testBug520503-3.js
c:\sources\fatval\js\src\trace-test\tests\basic\testBug560098.js
bad values
c:\sources\fatval\js\src\trace-test\tests\basic\testDeepBailWhileRecording.js
c:\sources\fatval\js\src\trace-test\tests\basic\testGroupAssignment.js
c:\sources\fatval\js\src\trace-test\tests\basic\testResumeOp.js
emitNative (native getters/setters) not supported yet
c:\sources\fatval\js\src\trace-test\tests\basic\regexpLastIndex.js
c:\sources\fatval\js\src\trace-test\tests\basic\testBug507425.js
c:\sources\fatval\js\src\trace-test\tests\basic\testNativeSetter.js
c:\sources\fatval\js\src\trace-test\tests\basic\testSetPropNeitherMissNorHit.js
The hard part is the emitNative stuff. Currently, one of the params of js::PropertyOp is |jsval id|. This is now a 64-bit struct on all platforms, so I don't think NJ knows how to compile a call to it yet. Maybe I'm wrong (which would be great). Otherwise, ideas include:
1. Teach NJ how to pass 64-bit struct params by value.
2. Create a dispatcher that takes arguments of a js::PropertyOp, then the arguments to the PropertyOp, except that the id would be a pointer. This should work easily, but clearly has some perf cost. Could be a good stopgap.
3. Fake up a signature that uses only 32-bit values, but whose arguments match PropertyOp. Then "box" the id specially for the fake argument list and call as normal in NJ.
#1 seems best, but I don't know if it is hard. Certainly it seems like a bunch of work to get it going on all platforms. Better ideas or advice appreciated.
Attachment #449392 -
Attachment is obsolete: true
Comment 6•15 years ago
|
||
Is jsid 32 bits on 32-bit platforms? If so, how about:
4. Change the API to use jsid id instead of jsval id, for all JSPropertyOp-matching signatures.
/be
Comment 7•15 years ago
|
||
We can fix NJ easily. It sorta kinda almost works actually.
Agree - it will just go through the FPU but still push correctly on the stack. We should be able to enable ARGTYPE_Q use on 32-bit (right now it's #ifdef'd off) and just use the same/similar logic as ARGTYPE_D. Maybe ARM ABI will need something different but no big deal.
Assignee | ||
Comment 9•15 years ago
|
||
Getting closer. The only bugs left that are detected by trace-tests are the lack of support for recursion and the lack of support for native property ops. There are some other FIXMEs that are important but apparently not detected by trace-tests. There are also a few opportunities for optimization that I won't attempt until everything is working. For example, calls to js_UnboxDouble can now be replaced by a LIR_ldd.
Attachment #449783 -
Attachment is obsolete: true
Comment 10•15 years ago
|
||
Unbox double should be able to unbox ints as well, so its a little diamond on trace. On top of that until we stop always forcing an i2f conversion, we need js_UnboxDouble so we can snoop and eliminate it.
Assignee | ||
Comment 11•15 years ago
|
||
This passes trace-tests. There are some warnings to be removed. There are also 3 JS_ASSERT(0) that are not tripped by trace-tests.
Attachment #450169 -
Attachment is obsolete: true
Assignee | ||
Comment 12•15 years ago
|
||
(In reply to comment #10)
> Unbox double should be able to unbox ints as well, so its a little diamond on
> trace.
I forgot about that.
> On top of that until we stop always forcing an i2f conversion, we need
> js_UnboxDouble so we can snoop and eliminate it.
That doesn't work any more. The input to the new unbox-on-trace code is a Value*. After the unbox-on-trace runs, the referent could get overwritten. So it is not safe to rebox by looking back to the input to unboxing.
Example: reading from a dense array. The input to the unbox-on-trace code is the address of the array slot. It is unboxed to some LIR-controlled value that is safe to use. After unboxing, other code could set that array slot. After this point, to box the unboxed value, we really need to box that one up--if we look past the js_UnboxDouble, we will read the new value instead of the original one.
Assignee | ||
Comment 13•15 years ago
|
||
Attachment #450274 -
Attachment is obsolete: true
Assignee | ||
Comment 14•15 years ago
|
||
Attachment #450461 -
Attachment is obsolete: true
Assignee | ||
Comment 15•15 years ago
|
||
Attachment #450483 -
Attachment is obsolete: true
Assignee | ||
Comment 16•15 years ago
|
||
Attachment #450518 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Attachment #450519 -
Attachment is patch: true
Attachment #450519 -
Attachment mime type: application/octet-stream → text/plain
Assignee | ||
Comment 17•15 years ago
|
||
http://hg.mozilla.org/users/lwagner_mozilla.com/fatval/rev/2deed859377a
This passes all the shell tests. I haven't tried a browser with it yet, but I wanted to get it in so it doesn't rot.
I get a 6% slowdown for SS on Windows, and a 3% slowdown on Mac. A microbenchmark shows that reading/writing ints from/to dense arrays is significantly slower, so that may be much of the reason. I am currently investigating that.
Comment 18•15 years ago
|
||
I sharked fannkuch (12% slowdown) and found all the extra time in js_Array_dense_setelem_double. Putting counters in the three js_Array_dense_setelem_* builtins, it seems that fatval tracing is calling the jsval setelem 0 times and the double setelem 1.2M times whereas trunk tracing calls the jsval setelem 1.06M times and the double setelem 183K times.
Comment 19•15 years ago
|
||
The same applies to crypto-aes: trunk calls the jsval setelem 137K times and the double setelem 300 times whereas fatval tracing does the almost exact opposite.
Comment 20•15 years ago
|
||
Another fun shark find: JSObject::resizeDenseArrayElements was generating the worst code you've ever seen in your life for the loop that writes holes to new elements. I suspect some combination of abstraction was confusing GCC. A simple rewrite (http://hg.mozilla.org/users/lwagner_mozilla.com/fatval/rev/54d42e9349ae) produces the code you'd expect and speeds up fatval tracing .7%.
Assignee | ||
Comment 21•15 years ago
|
||
Marking this finished. We are still about 6% slower with -j vs. the 32-bit jsvals. Work on that will go in followon bugs.
Updated•15 years ago
|
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•