456511 - (imacros) TM: Make conversion work on arbitrary JSObjects

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 456165 comment 13.  Brendan says that this is most certainly something to be done with trampolines, with arithmetic working on anything that has a sane valueOf.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Note that we need this to be able to trace the V8 testsuite.

Comment 2

[Andreas Gal :gal]

We have a related problem in the match_str_obj builtin, which potentially calls ToString on the object. This cases must be stringified on trace. Builtins should only take objects if they are guaranteed not to call script on them.

Comment 3

[Brendan Eich [:brendan]]

(In reply to comment #2)
> We have a related problem in the match_str_obj builtin,

js_String_p_match_obj

/be

Comment 4

[Brendan Eich [:brendan]]

This is not an enhancement, but a critical bug. See the dependent bug.

/be

Comment 5

[Brendan Eich [:brendan]]

Attachment: work in progress

This handles iterator testcases such as

var i = 0;
var o = {
    __iterator__: function () {
        return {
            next: function () {
                if (i == 10)
                    throw StopIteration;
                return i++;
            }
        };
    }
};
var a=[];
for(var j in o)
    a[j] = j*j;
print(uneval(a));

/be

Comment 6

[Brendan Eich [:brendan]]

Attachment: work in progress, iterators + object-to-string concatenation

Comment 7

[Andreas Gal :gal]

why don't we use regular js functions as macros? The call overhead on trace would be null. Just wondering.

Comment 8

[Brendan Eich [:brendan]]

We could, but we would have to write those functions in string constants or else in separately loaded .js files, figure out how to name them (in string constants they could be lambdas), figure out how to run the compiler to generate bytecode from those sources (without hurting startup or any other perf metric), and put up with the overhead of the record-time invocation (frame construction, etc.).

The imacros idea is to hand-code JSOP_* sequences that avoid the inefficiency and source overhead of self-hosting functions-as-macros. It seems worth it given all the combinations to be handled (getting into valueOf now for bz's testcase from bug 456165).

/be

Comment 9

[Brendan Eich [:brendan]]

Also we do guard in these macros, so we'd incur 4 stores of state.rp overhead for each inlined call. The call overhead on trace is not null.

/be

Comment 10

[Brendan Eich [:brendan]]

Forgot to mention that interposing a scripted helper function at recording time would mess with security checks, which are based on principals compiled into the function's script. We could clone helper functions' objects at record-time to carry overriding scope chain links to the right global objects (which own their own principals) to work around this, but it's more pain.

OTOH, using functions instead of macros would save fp->imacpc and the recovery code needed when the trace recorder aborts from within an imacro.

/be

Comment 11

[Brendan Eich [:brendan]]

Attachment: nearly ready w.i.p.

This gets bz's date-diff benchmark staying on trace, along with other tests I have to put into trace-test.js form. It should be good for some wins (modulo new latent bugs it exposes :-/) in SS.

/be

Comment 12

[Brendan Eich [:brendan]]

~1.5% win for me, 1341 w/o patch to 1316 w/ patch best of three bench.sh times. The main impetus is to keep us from reentering the interpreter, for correctness.

There is an #if 0 block in TraceRecorder::record_JSOP_GENERATOR, a thumbnail sketch only. The idea is to make a generator object on trace and flush native slots to its jsval slots on yield, and "build" the native slots from its jsvals when resuming the generator via a gen.next() or gen.send() built-in. Comments welcome there too.

/be

Comment 13

[Brendan Eich [:brendan]]

Attachment: ready apart from trace-test.js updates

Comment 14

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I think I'm missing something here: why do we want to always abort recording when we hit an imacro (call_imacro always returning false)?  Bug, or something subtle related to the interaction with the interpreter that might be worth a comment?

Comment 15

[Brendan Eich [:brendan]]

(In reply to comment #14)
> I think I'm missing something here: why do we want to always abort recording
> when we hit an imacro (call_imacro always returning false)?  Bug,

No, it works ;-).

> or something
> subtle related to the interaction with the interpreter that might be worth a
> comment?

See jstracer.h. We don't want a three-state return value for all the record_* hooks (I played with it, not worth the eyestrain or the extra overhead in all the call sites that do not use imacros). Instead we return false with fp->imacpc set, and only the RECORD macro expandions for the particular imacro-using ops include the "onfalse" code to switch regs.pc.

Facing a regression due to other changes, and the patch is out of date again. New patch shortly.

/be

Comment 16

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Yeah, I saw the TRACE_ARGS expansion, but didn't see the control flow in the onfalse part of RECORD_ARGS.  Thanks!

Comment 17

[Brendan Eich [:brendan]]

Attachment: ready for review

I should review too, I've been interdiff'ing a long time.

/be

Comment 18

[Andreas Gal :gal]

Don't we need a guard in binary() that the value method still exists at trace execution time? Or does the CALLPROP already guard sufficiently?

Why not use a large ip_adj high byte to indicate that we are not in an imacro so we don't have to bias the code position. At the very least we should add a static assert guarding the JSOP_NEXTITER shortcutting the initialization path.

This might want a static assert:

#define JSOP_IS_BINARY(op) ((uintN)((op) - JSOP_BITOR) <= (uintN)(JSOP_MOD - JSOP_BITOR))

We should wait for green before landing this. Its going to change the way we record a lot of the code. But we really want this in b2 so lets hope we can get it in soon.

Comment 19

[Brendan Eich [:brendan]]

(In reply to comment #18)
> Don't we need a guard in binary() that the value method still exists at trace
> execution time? Or does the CALLPROP already guard sufficiently?

The latter -- shapes guard method values :-).

> Why not use a large ip_adj high byte to indicate that we are not in an imacro
> so we don't have to bias the code position.

Because testing against zero is faster than testing against 0xff.

> At the very least we should add a
> static assert guarding the JSOP_NEXTITER shortcutting the initialization path.

Will do.

> This might want a static assert:
> 
> #define JSOP_IS_BINARY(op) ((uintN)((op) - JSOP_BITOR) <= (uintN)(JSOP_MOD -
> JSOP_BITOR))

Good point, I had that somewhere but seem to have lost it.

> We should wait for green before landing this. Its going to change the way we
> record a lot of the code. But we really want this in b2 so lets hope we can get
> it in soon.

The forthcoming patch passes the JS testsuite. More testing required, but it's looking good (I found a few glitches doing my own review). Thanks,

/be

Comment 20

[Brendan Eich [:brendan]]

Attachment: patch for more testing and review

Jesse, this patch should fix the fuzz-found bugs I linked to this bug. Let me know what's still broken. Thanks,

/be

Comment 21

[Jesse Ruderman]

Attachment: script that asserts with the patch

Assertion failure: (uint32)((atoms - script->atomMap.vector + ((uintN)(((regs.pc + 0)[1] << 8) | (regs.pc + 0)[2])))) < objects_->length, at ../jsinterp.cpp:6791

Comment 22

[Jesse Ruderman]

jsfunfuzz usually dies with:

Assertion failure: fp->imacpc ? atoms == COMMON_ATOMS_START(&rt->atomState) && GET_INDEX(regs.pc + 0) < js_common_atom_count : (size_t)(atoms - script->atomMap.vector) < (size_t)(script->atomMap.length - GET_INDEX(regs.pc + 0)), at ../jsinterp.cpp:5352

but I can't reproduce that easily.  (Long assertions!)

Comment 23

[Brendan Eich [:brendan]]

Attachment: fix abort_imacro to reset atoms

Comment 24

[Andreas Gal :gal]

Comment on attachment 347552 [details] [diff] [review]
fix abort_imacro to reset atoms

We should wait for green before landing this.

Comment 25

[Jesse Ruderman]

With this patch, "watch" causes issues, such as crashes [@ isPromoteInt] or assertions.  For example:

this.watch('n', function() { });
for (let j = 0; j < 9; ++j) { n = 3; }

Assertion failure: !TRACE_RECORDER(cx) ^ (jumpTable == recordingJumpTable), at ../jsinterp.cpp:4614

Comment 26

[Jesse Ruderman]

Same with setters:

this.__defineSetter__('x', function(q) q);
for (let j = 0; j < 5; ++j) { x = 0; } 

Crash [@ isPromoteInt

Comment 27

[Brendan Eich [:brendan]]

The latest jsfunfuzz finds are all due to ripping out deepAbort machinery, pending the work jorendorff's doing under the tracking bug 462027. I could put back the deepAbort junk pending that patch-series landing. Let me debug a bit and noodle on it.

/be

Comment 28

[Brendan Eich [:brendan]]

Correction -- I restored the deepAbort junk and those tests still crash, but only with the patch applied. Debugging...

/be

Comment 29

[Brendan Eich [:brendan]]

No, I was right the first time (in comment 27). My attempt to restore deepAbort suffered from too little context in isolating the diff to apply, resulting in the if (tr) {...} code at the bottom of js_Interpret being applied just after the return ok! Fixed patch next.

Jason, it's up to you to lay the deepAbort stuff to rest.

/be

Comment 30

[Brendan Eich [:brendan]]

Attachment: restore deepAbort machinery, lose abort_imacro bogosity, fix in-imacro exception handling

The imacro name fits: these must be interpreter-macros (or at least interpreter-somethings), they can't be hidden in the trace recorder as I initially hoped, and as bad remnants such as the abort_imacro code still tried to do.

When an imacro is active in a frame, any exception other than StopIteration from JSOP_NEXTITER will be handled as if thrown from the pc that called the imacro. The operands to the imacro may have been swapped (see, e.g. binary_obj_any_imacro in jstracer.cpp) but the stack will be popped back to the depth at which the try (if any) around the opcode with the implicit conversion was entered.

Hurray for termination rather than resumption semantics. Yes, this means you can't hand-code JSOP_TRY etc. in an imacro.

There's no need to abort_imacro, and indeed it would leave the stacked operands swapped in bad cases, so was just wrong. If trace recording aborts in an imacro, the imacro keeps on trucking until JSOP_STOP or exception.

Since the code at JSOP_STOP's if (fp->imacpc) {...} block is critical to imacro termination, I moved the end_imacro: label there and used a backward goto (which we try to avoid generally) from the hardcoded StopIteration exception handler way down under the error: label.

/be

Comment 31

[Brendan Eich [:brendan]]

BTW, bugzilla interdiff works (it didn't whine this time, did for a previous pair of patches -- yet still worked properly).

/be

Comment 32

[Brendan Eich [:brendan]]

Oops, silly null deref on testcase for bug 461307 (lucky I retested that one -- just saw bclary put it in the suite, so a retest tehre would have found it too). New patch anon.

/be

Comment 33

[Brendan Eich [:brendan]]

Attachment: stick a fork in it

Comment 34

[Jesse Ruderman]

Attachment: another script that asserts with the patch

Assertion failure: fp->slots + fp->script->nfixed + js_ReconstructStackDepth(cx, fp->script, fp->imacpc ? fp->imacpc : fp->regs->pc) == fp->regs->sp, at ../jstracer.cpp:3565

Comment 35

[Brendan Eich [:brendan]]

Attachment: it's done

That assertion is bogus for imacros (without excessive work to compute the imacro stack budget, which is easy enough to eyeball -- I will regret these words, some day!), but it did show up a bad assumption on my part (that adjacent static byte arrays will not be reordered).

/be

Comment 36

[Brendan Eich [:brendan]]

Attachment: er, this is done

Sorry, failed to invert the condition in that JS_ASSERT_IF.

/be

Comment 37

[Jesse Ruderman]

Attachment: another crashy script

During js_FinishJIT, triggers:

Assertion failed: _stats.freePages == _stats.pages (../nanojit/Fragmento.cpp:99)

Comment 38

[Andreas Gal :gal]

Thats a bit scary. ccing David.

Comment 39

[Andreas Gal :gal]

Has a regexp. Maybe its the other jit?

Comment 40

[Brendan Eich [:brendan]]

Not this patch. With attachment 347712 [details] I could not crash my patched build, until I qpopped, pulled and updated to get this much new stuff:

pulling from http://hg.mozilla.org/tracemonkey
searching for changes
adding changesets
adding manifests
adding file changes
added 26 changesets with 80 changes to 76 files
76 files updated, 0 files merged, 0 files removed, 0 files unresolved

and rebuilt. Now I see the assertbotch from comment 37, with or without this bug's imacros patch.

Jesse, please file a separate bug -- looks like dmandelin (please cc: the usual crew). Thanks,

/be

Comment 41

[Jesse Ruderman]

Oops!  Filed bug 464413 for that issue.

I'll do the rest of my testing tonight with this patch applied against rev 7aa86a0d3f80 (just before the regression).

Comment 42

[Brendan Eich [:brendan]]

Attachment: don't break liveconnect's C compilation, which includes jsinterp.h among others

Comment 43

[Brendan Eich [:brendan]]

Attachment: die liveconnect die!

Jesse, this should give you a build.

/be

Comment 44

[Jesse Ruderman]

This patch results in the most stable js shell I can remember.

Comment 45

[Andreas Gal :gal]

pushing in the patch for testing

Comment 46

[Andreas Gal :gal]

Backed out. Producing massive failures.

http://hg.mozilla.org/tracemonkey/rev/a40f2117bcc0

Comment 47

[Brendan Eich [:brendan]]

Attachment: patch to commit

Obvious, silly mistake (copy/paste thinko, really).

This passes Tp and Mochitests. I'm going to commit now, for testing when the infrastructure comes back online tomorrow morning.

/be

Comment 48

[Brendan Eich [:brendan]]

Fixed in tm:

http://hg.mozilla.org/tracemonkey/rev/51afdade0e86

/be

Comment 49

[Brendan Eich [:brendan]]

Noticed this earlier but forgot to fix it till this followup cset:

http://hg.mozilla.org/tracemonkey/rev/0b1f14dfd9cd

/be

Comment 50

[Brendan Eich [:brendan]]

This is on m-c now too, with followups:

http://hg.mozilla.org/mozilla-central/rev/51afdade0e86
http://hg.mozilla.org/mozilla-central/rev/0b1f14dfd9cd
http://hg.mozilla.org/mozilla-central/rev/5804f5597d3d

/be

Comment 51

[Bob Clary [:bc] (inactive)]

tests already in js1_8_1/trace/trace-test.js

Comment 52

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2