Add tracing and event support for parallel tasks#9192
Conversation
Also compresses the trace packet format by union-ing lifetime-disjoint fields. It was decided to do this here rather than in a separate PR to avoid excess API and ABI churn. Co-authored-by: OpenAI Codex <noreply@openai.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9192 +/- ##
=======================================
Coverage ? 69.39%
=======================================
Files ? 254
Lines ? 78345
Branches ? 18739
=======================================
Hits ? 54369
Misses ? 18486
Partials ? 5490 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| }; | ||
|
|
||
| #if (__cplusplus >= 201103L || _MSVC_LANG >= 201103L) | ||
| static_assert(sizeof(halide_trace_packet_t) == 6 * sizeof(uint32_t), "size mismatch in halide_trace_packet_t"); |
There was a problem hiding this comment.
So just to confirm, the packet size savings from this change is 4B/packet (28B → 24B) due to the type punning trick with the union above, correct? And then store/load events will access value_index and type in these offsets, while all other events will access id and thread_id, yes? Makes sense, just trying to confirm my understanding.
There was a problem hiding this comment.
Yep, that's all right
| if (e->event == halide_trace_load || e->event == halide_trace_store) { | ||
| packet->value_index = e->value_index; | ||
| packet->type = e->type; | ||
| } else { | ||
| packet->id = my_id; | ||
| packet->thread_id = (e->event == halide_trace_begin_parallel_task) ? e->thread_id : 0; | ||
| } |
There was a problem hiding this comment.
Ah ok, this answers my question from above, looks like I'm understanding the access pattern correctly!
parkerziegler
left a comment
There was a problem hiding this comment.
Looks good! I can't comment too much on the C++ side of things, but from a Halidoscope perspective the changes should be reasonable to port. We'll need to:
- Adjust the Rust packet parsing to account for the
uniontype punning and changed packet size. The Rust parser will handle this by just selecting the proper decoding function based on the event type (i.e., should I interpret the next four bytes as, say, ani32forthread_idor a sequence ofu8,u8,u16fortype.code,type.bits,type.lane). - Add the new event types.
- Ignore names on parallel events, which I believe correspond to "loop names" rather than func names based on L130-134 of test/correctness/tracing_thread_ids.cpp.
All in all, it should be pretty minimal. What I'll do is capture a trace with these changes locally, switch back over to my branch, and then implement the above updates on the Rust side. But I don't think that work needs to block this; I can always adjust my side if there's something I'm missing.
|
Per discussion with @abadams, the reported thread-ids match the OS platform libraries. This resulted in a bunch more code since every platform does it a little differently, but I think this is a good net change. We had discussed using Experiment:
|
This pull request adds tracing and event support for tasks that execute in parallel. It also optimizes the trace packet format by compressing it through the union of lifetime-disjoint fields. Combining these changes in a single pull request avoids unnecessary API and ABI churn.
Breaking changes
None.
Checklist