Closed Bug 479110 Opened 16 years ago Closed 16 years ago

TM: avoid frequent mismatch exits

Categories

(Core :: JavaScript Engine, defect, P1)

Product:
Core
Component:
JavaScript Engine
Platform:
x86
macOS
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.1

People

(Reporter: gal, Assigned: gal)

References

Details

(Keywords: fixed1.9.1, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 24 obsolete files)

latest
31.15 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
We currently mismatch exit (which is expensive) in cases where we should try to trace. Example: var x = [1,"foo",2,"bar",3,"hello",4]; var y = 0; while (x.length > 0) { x.pop(); ++y; } Here the pop is specialized to one type only. We should record both type paths.
Flags: blocking1.9.1?
Priority: -- → P1
Blocks: 479102
Attached patch patch, dies on trace-tests (obsolete) — — Splinter Review
This patch turns unbox mismatch side exits into branch exits. It traces the following to test cases: var a = [1,"2",3,"4",5,"6",7,"8",9,"10",11,"12",13,"14",15,"16"]; while (a.length > 0) a.pop(); var values = ["hi", "hi", "hi", null, 5, 5.1, true, undefined, /foo/, function() {}, [], {}], types = []; for (var i = 0; i < values.length; i++) values[i]; But we die on CallIteratorNext_tn in trace-test on the first trace (in the harness). More work to follow.
Attachment #363085 - Attachment is obsolete: true
Attached patch v3, working (obsolete) — — Splinter Review
I had to disable math-tests jit-stat checking. Turns out they read the jitstats wrong all along and thought they get traced, which they do not. We trace more now and this now is visible, which the test don't expect.
Attachment #363144 - Attachment is obsolete: true
Attachment #363265 - Flags: review?(jorendorff)
Attached patch v3, with -U 8 (obsolete) — — Splinter Review
Attachment #363265 - Attachment is obsolete: true
Attachment #363369 - Flags: review?(jorendorff)
Attachment #363265 - Flags: review?(jorendorff)
I think this is less critical than the blacklist part of the bug. I am ok with not holding up b3 for this.
Priority: P1 → P2
Target Milestone: mozilla1.9.1b3 → mozilla1.9.1
Comment on attachment 363369 [details] [diff] [review] v3, with -U 8 >diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp > ti->typeMap.captureMissingGlobalTypes(cx, > *(ti->globalSlots), > ti->nStackTypes); > } > > /* read into registers all values on the stack and all globals we know so far */ > import(treeInfo, lirbuf->sp, stackSlots, ngslots, callDepth, typeMap); > >+ /* unbox any boxed values we imported as jsvals */ >+ if (pendingUnbox_ins) { >+ JS_ASSERT(fragment != fragment->root && cx->fp->regs->pc == (jsbytecode*)fragment->ip); We can't be entering an initial trace if we're falling out of a trace after calling a jsval native that didn't unbox predictably, correct? Not sure I understand why the second condition is being asserted, or at least not why it couldn't always be asserted, would be useful to know... >+ /* >+ * If we are attaching a branch to an unbox operation that failed, make a note to unbox >+ * the value soon we imported everything. >+ */ the value as soon as we have imported >@@ -2084,23 +2112,27 @@ TraceRecorder::snapshot(ExitType exitTyp > /* Determine the type of a store by looking at the current type of the actual value the > interpreter is using. For numbers we have to check what kind of store we used last > (integer or double) to figure out what the side exit show reflect in its typemap. */ > FORALL_SLOTS(cx, ngslots, treeInfo->globalSlots->data(), callDepth, > *m++ = determineSlotType(vp); > ); > JS_ASSERT(unsigned(m - typemap) == ngslots + stackSlots); > >- /* If we are capturing the stack state on a specific instruction, the value on >- the top of the stack is a boxed value. */ >+ /* >+ * If we are capturing the stack state on a specific instruction, the value on >+ * the top of the stack is a boxed value. >+ */ >+ if ((pendingTraceableNative && (pendingTraceableNative->flags & JSTN_UNBOX_AFTER)) || >+ pendingUnbox_ins) { >+ typemap[stackSlots - 1] = JSVAL_BOXED; >+ } I'd put the entire condition on one line, looks like it fits, to avoid the condition-lines aligning exactly with the children of the block statement -- a pitfall of +4 indentation with brace on previous line. So many C-mandated or warning-silencing parentheses, alas... >-/* Record LIR for a tableswitch or tableswitchx op. We record LIR only >+/* Record LIR for a ploop0l; or tableswitchx op. We record LIR only Errant sed or something? Don't see how this is relevant. >@@ -5928,20 +5972,21 @@ TraceRecorder::guardDenseArray(JSObject* > if (cond) { >+ LIns* exit = snapshot(exitType); > /* Guard array length */ >- LIns* exit = guard(true, >- lir->ins2(LIR_ult, idx_ins, stobj_get_fslot(obj_ins, JSSLOT_ARRAY_LENGTH)), >- exitType)->oprnd2(); >+ guard(true, >+ lir->ins2(LIR_ult, idx_ins, stobj_get_fslot(obj_ins, JSSLOT_ARRAY_LENGTH)), >+ exit); Mm, much better than the inscrutable ->oprnd2() here, no need to know the exact formatting and use of the instruction guard returned. >@@ -7561,36 +7606,38 @@ TraceRecorder::record_FastNativeCallComp > Instead, snapshot(), which is invoked from unbox_jsval() below, will see "invoked from unbox_jsval()" is a lie now, please update. >+ LIns* exit = NULL; >+ > if (JSTN_ERRTYPE(pendingTraceableNative) == FAIL_STATUS) { > #ifdef DEBUG > // Keep cx->bailExit null when it's invalid. > lir->insStorei(INS_CONSTPTR(NULL), cx_ins, (int) offsetof(JSContext, bailExit)); > #endif > guard(true, > lir->ins_eq0( > lir->insLoad(LIR_ld, cx_ins, (int) offsetof(JSContext, builtinStatus))), >- STATUS_EXIT); >+ exit = snapshot(STATUS_EXIT)); Would rather see the assignment as its own statement prior to guard, for clarity and for the added sequence point, C argument order of evaluation being undefined... >+ jsval pendingUnbox_value; >+ nanojit::LIns* pendingUnbox_ins; pendingBoxedValue and pendingBoxedIns seem like better names to me to indicate they're boxed and need to be unboxed, also avoiding underscores -- we don't use underscores that often in SpiderMonkey code, although jstracer.cpp may have started to corrupt us. >diff --git a/js/src/math-trace-tests.js b/js/src/math-trace-tests.js >+ // Disable jitstats check. This never worked right. The actual part of the >+ // loop we cared about was never traced. We traced the filler parts early >+ // and then took a mismatch side exit on every subequent array read with >+ // a different type (gal, discovered when fixing bug 479110). >+ // testfunc.jitstats = { >+ // recorderStarted: 1, >+ // recorderAborted: 0, >+ // traceTriggered: 1 >+ // }; Need to file a followup to fix this, if we don't actually know that the behavior we were expecting to happen, would actually happen, for these functions... I'm willing to consider this an r=me if you are. :-)
Comment on attachment 363369 [details] [diff] [review] v3, with -U 8 Do we have to have the new pendingUnbox_ fields? It seems like it would be nicer to have TraceRecorder::import(Lins* ...) to do the unboxing, unless the import code has to be infallible. >+ JS_ASSERT(fragment != fragment->root && cx->fp->regs->pc == (jsbytecode*)fragment->ip); I agree with Waldo that this should be two separate assertions, and that it looks like the second half should be moved outside the enclosing `if`. >- /* WARNING: don't return before restoring the original pc if (resumeAfter). */ >+ /* >+ * When recording a native invocation that returns a jsval, set pc to resume after the call >+ * since we can't re-execute it. Make sure we don't return from this function until we restore >+ * the original value of regs->pc. >+ */ > bool resumeAfter = (pendingTraceableNative && > JSTN_ERRTYPE(pendingTraceableNative) == FAIL_STATUS); The words "that returns a jsval" are wrong. Perhaps "When calling a _FAIL native, make the snapshot's pc point to the next instruction after the CALL or APPLY. Even on failure, a _FAIL native must not be called again from the interpreter." Also, can we drop the "don't return from this function until" part? MUST_FLOW_THROUGH seems clear enough to me. >+ > if (resumeAfter) { > JS_ASSERT(*pc == JSOP_CALL || *pc == JSOP_APPLY); >+ /* Move past the JSOP_CALL/JSOP_APPLY opcode. */ > pc += cs.length; > regs->pc = pc; This one-line comment doesn't seem worth it to me (and fwiw put a blank line before any comment that takes one or more whole lines). >- /* If we are capturing the stack state on a specific instruction, the value on >- the top of the stack is a boxed value. */ >+ /* >+ * If we are capturing the stack state on a specific instruction, the value on >+ * the top of the stack is a boxed value. >+ */ >+ if ((pendingTraceableNative && (pendingTraceableNative->flags & JSTN_UNBOX_AFTER)) || >+ pendingUnbox_ins) { >+ typemap[stackSlots - 1] = JSVAL_BOXED; >+ } This makes more sense inside the "resumeAfter" block. The boxed value is only there if the stack we're capturing is the stack *after* the call. In the !resumeAfter block, you could assert that the JSTN_UNBOX_AFTER flag isn't set. While you're in here, could you improve that comment? "on a specific instruction" makes no sense to me. (I guess the alternative would be that we're "on a vague instruction"...) >+ if (guard) { >+ GuardRecord* lr = guard->record(); >+ VMSideExit* e = (VMSideExit*)lr->exit; >+ debug_only_v(printf(" lr=%p exitType=%d\n", (SideExit*)e, e->exitType);) >+ } else { >+ debug_only_v(printf(" redundant guard, eliminated\n");) >+ } This change to TR::guard() is nice. I noticed that some LIRWriters upstream of the VerboseWriter cache instructions, so when we add our own spew, the order of the output is not quite right. It would be nice to find a fix for that eventually. > guard(true, > lir->ins_eq0( > lir->insLoad(LIR_ld, cx_ins, (int) offsetof(JSContext, builtinStatus))), >- STATUS_EXIT); >+ exit = snapshot(STATUS_EXIT)); What Waldo said. > unbox_jsval((sprop->shortid == REGEXP_SOURCE) ? JSVAL_STRING : JSVAL_BOOLEAN, >- v_ins); >+ v_ins, >+ snapshot(BRANCH_EXIT)); BRANCH_EXIT is misleading here. We can't branch here; it would be a bad bug if we did, right? I think MISMATCH_EXIT is saner, though perhaps it should never happen either way. This patch shouldn't go in without a trace-test. r+ with those changes.
Attachment #363369 - Flags: review?(jorendorff) → review+
I will rework the patch. I suggest a new round of reviews after that.
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Attached patch refreshed, not reworked yet (obsolete) — — Splinter Review
Attachment #363369 - Attachment is obsolete: true
Blocks: 458016
Flags: wanted1.9.1+
Flags: blocking1.9.1-
Flags: blocking1.9.1+
Attached patch v5 (obsolete) — — Splinter Review
Attachment #366861 - Attachment is obsolete: true
Attached patch v6 (obsolete) — — Splinter Review
refreshed patch
Attachment #366870 - Attachment is obsolete: true
Attachment #367551 - Flags: review?(jwalden+bmo)
I was talking to brendan about this the other day, and we had an idea that seems simpler, at least for the iterator case. Because semantically the program never sees the JSVAL_HOLE (it is never stored into a variable, it is just a signal that there are no variables. Thus, it seems that making the wrapper for js_IteratorNext smarter would be enough to solve the problem and avoid the exit entirely. Btw, the imacros contain logic that tries to check for the JSVAL_HOLE, but in the current code that is dead (because we take the mismatch exit first). So the imacros for iterators should be updated as part of this patch.
This patch isn't strictly about the iterator problem. Any heap/API access that returns more than one possible type requires this. I am all for eliminating JSVAL_HOLE in the iterator path. Its a gross hack. We could use LIR_alloc and create additional out params.
(In reply to comment #14) > This patch isn't strictly about the iterator problem. Any heap/API access that > returns more than one possible type requires this. I am all for eliminating > JSVAL_HOLE in the iterator path. Its a gross hack. We could use LIR_alloc and > create additional out params. Two stack results would be ideal -- |nextval? boolean| as the imacro comment says. What's the patch look like? /be
Need a separate bug for that. Thats pretty much unrelated to this one. This deals with array and heap access mostly.
review ping
Attachment #367551 - Flags: review?(jwalden+bmo) → review+
http://hg.mozilla.org/tracemonkey/rev/e71cb3993380
Whiteboard: fixed-in-tracemonkey
Backed out due to tinderbox failures: http://hg.mozilla.org/tracemonkey/rev/830772ae7bf4
Whiteboard: fixed-in-tracemonkey
mandelbrot fails too (trace-tests in debug mode)
Attached patch refreshed (obsolete) — — Splinter Review
Attachment #367551 - Attachment is obsolete: true
484584 has a very nice test case for this bug.
Attachment #368722 - Attachment is obsolete: true
Comment on attachment 368723 [details] [diff] [review] refreshed patch accidentally dropped some code > this->generatedTraceableNative = new JSTraceableNative(); > JS_ASSERT(generatedTraceableNative); Argh. OOM happens, js/src/*.cpp checks for it, embedders test for it. I realize we're in a ctor, but isn't there a way to leave a trigger for orderly OOM reporting and auto-destruction of the incompletely initialized instance? >+ pendingBoxedValue = *p; We need to mark this from the GC, right? >@@ -2780,16 +2820,17 @@ TraceRecorder::emitTreeCall(Fragment* in > { > TreeInfo* ti = (TreeInfo*)inner->vmprivate; > /* Invoke the inner tree. */ > LIns* args[] = { INS_CONSTPTR(inner), lirbuf->state }; /* reverse order */ > LIns* ret = lir->insCall(&js_CallTree_ci, args); > /* Read back all registers, in case the called tree changed any of them. */ > import(ti, inner_sp_ins, exit->numStackSlots, exit->numGlobalSlots, > exit->calldepth, getFullTypeMap(exit)); >+ JS_ASSERT(!pendingBoxedIns); > /* Restore sp and rp to their original values (we still have them in a register). */ Blank line before comment-on-1-or-more-lines-unless-after-{. >+ // Disable jitstats check. This never worked right. The actual part of the >+ // loop we cared about was never traced. We traced the filler parts early >+ // and then took a mismatch side exit on every subequent array read with >+ // a different type (gal, discovered when fixing bug 479110). >+ // testfunc.jitstats = { >+ // recorderStarted: 1, >+ // recorderAborted: 0, >+ // traceTriggered: 1 >+ // }; /* ... */ comment is better here. Sorry for drive-by, want to see this patch land! /be
any news here?
Priority: P2 → P1
Attached patch patch (obsolete) — — Splinter Review
Use the original type map at the anchor instead of trying to capture a new one when attaching a trace to an unbox operation.
Attachment #368723 - Attachment is obsolete: true
The patch fails TUnit in test_mochikit. I reduced the failure by removing everything from manifest.json except MochiKit-Visual.html. Also, all tests can be removed from MochiKit-Visual.html except these three: multiple(["elt1", "ctn1"], appear, {afterFinish: function (effect) { is(effect.element.style.display != 'none', true, "multiple ok"); }}); toggle("elt3", "size", {afterFinish: function () { is(getElement('elt3').style.display != 'none', true, "toggle with css ok"); }}); toggle("elt3", "size", {afterFinish: function () { is(getElement('elt3').style.display, 'none', "toggle with css ok"); }}); The failure mode is the mochitest not completing. It complains about MochiKit.Base being undefined (see JS console).
This starts to fail in MochiKit.Base.concat with Gal's patch. The way to run it is like this: python runtests.py --test-path=dom/tests/mochitest/ajax/mochikit/tests/MochiKit-Visual.html I'll attach a patch with some dump() calls to help debugging.
Without patch: ... call concat + 28 call concat + 29 call concat + 30 ******** Starting test ********* call concat + 31 typeof SimpleTest.is() first param 'a': boolean call concat + 32 typeof SimpleTest.is() first param 'a': boolean call concat + 33 call concat + 34 typeof SimpleTest.is() first param 'a': boolean call concat + 35 typeof SimpleTest.is() first param 'a': string call concat + 36 call concat + 37 call concat + 38 ... With patch: ... call concat + 28 call concat + 29 call concat + 30 ******** Starting test ********* call concat + 31 typeof SimpleTest.is() first param 'a': boolean call concat + 32 typeof SimpleTest.is() first param 'a': boolean call concat + 33 call concat + 34 typeof SimpleTest.is() first param 'a': boolean call concat + 35 typeof SimpleTest.is() first param 'a': number JavaScript error: http://localhost:8888/tests/dom/tests/mochitest/ajax/mochikit/tests/SimpleTest/SimpleTest.js, line 48: MochiKit.Base is undefined JavaScript error: http://localhost:8888/tests/dom/tests/mochitest/ajax/mochikit/MochiKit/Base.js, line 912: MochiKit.Base is undefined call concat + 36 typeof SimpleTest.is() first param 'a': number JavaScript error: http://localhost:8888/tests/dom/tests/mochitest/ajax/mochikit/tests/SimpleTest/SimpleTest.js, line 48: MochiKit.Base is undefined JavaScript error: http://localhost:8888/tests/dom/tests/mochitest/ajax/mochikit/MochiKit/Base.js, line 912: MochiKit.Base is undefined WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520006: file /Users/sayrer/dev/clean-tm/xpcom/io/nsLocalFileOSX.mm, line 425 ...
More data. The type of the MochiKit global is also changed to 'number' on that first bogus occurrence with the patch applied. ******** Starting test ********* call concat + 31 typeof SimpleTest.is() first param 'a': boolean MochiKit: [object Object] typeof MochiKit: object call concat + 32 typeof SimpleTest.is() first param 'a': boolean MochiKit: [object Object] typeof MochiKit: object call concat + 33 call concat + 34 typeof SimpleTest.is() first param 'a': boolean MochiKit: [object Object] typeof MochiKit: object call concat + 35 typeof SimpleTest.is() first param 'a': number MochiKit: 502085600 typeof MochiKit: number JavaScript error: http://localhost:8888/tests/dom/tests/mochitest/ajax/mochikit/tests/SimpleTest/SimpleTest.js, line 50: MochiKit.Base is undefined JavaScript error: http://localhost:8888/tests/dom/tests/mochitest/ajax/mochikit/MochiKit/Base.js, line 912: MochiKit.Base is undefined
SimpleTest.is = function (a, b, name) { dump("typeof SimpleTest.is() first param 'a': " + typeof a + "\n") dump("MochiKit: " + MochiKit + "\n"); dump("typeof MochiKit: " + typeof MochiKit + "\n"); var repr = MochiKit.Base.repr; SimpleTest.ok(a == b, name, "got " + repr(a) + ", expected " + repr(b)); };
apply this patch and gal's, run python runtests.py --test-path=dom/tests/mochitest/ajax/mochikit/tests/MochiKit-Visual.html
Attached patch better dump diagnostics (obsolete) — — Splinter Review
Attached file stack (obsolete) —
JS_ASSERT(JSVAL_TAG(v) == JSVAL_OBJECT); /* if this fails the pointer was not aligned */
entering REGEXP trace at file:///Users/gal/workspace/tracemonkey-repository/debug-build/_tests/testing/mochitest/tests/dom/tests/mochitest/ajax/mochikit/MochiKit/Style.js:150@343, code: 0x11498f82 leaving REGEXP trace entering REGEXP trace at file:///Users/gal/workspace/tracemonkey-repository/debug-build/_tests/testing/mochitest/tests/dom/tests/mochitest/ajax/mochikit/MochiKit/Style.js:150@343, code: 0x11498f82 leaving REGEXP trace Looking for compat peer 41@60, from 0x19151c90 (ip: 0x1c87778c) checking vm types 0x19151c90 (ip: 0x1c87778c): callee0=O/O this0=O/O argv0=O/O argv1=O/O vars0=I/I vars1=O/O vars2=B/S string != tag6 checking vm types 0x19160fa0 (ip: 0x1c87778c): callee0=O/O this0=O/O argv0=O/O argv1=O/O vars0=I/I vars1=O/O vars2=B/B stack0=O/O stack1=S/S global0=O/O global1=I/I entering trace at file:///Users/gal/workspace/tracemonkey-repository/debug-build/_tests/testing/mochitest/tests/dom/tests/mochitest/ajax/mochikit/MochiKit/Base.js:41@60, native stack slots: 15 code: 0x1c989e23 global: object<0x1a989f20:Object> int<109> stack: callee0=object<0x124665e8:Function> this0=object<0x1a98f000:Object> argv0=object<0x1c9948e0:Object> argv1=object<0x1c994800:Object> vars0=int<1> vars1=object<0x1c994800:Object> vars2=boolean<2> stack0=object<0x1c994960:Iterator> stack1=string<0x12343a90> leaving trace at file:///Users/gal/workspace/tracemonkey-repository/debug-build/_tests/testing/mochitest/tests/dom/tests/mochitest/ajax/mochikit/MochiKit/Base.js:42@76, op=getelem, lr=0x1c97148c, exitType=0, sp=5, calldepth=0, cycles=26450 global0=int<446209824> global1=Assertion failure: type == JSVAL_OBJECT, at ../../../js/src/jstracer.cpp:1642 Program received signal SIGTRAP, Trace/breakpoint trap. JS_Assert (s=0x4e <Address 0x4e out of bounds>, file=0x4e <Address 0x4e out of bounds>, ln=78) at ../../../js/src/jsutil.cpp:69 69 abort(); (gdb)
This is the bug: enter: global: object<0x16085380:Object> int<109> leave: global0=int<369644416> global1=Assertion failure: global0 is the MochiKit variable. (gdb) p/x 369644416 $17 = 0x16085380 (gdb)
Just a wild guess: the patch uses the entry type map instead of capturing a new one. that map might be missing globals though that were not present when we started the tree.
Attached patch patch (obsolete) — — Splinter Review
clone the anchor and unpack boxed value early
Attachment #370323 - Attachment is obsolete: true
This iloops on the following test case: function testBug466128() { for each (var b in [1, 2, "three", 4, 5, 6, 7, 8]) { } } testBug466128(); We never compiled the other type variant of the loop, so this may be a bug in the patch, or not.
Still iloops on the same test case. I hate this bug so much.
Attachment #370521 - Attachment is obsolete: true
Attachment #370533 - Attachment is obsolete: true
Attachment #370536 - Attachment is obsolete: true
Attachment #370542 - Attachment is obsolete: true
Attachment #370553 - Attachment is obsolete: true
passes trace-tests, sending it to try server
Attachment #370556 - Attachment is obsolete: true
/opt/local/bin/python -u \ ../../../../../testing/xpcshell/runxpcshelltests.py \ ../../../../dist/bin/xpcshell \ ../../../../_tests/xpcshell/test_urlformatter/unit TEST-UNEXPECTED-FAIL | /Users/gal/workspace/tracemonkey-repository/debug-build/_tests/xpcshell/test_urlformatter/unit/test_urlformatter.js | test failed, see following log: >>>>>>> ### XPCOM_MEM_LEAK_LOG defined -- logging leaks to /var/folders/5l/5lEsGhCuFR0CKHBlrAwBU++++TI/-Tmp-/runxpcshelltests_leaks.log *** test pending *** test finished *** exiting *** PASS *** WARNING: nsExceptionService ignoring thread destruction after shutdown: file ../../../xpcom/base/nsExceptionService.cpp, line 194 ###!!! ASSERTION: bad!: 'Error', file ../../../../../js/src/xpconnect/src/xpccallcontext.cpp, line 88 nsTArray_base::IncrementLength(unsigned int)+0x00004C74 [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/components/libxpconnect.dylib +0x0000D878] nsXPCOMCycleCollectionParticipant::nsXPCOMCycleCollectionParticipant()+0x00004606 [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/components/libxpconnect.dylib +0x00047F90] nsXPCOMCycleCollectionParticipant::nsXPCOMCycleCollectionParticipant()+0x00005E61 [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/components/libxpconnect.dylib +0x000497EB] nsTArray_base::GetAutoArrayBuffer()+0x00001CB7 [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/components/libxpconnect.dylib +0x00041BC1] NS_RegisterMemoryReporter(nsIMemoryReporter*)+0x0000081C [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/libxpcom_core.dylib +0x000A3288] nsCreateInstanceByContractID::nsCreateInstanceByContractID(char const*, nsISupports*, unsigned int*)+0x0000D525 [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/components/libxpconnect.dylib +0x0002D9DF] nsSupportsArray::AppendElement(nsISupports*)+0x00002341 [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/components/libxpconnect.dylib +0x0004EDBF] nsCRT::IsUpper(char)+0x0000470C [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/components/libxpconnect.dylib +0x0005C510] js_obj_defineSetter(JSContext*, unsigned int, long*)+0x00002806 [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/libmozjs.dylib +0x0009F90A] js_TraceContext+0x00001A3F [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/libmozjs.dylib +0x0005F847] js_DumpAtoms+0x00001ECA [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/libmozjs.dylib +0x000290C2] JS_DestroyContext+0x00000019 [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/libmozjs.dylib +0x0000D689] NSGetModule+0x00002E82 [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/components/libxpconnect.dylib +0x0003F476] NSGetModule+0x00002733 [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/components/libxpconnect.dylib +0x0003ED27] DumpJSValue+0x00000642 [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/components/libxpconnect.dylib +0x00005F66] UNKNOWN [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/components/libxpconnect.dylib +0x000024F7] UNKNOWN [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/components/libxpconnect.dylib +0x00002757] nsTArray_base::IsEmpty() const+0x00002F9F [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/components/libxpconnect.dylib +0x0003C5ED] NS_NewGenericFactory(nsIGenericFactory**, nsModuleComponentInfo const*)+0x0000022A [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/libxpcom_core.dylib +0x000134E6] NS_NewGenericFactory(nsIGenericFactory**, nsModuleComponentInfo const*)+0x0000027E [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/libxpcom_core.dylib +0x0001353A] NS_NewGenericFactory(nsIGenericFactory**, nsModuleComponentInfo const*)+0x000003C6 [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/libxpcom_core.dylib +0x00013682] nsSupportsWeakReference::~nsSupportsWeakReference()+0x00001C4A [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/libxpcom_core.dylib +0x00082A76] nsSupportsWeakReference::~nsSupportsWeakReference()+0x00001CCA [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/libxpcom_core.dylib +0x00082AF6] nsSupportsWeakReference::~nsSupportsWeakReference()+0x0000401E [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/libxpcom_core.dylib +0x00084E4A] nsSupportsWeakReference::~nsSupportsWeakReference()+0x000031DB [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/libxpcom_core.dylib +0x00084007] nsSupportsWeakReference::~nsSupportsWeakReference()+0x00003E17 [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/libxpcom_core.dylib +0x00084C43] PL_DHashTableEnumerate+0x000000EA [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/libxpcom_core.dylib +0x00002CD4] nsSupportsWeakReference::~nsSupportsWeakReference()+0x00003A81 [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/libxpcom_core.dylib +0x000848AD] nsSupportsWeakReference::~nsSupportsWeakReference()+0x00003031 [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/libxpcom_core.dylib +0x00083E5D] NS_GetComponentRegistrar_P+0x000052C9 [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/libxpcom_core.dylib +0x0007ED51] NS_ShutdownXPCOM_P+0x00000528 [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/libxpcom_core.dylib +0x0001A06E] NS_ShutdownXPCOM+0x00000011 [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/libxpcom.dylib +0x00001A49] start+0x00004385 [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/xpcshell +0x00004F65] start+0x000000FB [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/xpcshell +0x00000CDB] start+0x00000029 [/Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/xpcshell +0x00000C09] <<<<<<< make[3]: *** [check] Error 1 make[2]: *** [check] Error 2 make[1]: *** [check] Error 2 make: *** [check] Error 2 Dies in make check.
Previous failure is another random known problem in m-c. Disabling that we fail here: whale:test gal$ make check-one SOLO_FILE=test_storage_legacy_3.js /opt/local/bin/python \ ../../../../../testing/xpcshell/runxpcshelltests.py \ --test=test_storage_legacy_3.js \ ../../../../dist/bin/xpcshell \ ../../../../_tests/xpcshell/test_passwordmgr/unit TEST-UNEXPECTED-FAIL | /Users/gal/workspace/tracemonkey-repository/debug-build/_tests/xpcshell/test_passwordmgr/unit/test_storage_legacy_3.js | test failed, see following log: >>>>>>> ### XPCOM_MEM_LEAK_LOG defined -- logging leaks to /var/folders/5l/5lEsGhCuFR0CKHBlrAwBU++++TI/-Tmp-/runxpcshelltests_leaks.log *** test pending WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520006: file ../../../xpcom/io/nsLocalFileOSX.mm, line 425 WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520006: file ../../../xpcom/io/nsLocalFileOSX.mm, line 425 WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520006: file ../../../xpcom/io/nsLocalFileOSX.mm, line 425 WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520006: file ../../../xpcom/io/nsLocalFileOSX.mm, line 425 WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520006: file ../../../xpcom/io/nsLocalFileOSX.mm, line 425 WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520006: file ../../../xpcom/io/nsLocalFileOSX.mm, line 425 FAILED in test #9 -- checking correct storage of mime64 converted entries: [Exception... "'[JavaScript Error: "login.wrappedJSObject is undefined" {file: "file:///Users/gal/workspace/tracemonkey-repository/debug-build/dist/bin/components/storage-Legacy.js" line: 1231}]' when calling method: [nsILoginManagerStorage::getAllLogins]" nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" location: "JS frame :: /Users/gal/workspace/tracemonkey-repository/debug-build/_tests/xpcshell/test_passwordmgr/unit/head_storage_legacy_1.js :: anonymous :: line 157" data: yes] *** FAIL *** WARNING: nsExceptionService ignoring thread destruction after shutdown: file ../../../xpcom/base/nsExceptionService.cpp, line 194 nsStringStats => mAllocCount: 2907 => mReallocCount: 74 => mFreeCount: 2907 => mShareCount: 241 => mAdoptCount: 87 => mAdoptFreeCount: 87 <<<<<<< make: *** [check-one] Error 1
If I change the native return type unboxing side exit to MISMATCH_EXIT the test case passes.
extracted shell testcase: function f(logins) { var result = []; for each (var hostLogins in logins) { result = result.concat(hostLogins); } print(uneval(result)); }; f({'http://dummyhost.mozilla.org':[{}]}); f({'http://dummyhost.mozilla.org':[{}]}); f({'http://dummyhost2.mozilla.org':[{}, {}]}); f({'http://dummyhost2.mozilla.org':[{}, {}]}); f({'http://dummyhost.mozilla.org':[{}], 'http://dummyhost2.mozilla.org':[{}, {}]}); f({'http://dummyhost.mozilla.org':[{}], 'http://dummyhost2.mozilla.org':[{}, {}]}); f({'https://site.org':[{}]}); f({'https://site.org':[{}]}); f({}); f({'https://site.org':[{}]}); f({'https://site.org':[{}]}); f({'http://dummyhost.mozilla.org':[{}]}); f({'http://dummyhost.mozilla.org':[{}]}); f({'http://dummyhost.mozilla.org':[{}]}); f({'http://dummyhost.mozilla.org':[{}]}); f({'http://dummyhost.mozilla.org':[{}], 'http://dummyhost2.mozilla.org':[{}]});
Slightly reduced: function f(logins) { var result = []; for each (var hostLogins in logins) { result = result.concat(hostLogins); } print(uneval(result)); }; f({'x':[{}]}); f({'x':[{}, {}]}); f({'x':[{}, {}]}); f({'x':[{}], 'y':[{}, {}]}); f({'x':[{}], 'y':[{}, {}]}); f({'x':[{}]}); f({'x':[{}], 'y':[{}]});
Attached patch v627 (obsolete) — — Splinter Review
Place the unboxed value into the right location on the stack when attaching a trace to a type mismatch, otherwise the old value (here callee) is visible there, which causes all sorts of hilarious things to happen.
Attachment #370561 - Attachment is obsolete: true
Attachment #370993 - Flags: review?(brendan)
Attachment #370993 - Flags: review?(brendan) → review+
Comment on attachment 370993 [details] [diff] [review] v627 >- JS_ASSERT(!_fragment->vmprivate && ti); >+ JS_ASSERT(!_fragment->vmprivate && ti && cx->fp->regs->pc == (jsbytecode*)_fragment->ip); JS_ASSERT_IF(_fragment->vmprivate, ...); >+ /* >+ * This is potentially the typemap of the side exit and thus shorter than the tree's >+ * global type map. >+ */ > if (ngslots < length) > mergeTypeMaps(&globalTypeMap/*out param*/, &ngslots/*out param*/, > treeInfo->globalTypeMap(), length, > (uint8*)alloca(sizeof(uint8) * length)); Full brace multiline consequents (or single- if cond is multi-). >+ /* >+ * When calling a _FAIL native, make the snapshot's pc point to the next >+ * instruction after the CALL or APPLY. Even on failure, a _FAIL native must not >+ * be called again from the interpreter. Tighten up the interpreter assertion about what op StopIteration might be thrown at? (the one that had or needed to have call/dup/true) >@@ -2136,25 +2169,18 @@ TraceRecorder::snapshot(ExitType exitTyp > otherwise we have to create our own. */ > VMSideExit** exits = treeInfo->sideExits.data(); > unsigned nexits = treeInfo->sideExits.length(); > if (exitType == LOOP_EXIT) { > for (unsigned n = 0; n < nexits; ++n) { > VMSideExit* e = exits[n]; > if (e->pc == pc && e->imacpc == fp->imacpc && > !memcmp(getFullTypeMap(exits[n]), typemap, typemap_size)) { ... > AUDIT(mergedLoopExits); >- return data; >+ return clone(exits[n]); Comment this? >+TraceRecorder::clone(VMSideExit* exit) >+{ >+ LIns* data = lir->skip(sizeof(GuardRecord)); >+ GuardRecord* rec = (GuardRecord*)data->payload(); >+ /* setup guard record structure with shared side exit */ Blank line before, and make a sentence (capitalize + full stop) in the comment. >+ memset(rec, 0, sizeof(GuardRecord)); >+ rec->exit = exit; >+ exit->addGuard(rec); >+ return data; >+} >+ >+JS_REQUIRES_STACK LIns* >+TraceRecorder::copy(VMSideExit* copy) >+{ >+ unsigned typemap_size = copy->numGlobalSlots + copy->numStackSlots; >+ LIns* data = lir->skip(sizeof(GuardRecord) + >+ sizeof(VMSideExit) + >+ typemap_size * sizeof(uint8)); >+ GuardRecord* rec = (GuardRecord*)data->payload(); >+ VMSideExit* exit = (VMSideExit*)(rec + 1); >+ /* setup guard record structure */ >+ memset(rec, 0, sizeof(GuardRecord)); >+ rec->exit = exit; >+ /* copy side exit structure */ Ditto*2. > TraceRecorder::emitTreeCall(Fragment* inner, VMSideExit* exit) > { > TreeInfo* ti = (TreeInfo*)inner->vmprivate; > /* Invoke the inner tree. */ > LIns* args[] = { INS_CONSTPTR(inner), lirbuf->state }; /* reverse order */ > LIns* ret = lir->insCall(&js_CallTree_ci, args); > /* Read back all registers, in case the called tree changed any of them. */ Pre-existing * 2. >+ JS_ASSERT(!memchr(getGlobalTypeMap(exit), JSVAL_BOXED, exit->numGlobalSlots) && >+ !memchr(getStackTypeMap(exit), JSVAL_BOXED, exit->numStackSlots)); > import(ti, inner_sp_ins, exit->numStackSlots, exit->numGlobalSlots, > exit->calldepth, getFullTypeMap(exit)); > /* Restore sp and rp to their original values (we still have them in a register). */ or * 3. > if (callDepth > 0) { > lir->insStorei(lirbuf->sp, lirbuf->state, offsetof(InterpState, sp)); > lir->insStorei(lirbuf->rp, lirbuf->state, offsetof(InterpState, rp)); > } > /* Guard that we come out of the inner tree along the same side exit we came out when Hrm, 4. >+ LIns* exit = snapshot(exitType); > /* Guard array length */ >- LIns* exit = guard(true, >- lir->ins2(LIR_ult, idx_ins, stobj_get_fslot(obj_ins, JSSLOT_ARRAY_LENGTH)), >- exitType)->oprnd2(); >+ guard(true, >+ lir->ins2(LIR_ult, idx_ins, stobj_get_fslot(obj_ins, JSSLOT_ARRAY_LENGTH)), >+ exit); > /* dslots must not be NULL */ > guard(false, > lir->ins_eq0(dslots_ins), > exit); > /* Guard array capacity */ Wahhh. r=me with fixes. /be
This causes a slow-down in unpack-code. We don't trace that well anyway. This looks like a secondary effect. I will look into it separately. unpack-code: *1.069x as slow* 124.2ms +/- 0.6% 132.8ms +/- 0.6% significant
Without: whale:src gal$ ./time.sh t/string-unpack-code.js interp: 162 159 153 155 158 jit: 132 131 131 131 133 jit factor: 1.18 With: whale:src gal$ ./time.sh t/string-unpack-code.js interp: 166 166 165 166 162 jit: 136 137 139 137 135 jit factor: 1.20 The patch makes the _interpreter_ slower it seems. Some strange performance effect. We should dmandelin this.
Attached patch nits, refreshed to tip (obsolete) — — Splinter Review
Attachment #370993 - Attachment is obsolete: true
Patch failed try-server in talos (segfault).
We are failing in prototype.js: entering trace at file:///Users/gal/Desktop/cnn_files/prototype.js:31@8, native stack slots: 13 code: 0x1b623e1b stack: callee0=object<0x1b66cb98:Function> this0=object<0x1b541118:Function> argv0=object<0x1a9adaa0:DOMPrototype> argv1=object<0x1b61ba00:Object> vars0=string<0x1b66b830> stac k0=object<0x1b61bb20:Iterator> stack1=string<0x1b66b838> leaving trace at file:///Users/gal/Desktop/cnn_files/prototype.js:32@25, op=setelem, lr=0x6fdf9c, exitType=10, sp=7, calldepth=0, cycles=7288 callee0=object<0x1b66cb98:Function> this0=object<0x1b541118:Function> argv0=object<0x1a9adaa0:DOMPrototype> argv1=object<0x1b61ba00:Object> vars0=string<0x1b66b838> stack0=obje ct<0x1b61bb20:Iterator> stack1=string<0x1b66b838> stack2=object<0x1b66b838:om_qiE16nsQueryInterfaceRK4nsID> stack3=object<0x1b611b60:Function> stack4=object<0x1a9adaa0:DOMProto type> stack5=string<0x1b66b838> stack6=object<0x1b66b838:om_qiE16nsQueryInterfaceRK4nsID> boolean<2> Looking for compat peer 31@8, from 0x1d913b60 (ip: 0x1d509094) stack5 and stack6 are on exit both 0x1b66b838, but stack6 thinks its an object (its really a string).
The code in question: for (var property in source) { destination[property] = source[property]; }
Depends on: 486798
Attached patch refreshed (obsolete) — — Splinter Review
Attachment #370999 - Attachment is obsolete: true
I abhor this bug.
Attachment #371015 - Attachment is obsolete: true
(In reply to comment #59) > Created an attachment (id=371029) [details] > pick the proper stack slot to read the boxed value from in secondary traces > > I abhor this bug. var a = toString; var b = toSource; for (iters = 0; iters < 5000; ++iters) { c = "" + function(){} for (i = 0; i < c.length; ++i) {} delete toSource; toSource = b; delete toString; toString = a; } crashes debug js shell with -j at 0xcdcdcdcd at js_AddScopeProperty. === Exception Type: EXC_BAD_ACCESS (SIGSEGV) Exception Codes: KERN_INVALID_ADDRESS at 0x00000000cdcdcdcd Crashed Thread: 0 Thread 0 Crashed: 0 js-dbg-tm-intelmac 0x000f0865 js_AddScopeProperty + 1239 1 js-dbg-tm-intelmac 0x000a90c0 js_SetPropertyHelper + 1112 2 js-dbg-tm-intelmac 0x0007e5bc js_Interpret + 91816 3 js-dbg-tm-intelmac 0x000925f5 js_Execute + 807 4 js-dbg-tm-intelmac 0x0001db0c JS_ExecuteScript + 54 5 js-dbg-tm-intelmac 0x000081dc Process(JSContext*, JSObject*, char*, int) + 1402 6 js-dbg-tm-intelmac 0x0000966a ProcessArgs(JSContext*, JSObject*, char**, int) + 2276 7 js-dbg-tm-intelmac 0x0000aaf9 main + 897 (js.cpp:4737) 8 js-dbg-tm-intelmac 0x00001d1b _start + 209 9 js-dbg-tm-intelmac 0x00001c49 start + 41
Version: unspecified → Trunk
Attached patch v7261 (obsolete) — — Splinter Review
Attachment #371029 - Attachment is obsolete: true
#60 is not related to this bug. make check passes again. sent to the try server.
(In reply to comment #62) > #60 is not related to this bug. Spun off as bug 486812.
(In reply to comment #61) > Created an attachment (id=371030) [details] > v7261 for each (let y in [-1e81, -1e81, -1e81, function(){}, -1e81, -1e81, function(){}, 0, 0]) {} Assertion failure: ((jsval) obj & JSVAL_TAGMASK) == JSVAL_OBJECT, at ../jsapi.h:118 (Debug js shell with -j) I'm quite sure this assertion is caused by this patch now. :)
Attached patch latest — — Splinter Review
Attachment #371030 - Attachment is obsolete: true
#64 wfm with 'latest' applied
(In reply to comment #66) > #64 wfm with 'latest' applied Confirmed WFM.
Attached patch refreshed on top of upvar2 (obsolete) — — Splinter Review
Attachment #371035 - Attachment is obsolete: true
Attachment #371037 - Attachment is obsolete: true
Attachment #371035 - Attachment is obsolete: false
Attachment #371038 - Attachment is obsolete: true
Attachment #371035 - Flags: review?(brendan)
Comment on attachment 371035 [details] [diff] [review] latest Waldo, if you are around, this patch has some small fixes relative to the version brendan r+'ed.
Attachment #371035 - Flags: review?(jwalden+bmo)
Patch landed waiting minor final stamp by brendan. http://hg.mozilla.org/tracemonkey/rev/790c0eda3122
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/790c0eda3122
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
*ping* Still need a quick review
Attachment #371035 - Flags: review?(jwalden+bmo) → review+
Attachment #371035 - Flags: review?(brendan)
disable jitstats for math-trace-tests js1_8_1/trace/math-trace-tests.js http://hg.mozilla.org/tracemonkey/rev/208ec4c256ba
/cvsroot/mozilla/js/tests/js1_8_1/trace/math-trace-tests.js,v <-- math-trace-tests.js new revision: 1.2; previous revision: 1.1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: