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

[Anton Afanasyev via Phabricator via cfe-commits]

anton-afanasyev marked 7 inline comments as done.
anton-afanasyev added a subscriber: aras-p.
anton-afanasyev added a comment.

In D60663#1465721 <https://reviews.llvm.org/D60663#1465721>, @lebedev.ri wrote:

> 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?


Oh, sure! @aras-p -- I have changed license header to fit it with current LLVM Relicensing state. Are you ok with this? (see https://reviews.llvm.org/D60663#change-jr7Lagn5WNFy)



================
Comment at: llvm/lib/Support/TimeProfiler.cpp:20
 #include <string>
 #include <unordered_map>
 #include <vector>
----------------
lebedev.ri wrote:
> Unused header?
Yes, thanks.


================
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));
   }
----------------
lebedev.ri wrote:
> Does
> ```
>     Stack.emplace_back(steady_clock::now(), {}, std::move(Name), Detail());
> ```
> not work?
> (also, the `std::string` returned from `Detail` function invocation is moved?)
Ok, changed.


================
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);
----------------
lebedev.ri wrote:
> I feel like this should be 
> ```
> static cl::opt<unsigned> TimeProfileGranularity(
>     "time-profile-granularity",
>     cl::desc("<fixme>"),
>     cl::init(500));
> ```
> ?
I planned to change this later together with unit tests (cause they need small time granularity), but can fix it now. Changed, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60663