[PATCH] D78030: [TimeProfiler] Emit clock synchronization point

Wed Apr 22 01:36:03 PDT 2020

russell.gallop accepted this revision.
russell.gallop added a comment.

LGTM, with a few small comments.

For the record, I wondered whether the time profiler could emit all ts as absolute, so I tried it out. This has two problems. 1). The "Total" numbers also need adjusting to be at the beginning of the trace time rather than zero, which feels a little odd, and it hard to see how long things are on the time scale. 2). All of the times end up being very large numbers so this bloats traces considerably.

================
Comment at: clang/test/Driver/check-time-trace-sections.py:23
+
+# Make sure that the 'beginningOfTime' is not earlier than 10 seconds ago.
+if seconds_since_epoch - beginning_of_time > 10:
----------------
Could also check that beginningOfTime isn't after seconds_since_epoch.

================
Comment at: llvm/lib/Support/TimeProfiler.cpp:78
   TimeTraceProfiler(unsigned TimeTraceGranularity = 0, StringRef ProcName = "")
-      : StartTime(steady_clock::now()), ProcName(ProcName),
-        Pid(sys::Process::getProcessId()), Tid(llvm::get_threadid()),
-        TimeTraceGranularity(TimeTraceGranularity) {
+      : BeginningOfTime(system_clock::now()), StartTime(steady_clock::now()),
+        ProcName(ProcName), Pid(sys::Process::getProcessId()),
----------------
Note that this will record BeginningOfTime for each TimeTraceProfiler started on a thread, but that won't be used. This shouldn't cause any harm  and I don't think that is very frequent so I think that this is okay.

================
Comment at: llvm/lib/Support/TimeProfiler.cpp:238
+
+    // Emit the absolute time of the moment when this TimeProfiler started
+    // measurements. This can be used to combine the profiling data from
----------------
Nit. This is a bit wordy. How about "Emit the absolute time when this TimeProfiler started."?

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

https://reviews.llvm.org/D78030