[PATCH] D60663: Time profiler: small fixes and optimizations

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri accepted this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

Ok, LG, thank you!



================
Comment at: llvm/lib/Support/TimeProfiler.cpp:30
+    cl::desc(
+        "Minimum time granularity (in microsecons) traced by time profiler"),
+    cl::init(500));
----------------
microsecon*d*s


================
Comment at: llvm/lib/Support/TimeProfiler.cpp:70-71
+
+  Entry(time_point<steady_clock> &&S, DurationType &&D, std::string &&N,
+        std::string &&Dt)
+      : Start(std::move(S)), Duration(std::move(D)), Name(std::move(N)),
----------------
I *think* `&&` are not needed.


================
Comment at: llvm/lib/Support/TimeProfiler.cpp:92
+    // Only include sections longer than TimeTraceGranularity.
+    if (duration_cast<microseconds>(E.Duration).count() > TimeTraceGranularity)
       Entries.emplace_back(E);
----------------
This will only track events that are **longer** than `TimeTraceGranularity`, which i think conflicts
with the `"Minimum time granularity (in microseconds) traced by time profiler"`
So either `>=` or reword the description a bit?


================
Comment at: llvm/lib/Support/TimeProfiler.cpp:156-157
 
-  std::vector<Entry> Stack;
-  std::vector<Entry> Entries;
+  SmallVector<Entry, 16> Stack;
+  SmallVector<Entry, 128> Entries;
   StringMap<CountAndDurationType> CountAndTotalPerName;
----------------
Since `TimeTraceProfiler` will be always heap-allocated (so stack usage is not a concern)
i think this is safe, and maybe even bump it a bit, depending on the values you see in average profiles?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60663/new/

https://reviews.llvm.org/D60663