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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Overall looking pretty good.



================
Comment at: lld/COFF/Driver.cpp:50
 
+static Timer InputFileTimer("Input File Reading");
+
----------------
ruiu wrote:
> zturner wrote:
> > ruiu wrote:
> > > zturner wrote:
> > > > ruiu wrote:
> > > > > zturner wrote:
> > > > > > ruiu wrote:
> > > > > > > zturner wrote:
> > > > > > > > ruiu wrote:
> > > > > > > > > Can you remove `addChildTimer` if you pass a parent timer to the constructor?
> > > > > > > > No because we don't want every timer to be enabled always.  Only if a certain branch of code is run (for example we only want to add PDB timers if /DEBUG is specified)
> > > > > > > But I don't think we need a separate flag. It seems you can show timing information only when a timer has non-zero time duration (which means the timer has activated and deactivated).
> > > > > > That's an option, but would there be issues if we use a "Parent" that is defined in another translation unit.  The child will call `parent->Children.add(*this)`.  Can we guarantee that the Parent has been constructed at that point?  I always forget when it's safe for one global variable to use another.  If we have to introduce `ManagedStatic` or something then I think it just makes the code more complicated.
> > > > > Good point, but you could add a timer to its parent not when the constructor is executed but when it's start()'ed.
> > > > We might start and stop it many times though (we already do this in `PDB.cpp` with `TypeMergingTimer`, for example).  So if we do it that way we'd need to make sure it hasn't already been added, at which point we have to use a `map` of some kind instead of a simple vector.
> > > I don't think we need a map. A timer can add itself to a parent timer first time start() is called -- which is when the timer duration is zero.
> > Currently a timer does not store a pointer to its parent.  In order to do this it would have to store that.  Should I make that change?
> Good point, and I'm almost convinced, but I'd think we still want it. The current API is easy to misuse. I think you forgot to call addChildTimer for LTOTimer.
Ah, this is a very nice way to force some initialization order. Thank you for doing this.


================
Comment at: lld/include/lld/Common/Timer.h:35-41
+  std::chrono::time_point<std::chrono::high_resolution_clock> StartTime;
+  std::chrono::nanoseconds Total;
+  llvm::SmallVector<Timer *, 2> Children;
+  std::string Name;
+  Timer *Parent;
+  void print(int Depth, double TotalDuration, bool Recurse = true) const;
+  explicit Timer(llvm::StringRef Name);
----------------
Please move private members after public members.


================
Comment at: lld/include/lld/Common/Timer.h:37
+  std::chrono::nanoseconds Total;
+  llvm::SmallVector<Timer *, 2> Children;
+  std::string Name;
----------------
I'd use std::vector when we do not have to optimize for speed.


================
Comment at: lld/include/lld/Common/Timer.h:41
+  void print(int Depth, double TotalDuration, bool Recurse = true) const;
+  explicit Timer(llvm::StringRef Name);
+
----------------
This constructor is used only by `root()`, so I'd make this private to prevent misuse of this constructor.


================
Comment at: lld/include/lld/Common/Timer.h:44
+public:
+  friend ScopedTimer;
+
----------------
It looks like ScopedTimer accesses only public functions of this class.


https://reviews.llvm.org/D41915