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