[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

[Anton Afanasyev via Phabricator via cfe-commits]

anton-afanasyev marked 23 inline comments as done.
anton-afanasyev added a comment.

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

> Some post-commit review (please submit a new review, don't replace this diff)
>  As usual, the incorrect license headers keep slipping through.


Ok, I've made a separate review for this: https://reviews.llvm.org/D60663



================
Comment at: llvm/trunk/include/llvm/Support/TimeProfiler.h:5-6
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
----------------
lebedev.ri wrote:
> OOOPS, wrong license.
Yes, thanks.


================
Comment at: llvm/trunk/include/llvm/Support/TimeProfiler.h:53
+/// is not initialized, the overhead is a single branch.
+struct TimeTraceScope {
+  TimeTraceScope(StringRef Name, StringRef Detail) {
----------------
lebedev.ri wrote:
> Did you mean to explicitly prohibit all the default constructors / `operator=`?
Ok, I'm to add lines like `TimeTraceScope() = delete;` here.


================
Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:5-6
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
----------------
lebedev.ri wrote:
> Wrong license.
Yes, I'm to fix it.


================
Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:29-30
+
+static std::string escapeString(StringRef Src) {
+  std::string OS;
+  for (const unsigned char &C : Src) {
----------------
lebedev.ri wrote:
> lebedev.ri wrote:
> > `SmallString<32>` ?
> > Also, it is safe to `OS.reserve(Src.size())`
> Also, you probably want to add `raw_svector_ostream` ontop, and `<<` into it.
This function has been already dropped here https://reviews.llvm.org/D60609 . I'm to submit code without it if json library using are ok for performance.


================
Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:59-60
+  DurationType Duration;
+  std::string Name;
+  std::string Detail;
+};
----------------
lebedev.ri wrote:
> `SmallString<32>`?
Hmm, would it make a sense for `Detail`? `Entry`s are heap-allocated, the actual size is ranging from several bytes to several hundreds. Also, getQualifiedNameAsString() which is usually used for `Detail` returns std::string.


================
Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:70-71
+
+  void begin(std::string Name, llvm::function_ref<std::string()> Detail) {
+    Entry E = {steady_clock::now(), {}, Name, Detail()};
+    Stack.push_back(std::move(E));
----------------
lebedev.ri wrote:
> Why not either take `StringRef` arg, or at least `std::move()` it when creating `Entry`?
Do you mean `std::move(Name)`? Ok.


================
Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:89
+    // itself.
+    if (std::find_if(++Stack.rbegin(), Stack.rend(), [&](const Entry &Val) {
+          return Val.Name == E.Name;
----------------
lebedev.ri wrote:
> llvm::find_if(llvm::reverse(), ) 
Hmm, one need not `[rbegin(), rend()]`, but `[rbegin()++,rend()]`, so have to explicitly specify begin and end, `llvm::reverse(Stack)` is inappropriate here.


================
Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:99
+
+  void Write(std::unique_ptr<raw_pwrite_stream> &OS) {
+    assert(Stack.empty() &&
----------------
lebedev.ri wrote:
> Why pass `std::unique_ptr` ?
> Just `raw_pwrite_stream &OS`
Yes, thanks! 
This was blindly copied from `CompilerInstance::createOutputFile()` return type.


================
Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:122
+      SortedTotals.push_back(E);
+    }
+    std::sort(SortedTotals.begin(), SortedTotals.end(),
----------------
lebedev.ri wrote:
> Elide `{}`
Ok, thanks.


================
Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:123
+    }
+    std::sort(SortedTotals.begin(), SortedTotals.end(),
+              [](const NameAndDuration &A, const NameAndDuration &B) {
----------------
lebedev.ri wrote:
> llvm::sort <- this is a correctness issue.
Yes, thanks!


================
Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:144-145
+
+  std::vector<Entry> Stack;
+  std::vector<Entry> Entries;
+  std::unordered_map<std::string, DurationType> TotalPerName;
----------------
lebedev.ri wrote:
> Would it make sense to make these `SmallVector`?
Ok, I'm to change it to
```
  SmallVector<Entry, 16> Stack;  
  SmallVector<Entry, 128> Entries;
```
`Stack` size may be enough for small sources while `Entries` usually amounts many thousands of `Entry`s.


================
Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:146-147
+  std::vector<Entry> Entries;
+  std::unordered_map<std::string, DurationType> TotalPerName;
+  std::unordered_map<std::string, size_t> CountPerName;
+  time_point<steady_clock> StartTime;
----------------
lebedev.ri wrote:
> 1. Eww, `std::unordered_map`, that will have *horrible* perf.
> 2. Eww, map with key = string. Use `llvm::StringMap`
You are right, but this is already fixed and submitted in this follow-up: https://reviews.llvm.org/D60404


================
Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:162
+
+void timeTraceProfilerWrite(std::unique_ptr<raw_pwrite_stream> &OS) {
+  assert(TimeTraceProfilerInstance != nullptr &&
----------------
lebedev.ri wrote:
> Just `raw_pwrite_stream &OS`?
Yes, thanks.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58675