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

[Russell Gallop via Phabricator via llvm-commits]

russell.gallop marked an inline comment as done.
russell.gallop added inline comments.


================
Comment at: lld/ELF/Driver.cpp:544
+    llvm::timeTraceProfilerWrite(profilerOS);
+    llvm::timeTraceProfilerCleanup();
+  }
----------------
ruiu wrote:
> 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?
Okay. I'll do that. Seems best as a follow up change as it will affect the time profiler usage in clang as well.


================
Comment at: lld/ELF/MarkLive.cpp:327
 template <class ELFT> void markLive() {
+  llvm::TimeTraceScope timeScope("GC");
   // If -gc-sections is not given, no sections are removed.
----------------
MaskRay wrote:
> Probably just reuse the function name: markLive
As I mentioned above, I would like these to make sense to linker users. Is that okay?

Is there a better place to measure for GC?


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

https://reviews.llvm.org/D71060