================
@@ -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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
