When collecting stacks for the profiler, include line + column information for JIT frames
Categories
(Core :: JavaScript Engine, enhancement, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox60 | --- | affected |
People
(Reporter: mstange, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•6 years ago
|
Updated•6 years ago
|
Reporter | ||
Updated•4 years ago
|
Comment 3•4 years ago
|
||
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.
Reporter | ||
Comment 4•3 years ago
|
||
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•3 years ago
|
||
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.
Reporter | ||
Comment 6•3 years ago
|
||
Thanks, Nicolas!
Comment 7•3 years ago
|
||
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•3 years ago
|
||
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.
Reporter | ||
Comment 9•3 years ago
|
||
This all sounds great!
Comment 10•3 years ago
|
||
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•3 years ago
|
||
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
.
Reporter | ||
Comment 12•2 years ago
|
||
What's the next step here? Pushing to try again and debugging the failures?
Comment 13•2 years ago
|
||
(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.
Updated•2 years ago
|
Description
•