Stack from profiler_capture_backtrace cannot be streamed from a marker in a different thread
Categories
(Core :: Gecko Profiler, defect, P2)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox84 | --- | fixed |
People
(Reporter: mozbugz, Assigned: mozbugz)
References
Details
Attachments
(1 file)
The combination of MarkerStack::Capture() and MarkerThreadId(someOtherThread) doesn't work, because:
profiler_capture_backtrace(which is used whenMarkerStack::Capture()is specified) returns a backtrace that internally records where the stack was captured.- This backtrace may be stored in a marker that is recorded into another thread, by using the
MarkerThreadIdoption; but the backtrace's thread id is not visibly remembered. - When streaming such marker, a
ProfilerBacktraceis reconstructed using the marker's own thread. - Because of this check, the stack's thread id is different from the marker's thread id, so the stack is not output!
This was working with the previous marker API, because backtraces were stored in ProfilerBacktraces, which included the correct thread id.
Either:
A. The marker should include the thread id where the backtrace was captured (requires some extra space).
B. The new API should use ProfilerBacktraces (difficult, more expensive).
C. The thread id should be somehow retrieved by pre-processing the backtrace contents (difficult, and requires double-reading).
D. When streaming stacks attached to a marker, the check should be bypassed by e.g., passing the magic not-a-thread 0 value, or a more explicit Maybe<int>, to ProfileBuffer::StreamSamplesToJSON()).
That check is needed when streaming all periodically-sampled samples from a thread, but in the case of a captured backtrace, there is only one sample, so the check is not really needed there.
So I think solution D is better, and could potentially help in future situations where we may want to stream single backtraces where the original thread is not relevant.
As an aside, we may want to really output the backtrace's thread id, for the front-end to display appropriately (e.g., to avoid confusion that a stack may look out of place in the marker's thread). So I will see if I can retrieve it when processing the backtrace, and a future bug (+ frontend issue) can expose it...
|
Assignee | |
Comment 1•5 years ago
|
||
The new Markers 2.0 code had missed one detail:
Backtraces in markers were serialized as just the ProfileChunkedBuffer, which doesn't expose the original thread id like ProfilerBacktrace did.
Then when outputting the profile, the marker code would use the marker's thread id (where the marker should be displayed in the frontend, which could be different from where the backtrace came) to deserialize and stream the attached marker, and a special check in the streaming code meant that the mismatched id would ignore the stored sample, and the displayed marker would show no stack.
With this patch, when streaming stacks from markers, the given thread id is 0 (an impossible thread id), which indicates that whatever sample is present should be streamed.
ProfilerBacktrace doesn't need to store the thread id anymore.
This solves the above problem.
As a bonus, the streaming code now reports the original thread of the sample(s) it found. This could be used in the future, to better show in the frontend that some stacks may come from other threads.
|
||
Updated•5 years ago
|
|
||
Comment 3•5 years ago
|
||
| bugherder | ||
Description
•