[PATCH] D41915: [lldCOFF] Print detailed timing information with /VERBOSE

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/COFF/Driver.cpp:715
 bool LinkerDriver::run() {
+  Timer::TimerPhaseRAII Timer =
+      Config->Metrics.startPhaseRAII(TP_InputFileReading);
----------------
`Timer::TimerPhaseRAII` is too long. I'd name this `ScopedTimer` and name the local variable `T`. As long as you use T as a name for the timer, there should be no confusion. I imagine that you could use it like this:

  ScopedTimer T(TotalTimer);


================
Comment at: lld/COFF/Timing.h:17
+namespace coff {
+enum TimingPhases {
+  TP_Total,
----------------
Instead of defining one timer which contains multiple timers in it, can you define a timer for each measurement like this?

  namespace lld {
  namespace coff {
  Timer TotalTimer("Total");
  Timer FileReadTimer("Input File Reading");
  ...
  }
  }

Then you can remove this enum.


================
Comment at: lld/Common/Timer.cpp:14
+void Timer::TimerPhase::stop() {
+  auto EndTime = std::chrono::high_resolution_clock::now();
+  Total += (EndTime - StartTime);
----------------
Please do not use `auto`.


================
Comment at: lld/Common/Timer.cpp:18
+
+float Timer::TimerPhase::millis() const {
+  return std::chrono::duration_cast<std::chrono::duration<float, std::milli>>(
----------------
Use double unless there's a reason to use float.


================
Comment at: lld/Common/Timer.cpp:34
+  // <indent>name:<padding for alignment>
+  message(formatv("{0}({1,+6}%) {2:ms}",
+                  fmt_pad(Name + ":", LI, RI), // <indent>name:<padding>
----------------
Instead of formatv, please use llvm::format which takes printf-style format string. It is more familiar (and thus easy to read) to most people who work on lld.


================
Comment at: lld/Common/Timer.cpp:103
+
+int Timer::calculateNameFieldWidth(int Depth, const TimerPhase &Phase) const {
+  // indentation, then name, then a colon and a space.
----------------
This is a bit too overdesigned at the moment. Just use some fixed width (e.g. 20 chars) as the width of the name field. I'd reduce the amount of code as much as possible.


================
Comment at: lld/include/lld/Common/Timer.h:56
+  struct TimerPhaseRAII {
+    TimerPhaseRAII() = default;
+
----------------
I don't think there's a valid use case for this constructor.


https://reviews.llvm.org/D41915