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.

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.

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.

Thanks, Nicolas!

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.

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.

This all sounds great!

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.

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.

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

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