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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
