Closed
Bug 579183
Opened 14 years ago
Closed 14 years ago
loosen-up StackSegment invariants
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: luke, Assigned: luke)
References
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(5 files, 5 obsolete files)
4.22 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
7.17 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
38.21 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
48.92 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
37.57 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
(As a syntactic first patch, I would like to rename CallStack to CallSegment because CallStack is a poor and confusing name. I have received this feedback from several people and personally agree.) The goal of the bug is to push CallSegments less frequently. At the moment, there is an invariant which isn't really necessary that each JS engine activation/reentrance has its own CallSegment. This means a CallSegment is pushed for every js::Invoke that (sometimes) goes between the arguments and the stackframe. This gets in the way of bug 539144, which wants argv[0] to be a (jit-time) constant offset from fp. Fixing this should also give a small speedup in js::Invoke which is showing up in shark on benchmarks.
Comment 1•14 years ago
|
||
CallSegment sounds like a segment of a call -- do you not want CallStackSegment? Or something shorter (CallFlow?). /be
![]() |
Assignee | |
Comment 2•14 years ago
|
||
CallStackSegment is what I originally had in mind... CallSegment was the abbreviation :P
Comment 3•14 years ago
|
||
That's like abbreviating "mailman" to "man" (excuse my gender-generic usage) :-P^2. /be
![]() |
Assignee | |
Comment 4•14 years ago
|
||
Fair enough, CallStackSegment it is. But now I have to s/cs/css/ all those variable names ;-)
![]() |
Assignee | |
Comment 5•14 years ago
|
||
s/CallStack/CallStackSegment/ s/callstack/segment/ (in comments) s/cs/css/ (variable names) s/previousInThread/previousInMemory/ (based feedback that "thread" made no sense)
Attachment #459514 -
Flags: review?(dvander)
![]() |
||
Updated•14 years ago
|
Attachment #459514 -
Flags: review?(dvander) → review+
![]() |
Assignee | |
Comment 6•14 years ago
|
||
This passes trace-tests (but probably breaking a lame decompiler assumption in reftests). By loosening the invariants I can avoid pushing segments for js::Invoke and do some inlining of StackSpace members called from Invoke. On TM, this gives a .6% speedup on V8 (30ms) and .9% speedup on SS (5ms).
Attachment #459514 -
Attachment is obsolete: true
![]() |
Assignee | |
Comment 7•14 years ago
|
||
Forgot to mention in comment 6, but after several bits of feedback I shortened CallStackSegment to StackSegment in the patch. I thought this would be ambiguous-sounding, but others don't think so and it pairs well with js::StackSpace. As predicted, bumping the down-frame's sp messes up the decompiler. In theory, this is solved by a simple test to see whether the given vp is above the simulated stack depth found by ReconstructPCStack. The problem is that the vp gets turned into spindex at a point where the simulated stack info is not available and, when this info is available, the vp is gone. I think the solution is to take out spindex and instead pass a Value* + a separate enum param to indicate JSDVG_{IGNORE,SEARCH}_STACK.
![]() |
Assignee | |
Comment 8•14 years ago
|
||
This small patch is a prerequisite for the one that pushes less segments. It teaches the decompiler/js_ReportIsNotFunction not to be fooled by a bumped regs.sp. Comments in the patch explain.
Attachment #460104 -
Attachment is obsolete: true
Attachment #460220 -
Flags: review?(brendan)
![]() |
Assignee | |
Comment 9•14 years ago
|
||
Almost ready for review.
![]() |
Assignee | |
Comment 10•14 years ago
|
||
I forgot to mention: I started doing what bug 580337 is talking about by introducing a NativeArgs structure that is used to build the arguments to invoke. I think the resulting code is much more readable.
Comment 11•14 years ago
|
||
Comment on attachment 460220 [details] [diff] [review] decompiler changes I use "invariant" too, it sounds Cool and Important, but this bug reminds me of http://www.cs.yale.edu/quotes.html #1 and #62, which are so true. >diff --git a/js/src/jsfun.cpp b/js/src/jsfun.cpp >--- a/js/src/jsfun.cpp >+++ b/js/src/jsfun.cpp >@@ -2557,24 +2557,43 @@ js_ValueToCallableObject(JSContext *cx, > > void > js_ReportIsNotFunction(JSContext *cx, const Value *vp, uintN flags) > { > const char *name = NULL, *source = NULL; > AutoValueRooter tvr(cx); > uintN error = (flags & JSV2F_CONSTRUCT) ? JSMSG_NOT_CONSTRUCTOR : JSMSG_NOT_FUNCTION; > LeaveTrace(cx); >+ >+ /* >+ * We try to the print the code that produced vp if vp is a value in the "that produced *vp if vp points to a value on the" This patch looks fine otherwise, but can you add a test or two, in particular showing the fallback dvg result? /be
Attachment #460220 -
Flags: review?(brendan) → review+
![]() |
Assignee | |
Comment 12•14 years ago
|
||
(In reply to comment #11) > I use "invariant" too, it sounds Cool and Important, but this bug reminds me of > http://www.cs.yale.edu/quotes.html #1 and #62, which are so true. Haha, nice. > This patch looks fine otherwise, but can you add a test or two, in particular > showing the fallback dvg result? Does it suffice that 6 decompiler reftests fail with the segment-patch applied without this one?
Comment 13•14 years ago
|
||
(In reply to comment #12) > > This patch looks fine otherwise, but can you add a test or two, in particular > > showing the fallback dvg result? > > Does it suffice that 6 decompiler reftests fail with the segment-patch applied > without this one? Probably -- I'm really asking for an example where the diagnostic degrades to the fallback string, and what it was like in (say) Firefox 3.6. /be
![]() |
Assignee | |
Comment 14•14 years ago
|
||
Ah, right. IIUC, this patch shouldn't affect any error messages now. With only the segment patch applied, some values would incorrectly take the "have a pc" path, instead of the fallback-string path. An example from js1_6/decompilation/regress-352613-01.js is: "a".replace(/a/, Array.prototype.map) which should print "a" is not a function but prints "a".replace is not a function without the decompiler patch.
![]() |
Assignee | |
Comment 15•14 years ago
|
||
I removed the invokeFriendAPI from StackSpace and just had CallMethod go through JS_CallFunction like everybody else.
Attachment #460425 -
Flags: review?(mrbkap)
![]() |
Assignee | |
Comment 16•14 years ago
|
||
Finally ready for review.
Attachment #460221 -
Attachment is obsolete: true
Attachment #460426 -
Flags: review?(jwalden+bmo)
![]() |
Assignee | |
Comment 17•14 years ago
|
||
Comment on attachment 460426 [details] [diff] [review] stack segment changes Oops, forgot the important case of fast-native called from JSAPI. That breaks the fast native's assumption that it is called "from the engine", i.e., from an active frame.
Attachment #460426 -
Flags: review?(jwalden+bmo)
Updated•14 years ago
|
Attachment #460425 -
Flags: review?(mrbkap) → review+
![]() |
Assignee | |
Comment 18•14 years ago
|
||
This patch just renames things as discussed above.
Attachment #460426 -
Attachment is obsolete: true
Attachment #461366 -
Flags: review?(jwalden+bmo)
![]() |
Assignee | |
Comment 19•14 years ago
|
||
The previous approach (bumping regs->sp) breaks down when fast natives are called from js::Invoke. Fixing that adds more complexity. This patch uses a simpler strategy that achieves the same segment-elision for js::Invoke calls from an active segment. I also noticed that FrameRegsIter showed up in callgrind (from its one hot callsite in the tracer's LeaveTree). Inlining the 99% case brings the total speedup to
Attachment #461370 -
Flags: review?(jwalden+bmo)
![]() |
Assignee | |
Comment 20•14 years ago
|
||
(something bizarre happened while I was switching between tabs) ... 7.3ms (1.4%) on SS and 17ms (2.9%) on v8-deltablue.
![]() |
Assignee | |
Comment 21•14 years ago
|
||
Updated to work with the next patch.
Attachment #461370 -
Attachment is obsolete: true
Attachment #461410 -
Flags: review?(jwalden+bmo)
Attachment #461370 -
Flags: review?(jwalden+bmo)
![]() |
Assignee | |
Comment 22•14 years ago
|
||
This patch adds the vp[0]/vp[1]/vp[2] use in lieu of args.callee()/args.thisv()/args[0]. While updating the code to use this, I removed a few now-unnecessary auto rooters and also was able to simplify the case of Invoke called from the interpreter. SS now shows a 9ms (1.7%) speedup.
Attachment #461412 -
Flags: review?(jwalden+bmo)
Updated•14 years ago
|
Attachment #461366 -
Flags: review?(jwalden+bmo) → review+
Comment 23•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/b1b935db8b1a
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 24•14 years ago
|
||
Comment on attachment 461410 [details] [diff] [review] stack segment changes v.3 >+JS_REQUIRES_STACK >+ExecuteFrameGuard::~ExecuteFrameGuard() >+{ >+ if (!cx) >+ return; if (!pushed()) Do this in the other frame guard destructors, too (preserving likelihood hints). (I must be missing something if this is all I have! ;-) )
Attachment #461410 -
Flags: review?(jwalden+bmo) → review+
Comment 25•14 years ago
|
||
Comment on attachment 461412 [details] [diff] [review] CallArgs >diff --git a/js/src/jsarray.cpp b/js/src/jsarray.cpp >+ args.callee() = calleev; >+ args.thisv() = thisv; >+ Value *argv = args.argv(); > if (REDUCE_MODE(mode)) >- *sp++ = *vp; >- *sp++ = tvr.value(); >- sp++->setInt32(i); >- *sp++ = objv; >+ *argv++ = *vp; >+ argv[0] = tvr.value(); >+ argv[1].setInt32(i); >+ argv[2] = objv; Assigning to "argv" (psych!) is quite confusing to read. This seems much preferable to me: args.callee() = calleev; args.thisv() = thisv; Value *argv = args.argv(); size_t j = 0; if (REDUCE_MODE(mode)) argv[j++] = *vp; argv[j++] = tvr.value(); argv[j++].setInt32(i); argv[j++] = objv; >diff --git a/js/src/jscntxt.h b/js/src/jscntxt.h >+/* >+ * This type can be used to call Invoke when the arguments have already been >+ * pushed onto the stack as part of normal exeuction. >+ */ "execution" >diff --git a/js/src/jsinterp.h b/js/src/jsinterp.h > /* >+ * Abstracts the layout of the stack passed to natives from the engine and from >+ * natives to js::Invoke. >+ */ >+struct CallArgs >+{ >+ Value *argv_; >+ uintN argc_; >+ protected: >+ CallArgs() {} >+ CallArgs(Value *argv, uintN argc) : argv_(argv), argc_(argc) {} >+ public: >+ Value *base() const { return argv_ - 2; } >+ Value &callee() const { return argv_[-2]; } >+ Value &thisv() const { return argv_[-1]; } >+ Value &operator[](unsigned i) const { return argv_[i]; } Assert in-range. We're not horribly far off from being able to assert in-rangeness for all argument value gets/sets (possibly with a DEBUG-asserting iterator for the case or two where we bump along a pointer writing to arguments). But it's not important to get that done here and now.
Attachment #461412 -
Flags: review?(jwalden+bmo) → review+
Comment 26•14 years ago
|
||
(In reply to comment #25) > We're not horribly far off from being able to assert in-rangeness for all > argument value gets/sets (possibly with a DEBUG-asserting iterator for the case > or two where we bump along a pointer writing to arguments). A good goal. > But it's not important to get that done here and now. File a bug and someone own it -- we should drive across the goal line for a TD. FIXME citation if appropriate. /be
![]() |
Assignee | |
Updated•14 years ago
|
Summary: loosen-up callsegment invariants → loosen-up StackSegment invariants
![]() |
Assignee | |
Comment 27•14 years ago
|
||
Thanks for all the reviews! http://hg.mozilla.org/tracemonkey/rev/7acc0fe02f45
Whiteboard: fixed-in-tracemonkey
Comment 28•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/b16d966d3b6f
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•