Add C++ Interpreter frames to JS::ProfilingFrameIterator
Categories
(Core :: JavaScript Engine, task, P3)
Tracking
()
People
(Reporter: jandem, Unassigned)
References
(Blocks 1 open bug)
Details
The C++ interpreter still uses pseudo stack entries. We could probably change this to be more similar to JIT and Wasm frames: use profilingActivation for InterpreterActivation too and add an iterator for walking the list of InterpreterFrames and report these similar to Baseline Interpreter frames.
We'd have to audit the ordering of fp/sp/pc writes and document this.
The script's "dynamic string" could initially be stored in the InterpreterFrame, similar to how the Baseline Interpreter stores this in JitScript.
|
Reporter | |
Comment 1•3 years ago
|
||
Markus, does this sound reasonable? I think this was the original plan when JIT profiling was overhauled but it never happened.
JS::ProfilingFrameIterator already supports the JIT and Wasm iterators internally so I think this would fit in pretty well.
|
|
Reporter | |
Comment 2•3 years ago
|
||
The most complicated part is probably figuring out what sort of atomics to use where. For the JIT profiling fields we use relaxed atomics and ProfiilngStackFrame uses release-acquire.
|
||
Comment 3•3 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #1)
Markus, does this sound reasonable? I think this was the original plan when JIT profiling was overhauled but it never happened.
Yes, it sounds great! I had a conversation about this with djvj in early 2016 (notes here) but never got around to filing the bug...
I have a nebulous concern that interpreter profiling is currently more expensive than JIT profiling (bug 848071) but I don't know if that's actually true. If this change causes us to stop copying the function name string (including script URL) to the buffer, then that would be a nice improvement, I think.
(In reply to Jan de Mooij [:jandem] from comment #2)
ProfiilngStackFrameuses release-acquire.
It uses release-acquire so that the sampler doesn't read partial state; in particular we don't want it to read partially-initialized entries, so we need to make sure that the "push" is observed after the fields are set. I don't know what the synchronization concerns are in the profiling frame iterator.
|
|
Reporter | |
Comment 4•3 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #3)
I have a nebulous concern that interpreter profiling is currently more expensive than JIT profiling (bug 848071) but I don't know if that's actually true. If this change causes us to stop copying the function name string (including script URL) to the buffer, then that would be a nice improvement, I think.
Yeah we definitely need a better solution for this string... For the Baseline Interpreter I ended up storing it in JitScript when profiling, and the profiler materializes a JS pseudo stack entry. Maybe improving this for the Baseline Interpreter would be a good first step, we can then apply the same mechanism to the C++ interpreter.
Description
•