1441689 - When collecting stacks for the profiler, include line + column information for JIT frames

Status: NEW

(Core :: JavaScript Engine, enhancement, P3)

Description

[Markus Stange [:mstange]]

It would be nice to know which line of a JS function was executing at any given time. We have this information for JS functions that are running in the interpreter, but we don't have it for JIT frames.

At the moment, for JIT frames, we only know at which line the current JS function starts, but not which line is actually executing.

Comment 1

[Markus Stange [:mstange]]

Attachment: Bug 1441689 - Add ProfiledFrameHandle::lookupScriptAndPc.

Review commit: https://reviewboard.mozilla.org/r/223616/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/223616/

Comment 2

[Markus Stange [:mstange]]

Attachment: Bug 1441689 - Add line numbers for all JIT frames. Causes crashes, unfortunately.

The crashes happened when ProfiledFrameHandle::lookupScriptAndPc was calling
BaselineScript::approximatePcForNativeAddress on a null baselineScript.
It looks like something has nulled out the JSScript's baselineScript.

Review commit: https://reviewboard.mozilla.org/r/223618/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/223618/

Comment 3

[Nicolas B. Pierron [:nbp]]

One of the problem I see is that we are doing code relocation with LICM, or share pieces of code with GVN, but we should have a non-precise form of this information at least stored in snapshots.

Comment 4

[Markus Stange [:mstange]]

The Firefox profiler is now adding a built-in source view, and will display this type of information for C++ code. It would be great to have the same information for JavaScript code.

Comment 5

[Nicolas B. Pierron [:nbp]]

Thanks Markus for the link with C++ / Rust examples, these are awesome!

One thing I noticed with the crashing patch, is that the RootedScript is assumed to be non-null. While this might be frequently the case this is not guaranteed, for multiple reasons:

• We have trampoline functions, which are C++/JIT calls wrapper to convert from the system ABI to an ABI which makes things easy for the GC.
• We have shared Inline Caches, which might not be associated with a script.
• We have WebAssembly, which does not use JSScript.

I think the JS team can find someone to help design a proper API which can be used by the Gecko Profiler.
I will raise this topic in our next SpiderMonkey meeting.

Comment 6

[Markus Stange [:mstange]]

Thanks, Nicolas!

Comment 7

[Nicolas B. Pierron [:nbp]]

The lasted patches bit-rot since the last time. However, I have a few comments to revive the current set of patches.

The current patches are adding ProfiledFrameHandle::lookupScriptAndPc, or renaming an existing function to this new name. Since the function got removed,as some clean-up went by. This is not a big deal, the js::jit::JitcodeGlobalEntry still has the information provided by js::jit::JitcodeGlobalEntry::callStackAtAddr. The second patch adds a way to query the ProfiledFrameHandle using lookupScriptAndPc, but this is no longer an option, as the function got removed.

One thing to note is how the ProfiledFrameEntry aJITFrame flows into StreamJITFrame. StreamJITFrame is called by JSONForJITFrame and JitFrameInfo::AddInfoForRange, which iterates over the ProfiledFrameEntry. As a result of iterating over the ProfiledFrameRange returned by JS::GetProfiledFrames.

The remarkable point, is that JS::GetProfiledFrames calls the other overload of js::jit::JitcodeGlobalEntry::callStackAtAddr.

callStackAtAddr has 2 definitions:

• A first definition with an out-param to return the BytecodeLocationVector, which is what we are interested in to get the line information.
• A second definition with an out-param to return the names of the functions, which is what JS::GetProfiledFrames returns.

To rebase the patches, a simple fix might be to change the ProfiledFrameRange to hold a BytecodeLocationVector, as well as adding a BytecodeLocation field to the ProfiledFrameEntry.

Then we should be able to query the line number in StreamJITFrame using the aJITFrame argument.

This does not tell us whether the same crash will occur or not, but this should reduce the risk as we will no longer be doing an extra lookup.
Later on, we might want to merge both callStackAtAddr with 2 out-param.

Comment 8

[Nicolas B. Pierron [:nbp]]

One thing to note, is that the BytecodeLocation carry some information which might be useful for JS Engine developers, which is the opcode which is executed. While most of these would be calls, it might be interesting to distinguish the opcode in the profiler.

Comment 9

[Markus Stange [:mstange]]

This all sounds great!

Comment 10

[Nicolas B. Pierron [:nbp]]

Attachment: Bug 1441689 - Instrument ProfiledFrameHandle to fetch line number information. r=

This patch changes the function callStackAtAddr to output both the Bytecode
location as well as the associated Script pointer. Unfortunately, we cannot use
the BytecodeLocation abstraction as it is link to too many other parts of the
JavaScript engine that we prefer not to expose.

With this modification, we are now able to annotate JIT frame within the
profiler, with line number information. The line number information can later be
used by the profiler interface to display the associated code.

Comment 11

[Nicolas B. Pierron [:nbp]]

I had local failures locally which did not seem related to the profiler. Thus, I pushed the new patch to try.

There are failures which are suggesting a follow-up issue related to AddJitInfoForRange.

Comment 12

[Markus Stange [:mstange]]

What's the next step here? Pushing to try again and debugging the failures?

Comment 13

[Nicolas B. Pierron [:nbp]]

(In reply to Markus Stange [:mstange] from comment #12)

What's the next step here? Pushing to try again and debugging the failures?

Ideally someone from the profiler team should resume working on this feature, on top of the patch I rebased, fixing the issues which are remaining in the Profiler.