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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 1

[Jim Blandy :jimb]

Attachment: Make CaptureCurrentStack accept an optional limit on the number of frames captured.

Comment 2

[Jim Blandy :jimb]

Try push:
https://tbpl.mozilla.org/?tree=Try&rev=4d3e19b66f1d

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 4

[Jim Blandy :jimb]

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.

Comment 5

[Shu-yu Guo [: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?

Comment 6

[Jim Blandy :jimb]

(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.

Comment 7

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b018836f449
https://hg.mozilla.org/integration/mozilla-inbound/rev/111548aad0c5

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/111548aad0c5
https://hg.mozilla.org/mozilla-central/rev/2b018836f449