pantsbuild / pants

The Pants Build System
https://www.pantsbuild.org
Apache License 2.0
3.32k stars 636 forks source link

Track PID/TID in WorkUnitMetadata #13960

Closed thejcannon closed 2 years ago

thejcannon commented 2 years ago

Is your feature request related to a problem? Please describe. The work unit callback could be used to output, say, a nice multi-process, multi-threaded flamegraph or Chrome JSON trace. However without the PID/TID, you'll end up with overlapping spans which isn't great.

Describe the solution you'd like PID/TID in the workunit metadata

Describe alternatives you've considered N/A

Additional context The PID/TID dont' need to be 1:1 with the OS-implementation. Just unique across the processes/threads

thejcannon commented 2 years ago

Initial attempts by a complete noob show that the PID is always the same if I add it directly in WorkunitMetadata::default.

This means either:

Either way, the implementation needs to be smarter.

Also, Rust's ThreadID seems to be opaque, and turning it to an int is "experimental"

jsirois commented 2 years ago

Keep in mind the Rust side is M-N via the tokio event loop. So an asynchronous task might jump between any number of threads before it completes. Also keep in mind that the only pids besides the main pantsd pid are those of the processes rust spawns to implement the Process -> ProcessResult intrinsic.

thejcannon commented 2 years ago

Whether threads or shared or not, reporting them uniquely would give an accurate view of what's going on under the hood.

In fact I would consider reporting shared thread IDs to be desirable so someone viewing the graph can see that threads are being re-used and get the warm fuzzies.

stuhood commented 2 years ago

Whether threads or shared or not, reporting them uniquely would give an accurate view of what's going on under the hood.

In fact I would consider reporting shared thread IDs to be desirable so someone viewing the graph can see that threads are being re-used and get the warm fuzzies.

It wouldn't, unfortunately: as John said, with an M-N scheduler, thread ids have zero association with what work is actually being done.


We spawn a Rust async "task" per node/@rule in the graph (which roughly corresponds to a workunit, but there will actually be a few more workunits than nodes because the Rust code manually creates workunits for finer grained work). So the most realistic "task id" effectively just ends up being the span_id of the workunit itself. It will be scheduled effectively-randomly onto a much smaller number of OS threads. The workunits form a tree (as discussed in Slack, I think that we should probably migrate that to a DAG, at least internally, but that doesn't change the strategy for this issue).

So one way to approximate what you are trying to do here is to choose a particular workunit type, and then render only the children of nodes of that type: i.e., render all flamegraphs below a particular @rule name. To do this, you'd match all workunits of a particular type and call them the "roots" of your trace. Then you'd assign all of the children of each root to the root span id... basically, treating the root span id as a thread id for the subgraph it represents.

stuhood commented 2 years ago

Going to call this closed by the previous comment: in general, assigning all of the children of a workunit to its span_id as a "thread id" accomplishes the goal. But it's a matter of choosing scoping.

stuhood commented 2 years ago

Oh: there is a "nail in the coffin" aspect to this issue that I totally forgot. As it runs, a workunit/task will actually end up running on dozens of different threads, because each time it awaits, it will get rescheduled. So not only is it random, but it also is a collection of ids rather than just one.

stuhood commented 2 years ago

We spawn a Rust async "task" per node/@rule in the graph (which roughly corresponds to a workunit, but there will actually be a few more workunits than nodes because the Rust code manually creates workunits for finer grained work). So the most realistic "task id" effectively just ends up being the span_id of the workunit itself. It will be scheduled effectively-randomly onto a much smaller number of OS threads. The workunits form a tree (as discussed in Slack, I think that we should probably migrate that to a DAG, at least internally, but that doesn't change the strategy for this issue).

See #14680 for some followup here. Getting accurate self-time for profiling is only one of the reasons why this should probably be a DAG.