1026188 - Allow embedders to save JS execution stacks

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Devtools is finding more and more places where we want a full JS stack at a specific event and it would be awesome if firefox could just use js::SavedStacks.

For one example, see bug 1019721 where we want to capture a stack for every promise creation and fulfillment.

Comment 1

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

Attachment: save-stack-jsapi.patch

Very small patch, but adds a new jsapi method.

Where/how should I test this?

Comment 2

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

Attachment: save-stack-jsapi.patch

This new version makes the SaveStack testing function use the new JS::CaptureCurrentStack function, so now all the tests in js/src/jit-test/tests/saved-stacks are exercising this new JSAPI function.

Comment 3

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

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

Comment 4

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

:bz, does this API provide enough for you to use in bug 966471?

Comment 5

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

Attachment: save-stack-jsapi.patch

Sorry for the re-review noise. This should fix the failures in the last try push.

New try push: https://tbpl.mozilla.org/?tree=Try&rev=9898bf9e7211

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yes, this should be fine.  I'll try doing some performance measurements too, just to see.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I guess one annoyance is we'll need a JSContext in every method that creates promises.  I suppose we could have bindings do this automatically for a promise-returning method.

So I measured the performance here.  On my brand-new laptop hardware, a CaptureCurrentStack() call when there is only one frame on the stack takes about 0.8us (basically doubling the cost of creating and resolving a Promise object, which already seems way too high to me; note that I only captured a single stack, not separate ones for creating and resolving).

This is without the patch for bug 1004110, but PCToLineNumber is only about 20% of CaptureCurrentStack in this case.

If I change my testcase to have 10-deep function nesting, CaptureCurrentStack takes about 3.5us.

I guess as long as we only do this when devtools are open it'll be ok.

For what it's worth, for the 10-deep nesting the time under CaptureCurrentStack() breaks down as follows:

10% self time in SavedStacks::insertFrames (largely call overhead, I bet).
30% atomizing the filename.  Can we make this lazy?  Lazily doing that for
    JS::StackDescription was a pretty big performance win.
25% under getOrCreateSavedFrame, mostly self time in HashPolicy::hash and in the
    hashtable's lookup() method.
11% InlineFrameIterator constructors.
10% PCToLineNumber
 8% ScriptFrameIter constructor.
 4% JSScript::scriptSource(), mostly calling UncheckedUnwrap (why??)

Comment 8

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

(In reply to Boris Zbarsky [:bz] from comment #7)
> I guess one annoyance is we'll need a JSContext in every method that creates
> promises.  I suppose we could have bindings do this automatically for a
> promise-returning method.
> 
> So I measured the performance here.  On my brand-new laptop hardware, a
> CaptureCurrentStack() call when there is only one frame on the stack takes
> about 0.8us (basically doubling the cost of creating and resolving a Promise
> object, which already seems way too high to me; note that I only captured a
> single stack, not separate ones for creating and resolving).
> 
> This is without the patch for bug 1004110, but PCToLineNumber is only about
> 20% of CaptureCurrentStack in this case.

The main use case I was optimizing in bug 1004110 was for our allocation site tracking which tends to capture the same stack many times. My measurements showed a 2.6x improvement with that patch running jimb's benchmark-debugger with tracking allocation sites enabled.

In this case, I think it will help less, and atomizing and calling PCToLineNumber lazily is the way to go. The catch is that we lose the perfect older frames tail-sharing that we have now. Actually, I think I have an idea of how we could fix that. More details in bug 1027305.

> 
> If I change my testcase to have 10-deep function nesting,
> CaptureCurrentStack takes about 3.5us.
> 
> I guess as long as we only do this when devtools are open it'll be ok.

Yeah, I think this is probably negligible compared to enabling debug mode and getting kicked out of ion back to baseline.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> which tends to capture the same stack many times.

Right, I was capturing exactly the same stack each time in my test here.

Comment 10

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

Comment on attachment 8441767 [details] [diff] [review]
save-stack-jsapi.patch

Switching reviewers because I can tell how busy Jason is just via the bugmail I'm getting about Symbols :P

:sfink, this should be a quick and easy review (I hope!) :)

Comment 11

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8441767 [details] [diff] [review]
save-stack-jsapi.patch

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

::: js/src/jsapi.cpp
@@ +6492,5 @@
>      rt->oomCallback = cb;
>      rt->oomCallbackData = data;
>  }
> +JS_PUBLIC_API(bool)
> +JS::CaptureCurrentStack(JSContext *cx, JS::MutableHandle<JSObject*> stackp)

MutableHandleObject again.

::: js/src/jsapi.h
@@ +5066,5 @@
>  (* OutOfMemoryCallback)(JSContext *cx, void *data);
>  extern JS_PUBLIC_API(void)
>  SetOutOfMemoryCallback(JSRuntime *rt, OutOfMemoryCallback cb, void *data);
> +extern JS_PUBLIC_API(bool)
> +CaptureCurrentStack(JSContext *cx, MutableHandle<JSObject*> stackp);

Ugh, I see it's a complete mishmash right now, but in jsapi.h the preferred style is MutableHandleObject in place of MutableHandle<JSObject*>.

Comment 12

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

Attachment: save-stack-jsapi.patch

With requested change.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f10a0e42e89

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7f10a0e42e89