827258 - Convert jsdbg2 to use StackIter

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Like IonMonkey, the baseline compiler won't use StackFrames. So we have to refactor the debugger(s) to use StackIter and/or ScriptFrameIter instead.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

Luke, this is mostly what we discussed before, so I'm asking you to review. Main changes:

* Adds a TaggedFramePtr class that either points to a StackFrame or a baseline frame. We use this class as key in the hash table, and we can also use it for JSAPI/JSD (make JSStackFrame* a TaggedFramePtr).

* Change jsdbg2 to use ScriptFrameIter instead of StackFrame*. I tried to store an heap-allocated iterator in Debugger.Frame, but that was a bit complicated (StackIter has a RootedScript field, Debugger.Frame would need a JSClass and finalizer, StackIter::pc_ was not updated). So this patch stores a TaggedFramePtr and constructs an iterator from that.

Still to do in follow-up patches:

* Convert GetDebugScopeForFrame
* Eval-in-frame changes
* Make AllFramesIter iterate over Ion frames

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Patch

Comment 3

[Luke Wagner [:luke]]

Most of the patch looks perfect; only one real issue: this patch makes all the frame accessors O(n).  Stacks can get really deep (50K) which will lead to O(n^2) behavior if the debugger, say, iterates with frame.older.  We've had these O(n^2) stack iterations bite us quite a few times in the past.

Now, the only time when we *need* to do this O(n) search is getNewestFrame, and in most cases, it's usually just the top frame, so no problem.

For the other cases, the problem is just, as you pointed out, that we can't store a StackIter in the heap.  Looking into that, I think the only problem is the RootedScript; is that right?  Assuming yes, in the non-Ion case, the script is trivially re-materialized from fp->script or baseline-frame->script (I assume).  So, instead, could we make some struct StackIterState that can be extracted from a non-ion StackIter and used to construct a StackIter and store StackIterState in the heap?  This StackIterState would have a copy of everything but the RootedScript.  (To avoid bugs where one adds a field to StackIter and forgets about StackIterState, perhaps you could just make StackIter store the StackIterState as a member variable, alongside script_.)

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch v2

Thanks, good point about the quadratic behavior.

We can remove StackIter::script_, but ionInlineFrames_ has the same problem (it holds the script and callee). So this patch adds a StackIter::Data struct containing everything except ionInlineFrames_, and stores a pointer to that structure inside Debugger.Frame.

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 700439 [details] [diff] [review]
Patch v2

Very nice

Comment 6

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7c4a6d06571

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e7c4a6d06571