Now that we have Baseline we spend a lot less time in the interpreter. Scripts are interpreted until the use count reaches 10 (used to be 40), and bailouts from Ion can directly resume into Baseline code (both JM+TI and Ion used to return to the interpreter on type changes etc). This means we should consider removing the property cache, simplifying the interpreter loop itself etc. Also, once we remove JM, none of the JITs use the JS stack and we should be able to simplify that too. For instance, right now a StackSegment contains both StackFrames and CallArgsLists, and the way frame iteration has to reconstruct the stack from that is pretty complicated and hard to debug. It would be great if we could clean this up.

Also, right now every IonActivation is tied to a StackFrame (ignoring FastInvoke for now). So when a native calls into JS, we have to push a StackFrame before we can enter the JITs. It would be nice if instead of this we could push a special mini-frame ("running something in Baseline/Ion") so that we don't need a full StackFrame. Luke, any thoughts?

Absolutely good idea. I had some ideas about doing this that I sent you in an email a couple months ago... trying to find that.

Ah, found it: "if js::StackFrame is really only used for the interpreter, we could make drastic simplifications to StackFrame and the rest of vm/Stack, perhaps even moving it entirely into jsinterp.cpp (i.e., vm/Interpreter) since it'd be an interpreter-internal detail. The only somewhat hot path is js::Invoke but, really, maybe we should stop with this scheme where the VM builds StackFrames before calling into the jits that immediately build a different stack frame. Perhaps instead we could change this scheme by having the callee (that is, js::Interpret, baseline/IM trampolines) build the callee's respective stack frame. I think this might be a major simplification." As for the "mini-frame" you mentioned, I suspect you're right that we'll want to push some sort of thing on some stack (probably the native stack) for call into jit code. However, we wouldn't have to view this as a frame but rather a different kind of thing (an "Activation") that doesn't have to masquerade as a stack frame to anyone. The activation can have a "kind" which is one of { Interp, Baseline, Ion, AsmJS } and probably a void* begin/end pair. Thus, we could think of any one JSRuntime's stack as being described by the regexp: (Activation ( InterpFrame | BaselineFrame | IonFrame | AsmJSFrame )* )* Also, we should totally rip out CallArgsList. I added it because the Debugger folks said they needed it "yesterday" so that the debugger stack walking could expose calls to builtins, but it is 2 years now and that hasn't happened, so let's take it out. Now, the annoying thing is preserving the ancient JSContext behavior (where each JSContext has its logically separate stack, even if they share the same StackSpace). Early on I didn't really understand any of this so I just preserved existing behavior. Nowadays, I think we could just a much simpler implementation: - each Activation records the JSContext* which pushed it - StackIter iterates over the entire JSRuntime stack but only shows the Activations for a given JSContext* - JS_SaveFrameChain/JS_RestoreFrameChain can just push an Activation with a flag set that says "stop here if you are iterating" (unless StackIter::GO_THROUGH_SAVED, of course) Any of that sound good to you?

(In reply to Luke Wagner [:luke] from comment #3) > Any of that sound good to you? Thanks, this all sounds fantastic! It would make StackIter a lot cleaner, among other things. One question: does this mean we can remove StackSegment? It looks like we can use the Activation list instead of prevInContext_/prevInMemory_. If we remove CallArgsList from StackSegment that leaves only FrameRegs*, and this can be part of the InterpreterActivation, right?

(In reply to Jan de Mooij [:jandem] from comment #4) > One question: does this mean we can remove StackSegment? It looks like we > can use the Activation list instead of prevInContext_/prevInMemory_. yep > If we remove CallArgsList from StackSegment that leaves only FrameRegs*, and this > can be part of the InterpreterActivation, right? Definitely. Ideally, js::Interpret would store the InterpreterActivation as a local variable which means that it could store FrameRegs by value and so we could avoid all this maddening FrameRegs copying without deoptimizing interp sp/pc access. On the subject of stack simplifications, we can also undo the formal/actual duplication trick and go back to having an argv* in the interp StackFrame.

Passes jit-tests with/without --ion-eager. This more or less implements the suggestion in comment 3: (*) Adds an Activation base class with subclasses InterpreterActivation (has a Vector of StackFrame's) and IonActivation. Making AsmJSActivation another subclass will be done as a follow-up I think. (*) Removes IonActivation::entryfp, Ion activations are no longer tied to a StackFrame. (*) Rewrites ScriptFrameIter to use the Activation list. (*) Removes AllFramesIter. ScriptFrameIter has an option to show frames from all contexts. This does not change the interpreter stack itself, but it's a first step in that direction. TODO: cleanup, implement JS_SaveFrameChain behavior (and write shell tests for this).

Cool! (Note: you should be able to test JS_SaveFrameChain via EvalInFrame in shell/js.cpp)

Green on Linux64 Try, still needs fuzzing. This is a first step towards removing/simplifying ContextStack/StackSpace. I also want to change the invoke paths to not push a StackFrame when we are entering the JITs, that should be a lot simpler now (see the FastInvoke changes) but can be done as follow-up work.

Comment on attachment 754802 [details] [diff] [review] Patch Review of attachment 754802 [details] [diff] [review]: ----------------------------------------------------------------- Overall looks great! I have some initial questions and some comments. ::: js/src/jscntxt.cpp @@ +1266,5 @@ > return false; > } > > + if (Activation *act = mainThread().activation) > + act->saveFrameChain(); After further refactoring, the whole stack.saveFrameChain() above will be able to go away, right? ::: js/src/jscntxt.h @@ +484,5 @@ > JSContext *ionJSContext; > uintptr_t ionStackLimit; > > + /* Points to the most recent activation running on the thread. */ > + js::Activation *activation; It'd be nice if you could have a "see comment in X" to refer the reader to what an activation really is. When the "VM stack layout" comment in Stack.h is updated, that would be good reference. ::: js/src/jscompartment.cpp @@ +605,2 @@ > return true; > + act = act->prev(); Perhaps use ActivationIterator? ::: js/src/jsinferinlines.h @@ +975,5 @@ > > /* static */ inline void > TypeScript::GetPcScript(JSContext *cx, JSScript **script, jsbytecode **pc) > { > + *script = cx->stack.currentScript(pc, ContextStack::ALLOW_CROSS_COMPARTMENT); I'd expect that we want the default of DONT_ALLOW_CROSS_COMPARTMENT here to avoid potential cross-compartment violations in callers. Are there tests that fail if you do that? ::: js/src/jsinterp.cpp @@ +1113,5 @@ > if (!entryFrame) > entryFrame = regs.fp(); > > + InterpreterActivation activation(cx, regs); > + if (!activation.pushFrame(entryFrame)) If pushFrame stops doing an append() (per other comment) and we want to enforce the invariant that InterpreterActivations are never empty, you could change the InterpreterActivation constructor to take the entryFrame. ::: js/src/jsinterpinlines.h @@ +737,5 @@ > HandleValue rref, MutableHandleValue res) > { > do { > // Don't call GetPcScript (needed for analysis) from inside Ion since it's expensive. > + bool analyze = cx->mainThread().activation->isInterpreter(); I'd like to avoid scattering a bunch of uses of .activation around and instead add more targeted queries to JSContext like cx->currentlyRunningInInterpreter()/cx->currentlyRunningInJit(). Ideally, I think, JSContext::activation would be private (and named activation_). ::: js/src/vm/Stack.cpp @@ +939,2 @@ > JS_ASSERT_IF(evalInFrame.isStackFrame(), !evalInFrame.asStackFrame()->runningInIon()); > + prevLink = NULL; nice @@ +1136,4 @@ > } > > + JS_ASSERT(!data_.interpFrames_.frame()->runningInIon()); > + data_.pc_ = interpAct->regs().pc; If there are multiple runninInIon frames that are popped, won't interpAct->regs().pc be wrong for interpFrames_.frame()? @@ +1162,2 @@ > > + JS_NOT_REACHED("Invalid activation"); To regularize the control flow (not have a loop end with JS_NOT_REACHED()), I wouldn't mind making moving the isInterpreter() case be at the end (w/ JS_ASSERT(activation->isInterpreter())). ::: js/src/vm/Stack.h @@ +108,5 @@ > * StackSpace, since they are adjacent on the thread's stack, but not from the > + * perspective of cx1 and cx2. Each independent stack is encapsulated and > + * managed by the js::ContextStack object stored in JSContext. ContextStack > + * is the primary interface to the rest of the engine for pushing and popping > + * the stack. Can StackSegment be removed altogether now? Before you're done with the whole refactoring, it seems like the "VM stack layout" comment at the top of this file will need a big update. @@ +980,1 @@ > bool runningInIon() const { It'd be nice if we could regularly use "Jit" everywhere we mean "baseline or Ion" @@ +1510,5 @@ > + // iterating when it reaches this activation (if GO_THROUGH_SAVED is not > + // set). > + size_t savedFrameChain_; > + > + enum Kind { Interpreter, Ion, AsmJS }; Perhaps s/Ion/Jit/ here and below? (AsmJS is generated code, but it is AOT ;) @@ +1565,5 @@ > +class InterpreterActivation : public Activation > +{ > + friend class js::InterpreterFrameIterator; > + > + Vector<StackFrame *, 8, SystemAllocPolicy> frames_; Why does InterpreterActivation need to save the whole Vector of frames? It seems like it could maintain an entry/current StackFrame* pair (where current was updated by pushFrame) and use fp->prev() to traverse? @@ +1610,5 @@ > + uint8_t *ionTop() const { > + JS_ASSERT(activation_->isIon()); > + return ionTop_; > + } > + bool more() const; For consistency with ScriptFrameIter, how about "done()"? @@ +1741,5 @@ > #endif > > + Data(JSContext *cx, PerThreadData *perThread, SavedOption savedOption, > + ContextOption contextOption); > + //Data(JSContext *cx, JSRuntime *rt, StackSegment *seg); //

Thanks for the comments, I will post an updated patch to address them. (In reply to Luke Wagner [:luke] from comment #9) > After further refactoring, the whole stack.saveFrameChain() above will be > able to go away, right? Yes :) > It'd be nice if you could have a "see comment in X" to refer the reader to > what an activation really is. When the "VM stack layout" comment in Stack.h > is updated, that would be good reference. Good idea, I will add some comments. > I'd like to avoid scattering a bunch of uses of .activation around and > instead add more targeted queries to JSContext like > cx->currentlyRunningInInterpreter()/cx->currentlyRunningInJit(). Ideally, I > think, JSContext::activation would be private (and named activation_). OK, makes sense. > If there are multiple runninInIon frames that are popped, won't > interpAct->regs().pc be wrong for interpFrames_.frame()? You are right, good catch. > Can StackSegment be removed altogether now? Yeah, I wanted to do that in a follow-up bug to not make this patch larger than necessary. We can start moving pieces like the GC marking code etc over to the activation list and then see what's left. I also hope we will be able to remove ContextStack, but I have to look into that more. > It'd be nice if we could regularly use "Jit" everywhere we mean "baseline or > Ion" Agreed, I wanted to do that in ion/ as well (IonFrames -> JitFrames etc), might as well start doing that here.. > Why does InterpreterActivation need to save the whole Vector of frames? It > seems like it could maintain an entry/current StackFrame* pair (where > current was updated by pushFrame) and use fp->prev() to traverse? For eval-in-frame, fp->prev() used to link to an arbitrary frame on the stack and complicate things. However, now we set fp->prev() to NULL for e-i-f frames these frames should always be the first frame in an activation.. So I think using fp->prev() will work now. Eventually I hope we can make fp->prev() NULL for all entry frames, so that we only have to store a pointer to the the top-most frame, but before we can do that I'd like to get rid of all other uses of fp->prev().

Addresses all comments so far. I started renaming Ion -> Jit in all code related to activations/ScriptFrameIter. I'm not yet doing that for IonFrame* though, it would make the patch a lot bigger and is better done as a follow-up I think. The InterpreterFrameIterator now also tracks the pc, to avoid the problem you found when we skip "runningInJit" StackFrame's.

> This means we should consider removing the property cache Bug 704356 has an old patch to do this. My measurements showed that slowdowns were small but enough that I was uncomfortable removing it (though I did make it much smaller; memory consumption was my motivation). But maybe things will be better now.

Comment on attachment 756003 [details] [diff] [review] Patch v2 Nice, sorry for the delay.

Rebased, applies to inbound tip (407dd32db3b0) decoder, gkw: Can I please get some fuzzing of this patch? It's a big patch that rewrites some pretty complicated parts of the engine.

Comment on attachment 760802 [details] [diff] [review] Rebased Browser build is stable so I decided to push this now to avoid more rebase pain.

Follow-up to disable PGO for all ScriptFrameIter methods to fix Windows PGO builds: https://hg.mozilla.org/integration/mozilla-inbound/rev/bcb4e711f4eb

https://hg.mozilla.org/mozilla-central/rev/554597fd45e9 https://hg.mozilla.org/mozilla-central/rev/bcb4e711f4eb