Allow embedders to save JS execution stacks

RESOLVED FIXED in mozilla33

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

(Blocks: 1 bug)

unspecified
mozilla33
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
Assignee: nobody → nfitzgerald
Created attachment 8441573 [details] [diff] [review]
save-stack-jsapi.patch

Very small patch, but adds a new jsapi method.

Where/how should I test this?
Attachment #8441573 - Flags: review?(jorendorff)
Created attachment 8441661 [details] [diff] [review]
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.
Attachment #8441573 - Attachment is obsolete: true
Attachment #8441573 - Flags: review?(jorendorff)
Attachment #8441661 - Flags: review?(jorendorff)
Try push: https://tbpl.mozilla.org/?tree=Try&rev=dac67b7c2bbc
:bz, does this API provide enough for you to use in bug 966471?
Flags: needinfo?(bzbarsky)
Created attachment 8441767 [details] [diff] [review]
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
Attachment #8441661 - Attachment is obsolete: true
Attachment #8441661 - Flags: review?(jorendorff)
Attachment #8441767 - Flags: review?(jorendorff)
Yes, this should be fine.  I'll try doing some performance measurements too, just to see.
Flags: needinfo?(bzbarsky)
Flags: needinfo?(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??)
Flags: needinfo?(bzbarsky) → needinfo?(nfitzgerald)
(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.
Flags: needinfo?(nfitzgerald)
> which tends to capture the same stack many times.

Right, I was capturing exactly the same stack each time in my test here.
Status: NEW → ASSIGNED
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!) :)
Attachment #8441767 - Flags: review?(jorendorff) → review?(sphink)
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*>.
Attachment #8441767 - Flags: review?(sphink) → review+
Created attachment 8446070 [details] [diff] [review]
save-stack-jsapi.patch

With requested change.
Attachment #8441767 - Attachment is obsolete: true
Attachment #8446070 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f10a0e42e89
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7f10a0e42e89
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.