Closed Bug 1426124 Opened 6 years ago Closed 6 years ago

filter out JIT frames from native stacks

Categories

(Core :: Gecko Profiler, enhancement, P1)

Product:
Core
Component:
Gecko Profiler
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: luke, Assigned: gregtatum)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1426124 - Discard JIT frames from native stacks in the profiler;
59 bytes, text/x-review-board-request
jandem
: review+
Details 
It's pretty normal when looking at a JS profile in perf.html to see a bunch of hex addresses interleaved with the JS function frames.  I think these are coming from the native stack walker reporting JIT frames.  The actual JS frames come from the JS::ProfilingFrameIterator and get merged with the native frames using the stack address to order.  But we're not throwing away the JIT frames found by the native stack walker, hence the interleaved JS->hex->JS->hex...

It seems like we should be able to pretty easily filter out these hex addresses while doing the merge, and save everyone a bunch of hassle.

Although the JS::ProfilingFrameIterator doesn't expose it, JitActivation contains a contiguous group of JIT stack frames and records the stack address when calling out of JIT code.  JitActivation could be easily extended to also record the entry stack address and thus provide an easy way to test whether a given native frame falls within a JitActivation.
(In reply to Luke Wagner [:luke] from comment #0)
> JitActivation could be easily
> extended to also record the entry stack address and thus provide an easy way
> to test whether a given native frame falls within a JitActivation.

We can probably use the JitActivation address for this, since it's stack allocated? I'd like to avoid adding more stuff to JitActivation because it affects performance of C++ -> JIT calls.
Good point, agreed.
We could filter them out if needed in perf.html using some kind of heuristic, it just depends on what kind of behavior is desired. Are they useful to have in profiles at all, or are they just noise? I could see having transform filter that is applied by default to profiles that removes all of them, then if you want to see the data you can pop off that filter. Then if you want to re-apply it you would right click on a JIT call node, and choose "Remove JIT frames".

perf.html: How to handle unsymbolicated JIT frames:
https://github.com/devtools-html/perf.html/issues/586

Bug 1407662: Provide information on JIT code region in profile

I'd like to have a good plan with some consensus on the best approach here.
(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #3)
> We could filter them out if needed in perf.html

I would prefer to get rid of them in the profiler core itself
(along the lines of Luke's proposal).  We already do a bunch of
stack processing (unwinding, merging) there and so that strikes me
as the right place to do it.  Then perf.html can continue to assume
that the stacks it gets from the profiler core don't need any further
cleaning up.

That's all on the assumption that the relevant frames are just noise
and play no useful ruole in the GUI.
Great, that sounds nice to me.
Agreed that they are just noise.
I agree.

We can still store the hex addresses on the JS stack frame, if we think it's useful information. For example, once perf.html can show assembly code for C++ / rust functions, we might want to capture the generated code for JS functions in the profile, and when the user selects such a jitted JS function, show the assembly code for it. If we do that, the execution address would be needed in order to highlight the right instructions.
Priority: -- → P1
Assignee: nobody → gtatum
I'm tentatively taking this one.
I attached a WIP patch. There are still unsymbolicated JIT frames in the profile, but many of them are now filtered. I'd like to investigate a bit more.

Before patch: https://perfht.ml/2JnzZG4
After patch: https://perfht.ml/2HrYMrU
Attachment #8970902 - Flags: review?(jdemooij)
:jandem I was chatting with some folks on #jsapi about this particular bug, and they suggested I ping you for review on it. In the previous comment I shared two profiles. I was able to remove many of the JIT frames using the stack boundaries of the JitActivations, but many are mysteriously still around. I tried looking into it a bit more, but haven't been able to figure out the particular reason for this.

At this point this patch is already creating more readable profiles, so I'd be ok moving forward with it, even if I am not completely able to remove all of the native JIT frames, and then work on follow-ups to remove all of them.
Comment on attachment 8970902 [details]
Bug 1426124 - Discard JIT frames from native stacks in the profiler;

https://reviewboard.mozilla.org/r/239652/#review247204

Very nice! This is a big improvement.

::: js/src/vm/Stack.cpp:1986
(Diff revision 1)
>          frame.kind = Frame_Wasm;
>          frame.stackAddress = stackAddr;
>          frame.returnAddress = nullptr;
>          frame.activation = activation_;
>          frame.label = nullptr;
> +        frame.endStackAddress = nullptr;

Here we should do the same as below I think.

::: js/src/vm/Stack.cpp:2013
(Diff revision 1)
>      frame.kind = entry->isBaseline() ? Frame_Baseline : Frame_Ion;
>      frame.stackAddress = stackAddr;
>      frame.returnAddress = returnAddr;
>      frame.activation = activation_;
>      frame.label = nullptr;
> +    frame.endStackAddress = activation_->asJit()->jsExitFP();

This will assert in debug builds when we have a JIT frame but then called into Wasm code (because we will have a wasmExitFp instead of a jsExitFP).

The simplest fix is probably to add a packedExitFP() accessor method to JitActivation (similar to jsExitFP but without the assert) and call that here and above.

There's also the following problem: if we return from C++ back to JIT code, we can have a stale exitFp value in the JitActivation (we don't reset it to nullptr). I think that's fine, though, because in that case there won't be any native frames to skip anyway.
Attachment #8970902 - Flags: review?(jdemooij) → review+
It looks like the remaining unsymbolicated frames are for:

* The entry frame when we go from C++ -> JIT: this one I'd fix in the profiler code by checking frame.activation (instead of frame.stackAddress of the first frame). The JitActivation is pushed on the stack right before we call into JIT code so it's a good marker.

* The exit frame when we go from JIT -> C++: this one I'm not sure about, maybe we can check |endStackAddress - X| or so instead of endStackAddress.
(In reply to Jan de Mooij [:jandem] from comment #12)
> ::: js/src/vm/Stack.cpp:2013
> (Diff revision 1)
> >      frame.kind = entry->isBaseline() ? Frame_Baseline : Frame_Ion;
> >      frame.stackAddress = stackAddr;
> >      frame.returnAddress = returnAddr;
> >      frame.activation = activation_;
> >      frame.label = nullptr;
> > +    frame.endStackAddress = activation_->asJit()->jsExitFP();
> 
> This will assert in debug builds when we have a JIT frame but then called
> into Wasm code (because we will have a wasmExitFp instead of a jsExitFP).
> 
> The simplest fix is probably to add a packedExitFP() accessor method to
> JitActivation (similar to jsExitFP but without the assert) and call that
> here and above.

Just as an FYI, depending on how endStackAddress is used: wasm frames have the lowest bit of packedExitFP_ set to 1, so an alternative is just to do something like:

frame.endStackAddress = activation_->asJit()->isJSJit() ? activation_->asJit()->jsExitFP() : activation_->asJit()->wasmExitFP();
Yeah, that works too. (I somehow thought jsExitFP() and wasmExitFP() asserted non-null-ness but they don't.)
:jandem I updated the patch to include the suggestion for the C++ -> JIT fix with the activation address. I'm going to file a follow-up for the JIT -> C++ work as I'm not sure what the `endStackAddress - X` should be, and I'd like to investigate more.

You already put an r+ on my first patch but I'd appreciate another look at the new changes. Also, do you have a recommendation for a try run for JS patches? This is my first patch in this part of the codebase. For the profiler I usually do:

try: -b do -p all -u xpcshell,gtest,mochitest-dt -t none
Flags: needinfo?(jdemooij)
Comment on attachment 8970902 [details]
Bug 1426124 - Discard JIT frames from native stacks in the profiler;

https://reviewboard.mozilla.org/r/239652/#review247846

This looks reasonable, but I'm not familiar with MergeStacks and not a peer of that code, so please request additional review from someone who is.
(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #17)
> Also, do you have a recommendation for a try run for JS
> patches? This is my first patch in this part of the codebase. For the
> profiler I usually do:
> 
> try: -b do -p all -u xpcshell,gtest,mochitest-dt -t none

When you touch js/src, js/public, etc, you should get Linux64 SpiderMonkey shell builds/tests triggered for your push and that's probably sufficient here. You could add the following, though: -u jsreftest,jittests
Flags: needinfo?(jdemooij)
Attachment #8970902 - Flags: review?(mstange)
Sorry, one thing I just realized: could you rename exitFP to jsOrWasmExitFP? That makes it a bit less likely for people to call it when they really want jsExitFP.
Flags: needinfo?(gtatum)
Comment on attachment 8970902 [details]
Bug 1426124 - Discard JIT frames from native stacks in the profiler;

https://reviewboard.mozilla.org/r/239652/#review247976

::: tools/profiler/core/platform.cpp:1024
(Diff revision 2)
> +        // If the latest JS frame was JIT, this could be a sampled JIT operation
> +        // masquerading as a native stack. In that case skip this frame as it would
> +        // never be symbolicated.

Let's reword this a little. The frame is not masquerading as anything - it is an actual native stack frame. How about this:

// If the latest JS frame was JIT, this could be the native frame that
// corresponds to it. In that case, skip the native frame, because there's
// no need for the same frame to be present twice in the stack. The JS
// frame can be considered the symbolicated version of the native frame.
Attachment #8970902 - Flags: review?(mstange) → review+
Blocks: 1460704
Flags: needinfo?(gtatum)
Status: NEW → ASSIGNED
The SpiderMonkey try run looked clean, checking the profiler tests to:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c582071af6f5ad506e5052da0f57f28cdf2abe6
Pushed by gtatum@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7f73d4e752f4
Discard JIT frames from native stacks in the profiler; r=jandem,mstange
https://hg.mozilla.org/mozilla-central/rev/7f73d4e752f4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Blocks: 1463559
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: