Closed Bug 569766 Opened 15 years ago Closed 15 years ago

JM: Get tracing working with fat values

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dmandelin, Assigned: dmandelin)

References

Details

Attachments

(1 file, 9 obsolete files)

Fat values is almost done for the interpreter. It's time to make it work with tracing too.
Depends on: 569767
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.
Attached patch WIP (obsolete) — Splinter Review
Most build errors are fixed with this patch, but I punted on some important stuff, esp. box_value and unbox_value.
Attached patch WIP2 (obsolete) — Splinter Review
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
Attached patch WIP 3 (obsolete) — Splinter Review
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
Attached patch WIP 4 (obsolete) — Splinter Review
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
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
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.
Attached patch WIP 5 (obsolete) — Splinter Review
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
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.
Attached patch WIP 6 (obsolete) — Splinter Review
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
(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.
Attached patch WIP 7 (obsolete) — Splinter Review
Attachment #450274 - Attachment is obsolete: true
Attached patch WIP 8 (fix Mac warnings) (obsolete) — Splinter Review
Attachment #450461 - Attachment is obsolete: true
Attached patch WIP 9 (obsolete) — Splinter Review
Attachment #450483 - Attachment is obsolete: true
Attachment #450518 - Attachment is obsolete: true
Attachment #450519 - Attachment is patch: true
Attachment #450519 - Attachment mime type: application/octet-stream → text/plain
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.
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.
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.
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%.
Marking this finished. We are still about 6% slower with -j vs. the 32-bit jsvals. Work on that will go in followon bugs.
Depends on: 571623
Depends on: 572042
Depends on: 572229
Depends on: 573084
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.

Attachment

General

Created:
Updated:
Size: