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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:27
 
+static cl::opt TimeTraceGranularity(
+"time-trace-granularity",

anton-afanasyev wrote:
> sammccall wrote:
> > I know this is late, but... this shows up in the help for any tool that 
> > links in libSupport, many of which don't support the time profiler. Can you 
> > mark this as hidden or (preferably) move this to cc1_main?
> @sammccall Yes, thanks! Here is the fix https://reviews.llvm.org/D65202 , 
> please, review it.
Thanks! A few nits but that's a big improvement I think.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60663



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Anton Afanasyev via Phabricator via cfe-commits]

anton-afanasyev marked 2 inline comments as done.
anton-afanasyev added inline comments.



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:27
 
+static cl::opt TimeTraceGranularity(
+"time-trace-granularity",

sammccall wrote:
> I know this is late, but... this shows up in the help for any tool that links 
> in libSupport, many of which don't support the time profiler. Can you mark 
> this as hidden or (preferably) move this to cc1_main?
@sammccall Yes, thanks! Here is the fix https://reviews.llvm.org/D65202 , 
please, review it.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60663



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:27
 
+static cl::opt TimeTraceGranularity(
+"time-trace-granularity",

I know this is late, but... this shows up in the help for any tool that links 
in libSupport, many of which don't support the time profiler. Can you mark this 
as hidden or (preferably) move this to cc1_main?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60663



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Anton Afanasyev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL358448: Time profiler: small fixes and optimizations 
(authored by anton-afanasyev, committed by ).
Herald added a subscriber: kristina.

Changed prior to commit:
  https://reviews.llvm.org/D60663?vs=195069=195244#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D60663

Files:
  cfe/trunk/tools/driver/cc1_main.cpp
  llvm/trunk/include/llvm/Support/TimeProfiler.h
  llvm/trunk/lib/Support/TimeProfiler.cpp

Index: llvm/trunk/include/llvm/Support/TimeProfiler.h
===
--- llvm/trunk/include/llvm/Support/TimeProfiler.h
+++ llvm/trunk/include/llvm/Support/TimeProfiler.h
@@ -1,9 +1,8 @@
 //===- llvm/Support/TimeProfiler.h - Hierarchical Time Profiler -*- C++ -*-===//
 //
-// The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===--===//
 
@@ -33,7 +32,7 @@
 /// Write profiling data to output file.
 /// Data produced is JSON, in Chrome "Trace Event" format, see
 /// https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAySU/preview
-void timeTraceProfilerWrite(std::unique_ptr );
+void timeTraceProfilerWrite(raw_pwrite_stream );
 
 /// Manually begin a time section, with the given \p Name and \p Detail.
 /// Profiler copies the string data, so the pointers can be given into
@@ -51,6 +50,13 @@
 /// the section; and when it is destroyed, it stops it. If the time profiler
 /// is not initialized, the overhead is a single branch.
 struct TimeTraceScope {
+
+  TimeTraceScope() = delete;
+  TimeTraceScope(const TimeTraceScope &) = delete;
+  TimeTraceScope =(const TimeTraceScope &) = delete;
+  TimeTraceScope(TimeTraceScope &&) = delete;
+  TimeTraceScope =(TimeTraceScope &&) = delete;
+
   TimeTraceScope(StringRef Name, StringRef Detail) {
 if (TimeTraceProfilerInstance != nullptr)
   timeTraceProfilerBegin(Name, Detail);
Index: llvm/trunk/lib/Support/TimeProfiler.cpp
===
--- llvm/trunk/lib/Support/TimeProfiler.cpp
+++ llvm/trunk/lib/Support/TimeProfiler.cpp
@@ -1,30 +1,35 @@
 //===-- TimeProfiler.cpp - Hierarchical Time Profiler -===//
 //
-// The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===--===//
 //
-/// \file Hierarchical time profiler implementation.
+// This file implements hierarchical time profiler.
 //
 //===--===//
 
 #include "llvm/Support/TimeProfiler.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
 #include 
 #include 
 #include 
-#include 
 #include 
 
 using namespace std::chrono;
 
 namespace llvm {
 
+static cl::opt TimeTraceGranularity(
+"time-trace-granularity",
+cl::desc(
+"Minimum time granularity (in microseconds) traced by time profiler"),
+cl::init(500));
+
 TimeTraceProfiler *TimeTraceProfilerInstance = nullptr;
 
 static std::string escapeString(StringRef Src) {
@@ -61,18 +66,21 @@
   DurationType Duration;
   std::string Name;
   std::string Detail;
+
+  Entry(time_point &, DurationType &, std::string &,
+std::string &)
+  : Start(std::move(S)), Duration(std::move(D)), Name(std::move(N)),
+Detail(std::move(Dt)){};
 };
 
 struct TimeTraceProfiler {
   TimeTraceProfiler() {
-Stack.reserve(8);
-Entries.reserve(128);
 StartTime = steady_clock::now();
   }
 
   void begin(std::string Name, llvm::function_ref Detail) {
-Entry E = {steady_clock::now(), {}, Name, Detail()};
-Stack.push_back(std::move(E));
+Stack.emplace_back(steady_clock::now(), DurationType{}, std::move(Name),
+   Detail());
   }
 
   void end() {
@@ -80,8 +88,8 @@
 auto  = Stack.back();
 E.Duration = steady_clock::now() - E.Start;
 
-// Only include sections longer than 500us.
-if (duration_cast(E.Duration).count() > 500)
+// Only include sections longer than TimeTraceGranularity msec.
+if (duration_cast(E.Duration).count() > 

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

[Aras Pranckevičius via Phabricator via cfe-commits]

aras-p added a comment.

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

Yes, absolutely fine. The only reason why some files started with the old 
license blurb is because I started the branch before licensing switch kicked in.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60663



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri accepted this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

Ok, LG, thank you!




Comment at: llvm/lib/Support/TimeProfiler.cpp:30
+cl::desc(
+"Minimum time granularity (in microsecons) traced by time profiler"),
+cl::init(500));

microsecon*d*s



Comment at: llvm/lib/Support/TimeProfiler.cpp:70-71
+
+  Entry(time_point &, DurationType &, std::string &,
+std::string &)
+  : Start(std::move(S)), Duration(std::move(D)), Name(std::move(N)),

I *think* `&&` are not needed.



Comment at: llvm/lib/Support/TimeProfiler.cpp:92
+// Only include sections longer than TimeTraceGranularity.
+if (duration_cast(E.Duration).count() > TimeTraceGranularity)
   Entries.emplace_back(E);

This will only track events that are **longer** than `TimeTraceGranularity`, 
which i think conflicts
with the `"Minimum time granularity (in microseconds) traced by time profiler"`
So either `>=` or reword the description a bit?



Comment at: llvm/lib/Support/TimeProfiler.cpp:156-157
 
-  std::vector Stack;
-  std::vector Entries;
+  SmallVector Stack;
+  SmallVector Entries;
   StringMap CountAndTotalPerName;

Since `TimeTraceProfiler` will be always heap-allocated (so stack usage is not 
a concern)
i think this is safe, and maybe even bump it a bit, depending on the values you 
see in average profiles?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60663



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Anton Afanasyev via Phabricator via cfe-commits]

anton-afanasyev added a comment.

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


I've communicated to Aras by mail, he is ok with new license header:

> "Sure! I started my branch before the LLVM license change. The new one is 
> fine."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60663



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Anton Afanasyev via Phabricator via cfe-commits]

anton-afanasyev updated this revision to Diff 195069.
anton-afanasyev added a comment.

Updated


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60663

Files:
  clang/tools/driver/cc1_main.cpp
  llvm/include/llvm/Support/TimeProfiler.h
  llvm/lib/Support/TimeProfiler.cpp

Index: llvm/lib/Support/TimeProfiler.cpp
===
--- llvm/lib/Support/TimeProfiler.cpp
+++ llvm/lib/Support/TimeProfiler.cpp
@@ -1,30 +1,35 @@
 //===-- TimeProfiler.cpp - Hierarchical Time Profiler -===//
 //
-// The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===--===//
 //
-/// \file Hierarchical time profiler implementation.
+// This file implements hierarchical time profiler.
 //
 //===--===//
 
 #include "llvm/Support/TimeProfiler.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
 #include 
 #include 
 #include 
-#include 
 #include 
 
 using namespace std::chrono;
 
 namespace llvm {
 
+static cl::opt TimeTraceGranularity(
+"time-trace-granularity",
+cl::desc(
+"Minimum time granularity (in microsecons) traced by time profiler"),
+cl::init(500));
+
 TimeTraceProfiler *TimeTraceProfilerInstance = nullptr;
 
 static std::string escapeString(StringRef Src) {
@@ -61,18 +66,21 @@
   DurationType Duration;
   std::string Name;
   std::string Detail;
+
+  Entry(time_point &, DurationType &, std::string &,
+std::string &)
+  : Start(std::move(S)), Duration(std::move(D)), Name(std::move(N)),
+Detail(std::move(Dt)){};
 };
 
 struct TimeTraceProfiler {
   TimeTraceProfiler() {
-Stack.reserve(8);
-Entries.reserve(128);
 StartTime = steady_clock::now();
   }
 
   void begin(std::string Name, llvm::function_ref Detail) {
-Entry E = {steady_clock::now(), {}, Name, Detail()};
-Stack.push_back(std::move(E));
+Stack.emplace_back(steady_clock::now(), DurationType{}, std::move(Name),
+   Detail());
   }
 
   void end() {
@@ -80,8 +88,8 @@
 auto  = Stack.back();
 E.Duration = steady_clock::now() - E.Start;
 
-// Only include sections longer than 500us.
-if (duration_cast(E.Duration).count() > 500)
+// Only include sections longer than TimeTraceGranularity.
+if (duration_cast(E.Duration).count() > TimeTraceGranularity)
   Entries.emplace_back(E);
 
 // Track total time taken by each "name", but only the topmost levels of
@@ -100,20 +108,20 @@
 Stack.pop_back();
   }
 
-  void Write(std::unique_ptr ) {
+  void Write(raw_pwrite_stream ) {
 assert(Stack.empty() &&
"All profiler sections should be ended when calling Write");
 
-*OS << "{ \"traceEvents\": [\n";
+OS << "{ \"traceEvents\": [\n";
 
 // Emit all events for the main flame graph.
 for (const auto  : Entries) {
   auto StartUs = duration_cast(E.Start - StartTime).count();
   auto DurUs = duration_cast(E.Duration).count();
-  *OS << "{ \"pid\":1, \"tid\":0, \"ph\":\"X\", \"ts\":" << StartUs
-  << ", \"dur\":" << DurUs << ", \"name\":\"" << escapeString(E.Name)
-  << "\", \"args\":{ \"detail\":\"" << escapeString(E.Detail)
-  << "\"} },\n";
+  OS << "{ \"pid\":1, \"tid\":0, \"ph\":\"X\", \"ts\":" << StartUs
+ << ", \"dur\":" << DurUs << ", \"name\":\"" << escapeString(E.Name)
+ << "\", \"args\":{ \"detail\":\"" << escapeString(E.Detail)
+ << "\"} },\n";
 }
 
 // Emit totals by section name as additional "thread" events, sorted from
@@ -121,32 +129,32 @@
 int Tid = 1;
 std::vector SortedTotals;
 SortedTotals.reserve(CountAndTotalPerName.size());
-for (const auto  : CountAndTotalPerName) {
+for (const auto  : CountAndTotalPerName)
   SortedTotals.emplace_back(E.getKey(), E.getValue());
-}
-std::sort(SortedTotals.begin(), SortedTotals.end(),
-  [](const NameAndCountAndDurationType ,
- const NameAndCountAndDurationType ) {
-return A.second.second > B.second.second;
-  });
+
+llvm::sort(SortedTotals.begin(), SortedTotals.end(),
+   [](const NameAndCountAndDurationType ,
+  const NameAndCountAndDurationType ) {
+ return A.second.second > B.second.second;
+   });
 for (const auto  : SortedTotals) {
   auto DurUs = 

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

[Anton Afanasyev via Phabricator via cfe-commits]

anton-afanasyev updated this revision to Diff 195068.
anton-afanasyev marked 2 inline comments as done.
anton-afanasyev added a comment.

Updated


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60663

Files:
  clang/tools/driver/cc1_main.cpp
  llvm/include/llvm/Support/TimeProfiler.h
  llvm/lib/Support/TimeProfiler.cpp

Index: llvm/lib/Support/TimeProfiler.cpp
===
--- llvm/lib/Support/TimeProfiler.cpp
+++ llvm/lib/Support/TimeProfiler.cpp
@@ -1,19 +1,19 @@
 //===-- TimeProfiler.cpp - Hierarchical Time Profiler -===//
 //
-// The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===--===//
 //
-/// \file Hierarchical time profiler implementation.
+// This file implements hierarchical time profiler.
 //
 //===--===//
 
 #include "llvm/Support/TimeProfiler.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
 #include 
 #include 
@@ -25,6 +25,12 @@
 
 namespace llvm {
 
+static cl::opt TimeTraceGranularity(
+"time-trace-granularity",
+cl::desc(
+"Minimum time granularity (in microsecons) traced by time profiler"),
+cl::init(500));
+
 TimeTraceProfiler *TimeTraceProfilerInstance = nullptr;
 
 static std::string escapeString(StringRef Src) {
@@ -61,18 +67,21 @@
   DurationType Duration;
   std::string Name;
   std::string Detail;
+
+  Entry(time_point &, DurationType &, std::string &,
+std::string &)
+  : Start(std::move(S)), Duration(std::move(D)), Name(std::move(N)),
+Detail(std::move(Dt)){};
 };
 
 struct TimeTraceProfiler {
   TimeTraceProfiler() {
-Stack.reserve(8);
-Entries.reserve(128);
 StartTime = steady_clock::now();
   }
 
   void begin(std::string Name, llvm::function_ref Detail) {
-Entry E = {steady_clock::now(), {}, Name, Detail()};
-Stack.push_back(std::move(E));
+Stack.emplace_back(steady_clock::now(), DurationType{}, std::move(Name),
+   Detail());
   }
 
   void end() {
@@ -80,8 +89,8 @@
 auto  = Stack.back();
 E.Duration = steady_clock::now() - E.Start;
 
-// Only include sections longer than 500us.
-if (duration_cast(E.Duration).count() > 500)
+// Only include sections longer than TimeTraceGranularity.
+if (duration_cast(E.Duration).count() > TimeTraceGranularity)
   Entries.emplace_back(E);
 
 // Track total time taken by each "name", but only the topmost levels of
@@ -100,20 +109,20 @@
 Stack.pop_back();
   }
 
-  void Write(std::unique_ptr ) {
+  void Write(raw_pwrite_stream ) {
 assert(Stack.empty() &&
"All profiler sections should be ended when calling Write");
 
-*OS << "{ \"traceEvents\": [\n";
+OS << "{ \"traceEvents\": [\n";
 
 // Emit all events for the main flame graph.
 for (const auto  : Entries) {
   auto StartUs = duration_cast(E.Start - StartTime).count();
   auto DurUs = duration_cast(E.Duration).count();
-  *OS << "{ \"pid\":1, \"tid\":0, \"ph\":\"X\", \"ts\":" << StartUs
-  << ", \"dur\":" << DurUs << ", \"name\":\"" << escapeString(E.Name)
-  << "\", \"args\":{ \"detail\":\"" << escapeString(E.Detail)
-  << "\"} },\n";
+  OS << "{ \"pid\":1, \"tid\":0, \"ph\":\"X\", \"ts\":" << StartUs
+ << ", \"dur\":" << DurUs << ", \"name\":\"" << escapeString(E.Name)
+ << "\", \"args\":{ \"detail\":\"" << escapeString(E.Detail)
+ << "\"} },\n";
 }
 
 // Emit totals by section name as additional "thread" events, sorted from
@@ -121,32 +130,32 @@
 int Tid = 1;
 std::vector SortedTotals;
 SortedTotals.reserve(CountAndTotalPerName.size());
-for (const auto  : CountAndTotalPerName) {
+for (const auto  : CountAndTotalPerName)
   SortedTotals.emplace_back(E.getKey(), E.getValue());
-}
-std::sort(SortedTotals.begin(), SortedTotals.end(),
-  [](const NameAndCountAndDurationType ,
- const NameAndCountAndDurationType ) {
-return A.second.second > B.second.second;
-  });
+
+llvm::sort(SortedTotals.begin(), SortedTotals.end(),
+   [](const NameAndCountAndDurationType ,
+  const NameAndCountAndDurationType ) {
+ return A.second.second > B.second.second;
+   });
 for (const auto  : SortedTotals) {
   auto 

[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 , @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 
 #include 
 #include 

lebedev.ri wrote:
> Unused header?
Yes, thanks.



Comment at: llvm/lib/Support/TimeProfiler.cpp:47-48
   void begin(std::string Name, llvm::function_ref 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(E.Duration).count() > 500)
   Entries.emplace_back(E);

lebedev.ri wrote:
> I feel like this should be 
> ```
> static cl::opt TimeProfileGranularity(
> "time-profile-granularity",
> cl::desc(""),
> 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

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?




Comment at: llvm/lib/Support/TimeProfiler.cpp:20
 #include 
 #include 
 #include 

Unused header?



Comment at: llvm/lib/Support/TimeProfiler.cpp:37-38
   DurationType Duration;
   std::string Name;
   std::string Detail;
 };

Ah yes, they are allocated when created. Hmm.



Comment at: llvm/lib/Support/TimeProfiler.cpp:47-48
   void begin(std::string Name, llvm::function_ref Detail) {
-Entry E = {steady_clock::now(), {}, Name, Detail()};
+Entry E = {steady_clock::now(), {}, std::move(Name), Detail()};
 Stack.push_back(std::move(E));
   }

Does
```
Stack.emplace_back(steady_clock::now(), {}, std::move(Name), Detail());
```
not work?
(also, the `std::string` returned from `Detail` function invocation is moved?)



Comment at: llvm/lib/Support/TimeProfiler.cpp:57
 // Only include sections longer than 500us.
 if (duration_cast(E.Duration).count() > 500)
   Entries.emplace_back(E);

I feel like this should be 
```
static cl::opt TimeProfileGranularity(
"time-profile-granularity",
cl::desc(""),
cl::init(500));
```
?



Comment at: llvm/lib/Support/TimeProfiler.cpp:65
 // itself.
 if (std::find_if(++Stack.rbegin(), Stack.rend(), [&](const Entry ) {
   return Val.Name == E.Name;

Yes, good point.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60663



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Anton Afanasyev via Phabricator via cfe-commits]

anton-afanasyev created this revision.
anton-afanasyev added a reviewer: lebedev.ri.
Herald added subscribers: llvm-commits, cfe-commits, mgrang, hiraditya.
Herald added projects: clang, LLVM.

Fixes from Roman's review here: https://reviews.llvm.org/D58675#1465336


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D60663

Files:
  clang/tools/driver/cc1_main.cpp
  llvm/include/llvm/Support/TimeProfiler.h
  llvm/lib/Support/TimeProfiler.cpp

Index: llvm/lib/Support/TimeProfiler.cpp
===
--- llvm/lib/Support/TimeProfiler.cpp
+++ llvm/lib/Support/TimeProfiler.cpp
@@ -1,13 +1,12 @@
 //===-- TimeProfiler.cpp - Hierarchical Time Profiler -===//
 //
-// The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===--===//
 //
-/// \file Hierarchical time profiler implementation.
+// This file implements hierarchical time profiler.
 //
 //===--===//
 
@@ -41,13 +40,11 @@
 
 struct TimeTraceProfiler {
   TimeTraceProfiler() {
-Stack.reserve(8);
-Entries.reserve(128);
 StartTime = steady_clock::now();
   }
 
   void begin(std::string Name, llvm::function_ref Detail) {
-Entry E = {steady_clock::now(), {}, Name, Detail()};
+Entry E = {steady_clock::now(), {}, std::move(Name), Detail()};
 Stack.push_back(std::move(E));
   }
 
@@ -76,7 +73,7 @@
 Stack.pop_back();
   }
 
-  void Write(std::unique_ptr ) {
+  void Write(raw_pwrite_stream ) {
 assert(Stack.empty() &&
"All profiler sections should be ended when calling Write");
 
@@ -103,14 +100,14 @@
 int Tid = 1;
 std::vector SortedTotals;
 SortedTotals.reserve(CountAndTotalPerName.size());
-for (const auto  : CountAndTotalPerName) {
+for (const auto  : CountAndTotalPerName)
   SortedTotals.emplace_back(E.getKey(), E.getValue());
-}
-std::sort(SortedTotals.begin(), SortedTotals.end(),
-  [](const NameAndCountAndDurationType ,
- const NameAndCountAndDurationType ) {
-return A.second.second > B.second.second;
-  });
+
+llvm::sort(SortedTotals.begin(), SortedTotals.end(),
+   [](const NameAndCountAndDurationType ,
+  const NameAndCountAndDurationType ) {
+ return A.second.second > B.second.second;
+   });
 for (const auto  : SortedTotals) {
   auto DurUs = duration_cast(E.second.second).count();
   auto Count = CountAndTotalPerName[E.first].first;
@@ -140,12 +137,12 @@
 {"args", json::Object{{"name", "clang"}}},
 });
 
-*OS << formatv("{0:2}", json::Value(json::Object(
-{{"traceEvents", std::move(Events)}})));
+OS << formatv("{0:2}", json::Value(json::Object(
+   {{"traceEvents", std::move(Events)}})));
   }
 
-  std::vector Stack;
-  std::vector Entries;
+  SmallVector Stack;
+  SmallVector Entries;
   StringMap CountAndTotalPerName;
   time_point StartTime;
 };
@@ -161,7 +158,7 @@
   TimeTraceProfilerInstance = nullptr;
 }
 
-void timeTraceProfilerWrite(std::unique_ptr ) {
+void timeTraceProfilerWrite(raw_pwrite_stream ) {
   assert(TimeTraceProfilerInstance != nullptr &&
  "Profiler object can't be null");
   TimeTraceProfilerInstance->Write(OS);
Index: llvm/include/llvm/Support/TimeProfiler.h
===
--- llvm/include/llvm/Support/TimeProfiler.h
+++ llvm/include/llvm/Support/TimeProfiler.h
@@ -1,9 +1,8 @@
 //===- llvm/Support/TimeProfiler.h - Hierarchical Time Profiler -*- C++ -*-===//
 //
-// The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===--===//
 
@@ -33,7 +32,7 @@
 /// Write profiling data to output file.
 /// Data produced is JSON, in Chrome "Trace Event" format, see
 /// https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAySU/preview
-void timeTraceProfilerWrite(std::unique_ptr );
+void timeTraceProfilerWrite(raw_pwrite_stream );
 
 /// Manually begin a time section, with the given \p Name and \p Detail.
 /// Profiler copies the string data, so the