[PATCH] D71060: [LLD][ELF] Add time-trace to ELF LLD (2/2)

[Rui Ueyama via Phabricator via llvm-commits]

ruiu accepted this revision.
ruiu added a comment.

LGTM



================
Comment at: lld/ELF/Driver.cpp:544
+    llvm::timeTraceProfilerWrite(profilerOS);
+    llvm::timeTraceProfilerCleanup();
+  }
----------------
anton-afanasyev wrote:
> anton-afanasyev wrote:
> > russell.gallop wrote:
> > > ruiu wrote:
> > > > What does this cleanup function do? If some cleanup is needed, can we run it on timeTraceProfilerWrite?
> > > Cleanup disables the profiler and deletes the data.
> > > 
> > > This is the design as from the original addition of the time profiler (https://reviews.llvm.org/D58675). I think it allows more flexibility (e.g. we may want to write a text report of the same data) though I don't think we use that flexibility at the moment. I'm not sure what the original reason for this was. @anton-afanasyev please can you comment on why this is and whether `timeTraceProfilerCleanup` could be combined with `timeTraceProfilerWrite`? Thanks.
> > Yes, that is for the flexibility in a future. We may want in follow-ups to support different `Writers` (for instance, to terminal). But you are right it could be combined with current `Writer` for now.
> *I mean any kind of another output format, short summary for terminal, for instance.
OK, I prefer merging the cleanup function with write function because (1) it's less error-prone, and (2) if you need to write a result to two different stream, you can easily do that by writing to a string buffer and then write the buffer contents to two streams. Do you mind if I ask you do that as a follow-up patch?


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

https://reviews.llvm.org/D71060