[clang] [llvm] Added instant events and marking defered templated instantiation. (PR #103039)

Ilya Biryukov via cfe-commits cfe-commits at lists.llvm.org
Wed Aug 28 08:37:07 PDT 2024


================
@@ -92,13 +94,17 @@ struct TimeTraceMetadata {
   bool isEmpty() const { return Detail.empty() && File.empty(); }
 };
 
+struct TimeTraceProfilerEntry;
----------------
ilya-biryukov wrote:

The idea of sharing most code and configuring only the relevant bits we need to write for each event seems really nice, but the added class hierarchy and the way they're used may have some negative consequences on performance and readability of the code.

Performance-wise, we are now paying for
- for a vtable in each of the events,
- for `shared_ptr`s,
- for extra indirection in the output vector of events coming from `shared_ptrs`.

I would be surprised if we do not end up with a significant performance hit after these changes.
Readability-wise, the class hierarchy that uses `structs` and exposes members publicly is also somewhat of a code smell and does not feel necessarily better than the previous approach. Inheritance is one way to implement it, don't get me wrong, but the original code used a different approach and I don't think there's a strong reason to prefer inheritance here, even if we feel the need to move some code around (e.g. into separate functions).

- Could we go back to enum instead of the virtual methods? We could live with a switch on the enum value, it should not be a big deal. Also, ignoring some optional data (i.e. `End`, `InstantEvents`) for instant events should not be a problem.
- Could we get rid of `shared_ptr` altogether too? I am not sure in which cases we started introducing shared ownership in the first place, but I hope that's not actually needed as the previous ownership model should work just fine in presence of instant events.
- I feel it would be beneficial to distinguish between the output events and "in-progress" events as we store a lot of output events and they don't need to contain a vector of instant events attached to them, but the in-progress ones have to. This could be done through composition:
```cpp
struct InProgressEntry {
  TimeTraceProfilerEntry Event;
  std::vector<TimeTraceProfilerEntry> InstantEvents;
};
```

https://github.com/llvm/llvm-project/pull/103039


More information about the cfe-commits mailing list