Closed Bug 1031143 Opened 10 years ago Closed 10 years ago

SavedStacks / JS::CaptureCurrentStack should take an optional depth limit

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: fitzgen, Assigned: jimb)

References

Details

Attachments

(1 file)

So that you can limit the number of frames from the stack you want to capture. If a limit n was provided, we would only capture the n youngest frames.
Blocks: 857648
Assignee: nobody → jimb
Status: NEW → ASSIGNED
Attachment #8448895 - Flags: review?(nfitzgerald)
Comment on attachment 8448895 [details] [diff] [review]
Make CaptureCurrentStack accept an optional limit on the number of frames captured.

Review of attachment 8448895 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, but I'm not a SpiderMonkey peer.
Attachment #8448895 - Flags: review?(nfitzgerald) → feedback+
Attachment #8448895 - Flags: review?(evilpies)
Comment on attachment 8448895 [details] [diff] [review]
Make CaptureCurrentStack accept an optional limit on the number of frames captured.

I think Tom is on vacation.
Attachment #8448895 - Flags: review?(evilpies) → review?(ejpbruel)
Attachment #8448895 - Flags: review?(ejpbruel) → review?(shu)
Comment on attachment 8448895 [details] [diff] [review]
Make CaptureCurrentStack accept an optional limit on the number of frames captured.

Review of attachment 8448895 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/TestingFunctions.cpp
@@ +885,5 @@
> +    if (args.length() >= 1) {
> +        double d;
> +        if (!ToNumber(cx, args[0], &d))
> +            return false;
> +        if (d < 0) {

This is just a testing function so it doesn't really matter, but a better API would be having Infinity meaning "no limit" than 0 meaning "no limit"

::: js/src/jit-test/tests/saved-stacks/max-frame-count.js
@@ +17,5 @@
> +assertEq(recur(0, 0).parent !== null, true);
> +assertEq(recur(0, 0).parent.parent, null);
> +assertEq(recur(1, 0).parent.parent.parent, null);
> +assertEq(recur(2, 0).parent.parent.parent.parent, null);
> +assertEq(recur(3, 0).parent.parent.parent.parent.parent, null);

Why are there so many parents here? What are the top 2? One of them I imagine is the toplevel, what's the one above that?
Attachment #8448895 - Flags: review?(shu) → review+
(In reply to Shu-yu Guo [:shu] from comment #5)
> Comment on attachment 8448895 [details] [diff] [review]
> Make CaptureCurrentStack accept an optional limit on the number of frames
> captured.
> 
> Review of attachment 8448895 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/builtin/TestingFunctions.cpp
> @@ +885,5 @@
> > +    if (args.length() >= 1) {
> > +        double d;
> > +        if (!ToNumber(cx, args[0], &d))
> > +            return false;
> > +        if (d < 0) {
> 
> This is just a testing function so it doesn't really matter, but a better
> API would be having Infinity meaning "no limit" than 0 meaning "no limit"

I agree. However, I'm nervous about the portability of <math.h>'s INFINITY constant. I want to land this promptly, so I'm going to leave it as-is.


> ::: js/src/jit-test/tests/saved-stacks/max-frame-count.js
> @@ +17,5 @@
> > +assertEq(recur(0, 0).parent !== null, true);
> > +assertEq(recur(0, 0).parent.parent, null);
> > +assertEq(recur(1, 0).parent.parent.parent, null);
> > +assertEq(recur(2, 0).parent.parent.parent.parent, null);
> > +assertEq(recur(3, 0).parent.parent.parent.parent.parent, null);
> 
> Why are there so many parents here? What are the top 2? One of them I
> imagine is the toplevel, what's the one above that?

In the 'recur(0, 0)' case, the two frames are the single call to recur, and the top-level script.

In the 'recur(3, 0)' case, we've got four frames for recur, and one frame for the top-level script.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b018836f449
https://hg.mozilla.org/integration/mozilla-inbound/rev/111548aad0c5
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: