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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Looks good ignoring the json bits.

Re license:

> https://reviews.llvm.org/D58675
>  This is the first part of time tracing system, I have splitted them cause this part is mostly written by Aras Pranckevicius except of several minor fixes concerning formatting.

So i can't and won't claim any legal knowledge, but it maybe would be good for him to at least comment here, that he is ok with this?



================
Comment at: llvm/lib/Support/TimeProfiler.cpp:20
 #include <string>
 #include <unordered_map>
 #include <vector>
----------------
Unused header?


================
Comment at: llvm/lib/Support/TimeProfiler.cpp:37-38
   DurationType Duration;
   std::string Name;
   std::string Detail;
 };
----------------
Ah yes, they are allocated when created. Hmm.


================
Comment at: llvm/lib/Support/TimeProfiler.cpp:47-48
   void begin(std::string Name, llvm::function_ref<std::string()> Detail) {
-    Entry E = {steady_clock::now(), {}, Name, Detail()};
+    Entry E = {steady_clock::now(), {}, std::move(Name), Detail()};
     Stack.push_back(std::move(E));
   }
----------------
Does
```
    Stack.emplace_back(steady_clock::now(), {}, std::move(Name), Detail());
```
not work?
(also, the `std::string` returned from `Detail` function invocation is moved?)


================
Comment at: llvm/lib/Support/TimeProfiler.cpp:57
     // Only include sections longer than 500us.
     if (duration_cast<microseconds>(E.Duration).count() > 500)
       Entries.emplace_back(E);
----------------
I feel like this should be 
```
static cl::opt<unsigned> TimeProfileGranularity(
    "time-profile-granularity",
    cl::desc("<fixme>"),
    cl::init(500));
```
?


================
Comment at: llvm/lib/Support/TimeProfiler.cpp:65
     // itself.
     if (std::find_if(++Stack.rbegin(), Stack.rend(), [&](const Entry &Val) {
           return Val.Name == E.Name;
----------------
Yes, good point.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60663