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

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: lld/COFF/Driver.cpp:50
 
+static Timer InputFileTimer("Input File Reading");
+
----------------
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.


================
Comment at: lld/Common/Timer.cpp:80-87
+  int LI = Depth * 2;
+  int RI = std::max<int>(0, 30 - Name.size() - 1 - LI);
+  SmallString<32> Str;
+  llvm::raw_svector_ostream Stream(Str);
+  repeat(Stream, ' ', LI);
+  Stream << Name << ":";
+  repeat(Stream, ' ', RI);
----------------
ruiu wrote:
> zturner wrote:
> > ruiu wrote:
> > > zturner wrote:
> > > > ruiu wrote:
> > > > >   std::string S = (std::string(' ', Depth * 2) + Name).str();
> > > > >   format("% 30s (%6.2f%%) %5d ms", S, 100 * millis() / TotalDuration, (int)millis());
> > > > > 
> > > > > is perhaps a bit easier to read?
> > > > This isn't quite the same.  printf right justifies `%30s`, I want it left justified.  I also don't want it left justified to 30, because the indentation would have changed that.  I think it's easier to just explicitly pad on the left and pad on the right.
> > >   std::string S = (std::string(' ', Depth * 2) + Name).str();
> > > 
> > > indents the string, no?
> > Yes, but then the `%30s` is no longer correct, because some of the 30 has been consumed by the `Depth*2` characters on the left.  The point is that after indenting, the field width is no longer a constant.  That said, I think this code is pretty simple already no?
> Aah, sorry I misunderstood the issue. Can't you use '% -30s'? It left-justifies the string.
If we append the spaces first to construct a temporary string then that should work, but that results in some extra copies.  I don't think it would result in shorter or fewer lines of code either.


https://reviews.llvm.org/D41915