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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

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



================
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.
+//
----------------
OOOPS, wrong license.


================
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) {
----------------
Did you mean to explicitly prohibit all the default constructors / `operator=`?


================
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.
+//
----------------
Wrong license.


================
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) {
----------------
`SmallString<32>` ?
Also, it is safe to `OS.reserve(Src.size())`


================
Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:59-60
+  DurationType Duration;
+  std::string Name;
+  std::string Detail;
+};
----------------
`SmallString<32>`?


================
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));
----------------
Why not either take `StringRef` arg, or at least `std::move()` it when creating `Entry`?


================
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;
----------------
llvm::find_if(llvm::reverse(), ) 


================
Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:99
+
+  void Write(std::unique_ptr<raw_pwrite_stream> &OS) {
+    assert(Stack.empty() &&
----------------
Why pass `std::unique_ptr` ?
Just `raw_pwrite_stream &OS`


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


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


================
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;
----------------
Would it make sense to make these `SmallVector`?


================
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;
----------------
1. Eww, `std::unordered_map`, that will have *horrible* perf.
2. Eww, map with key = string. Use `llvm::StringMap`


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


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58675