[PATCH] D90686: [lld][ELF] Add additional time trace categories

[Russell Gallop via Phabricator via llvm-commits]

russell.gallop added inline comments.


================
Comment at: lld/ELF/Writer.cpp:2182
+  {
+    llvm::TimeTraceScope timeScope("Finalize synthetic sections");
+    // finalizeAddressDependentContent may have added local symbols to the
----------------
grimar wrote:
> jhenderson wrote:
> > grimar wrote:
> > > Does it make sense to name this and the scope above as:
> > > 
> > > "Finalize synthetic sections (1/2)"
> > > "Finalize synthetic sections (2/2)"
> > > 
> > > ?
> > I'd say no, probably, but it depends on what you want the output to look like. By naming both scopes the same, they will appear in the same colour in the chrome://tracing output, and will be combined into a single "Total Finalize synthetic sections" block below the timeline, to give an idea of how long the combined amount takes. Depending on the time taken in the intermediate bit between the two blocks, and the time trace granularity, the two blocks might even be merged into a single block in the output, if I'm not mistaken. Of course, it might be that this "treating them as the same thing" bit is actually unhelpful. To me, they are doing the same thing from a high level, so I think they should be named the same, but I could see an argument that they are different blocks and therefore should be treated differently (this feeds into my thinking that perhaps we should add subscopes for each synthetic section).
> Ah, I didn't know that they will be combined into a single block. My concern was about having 2 different blocks with the same name. This place looks fine to me then.
> Depending on the time taken in the intermediate bit between the two blocks, and the time trace granularity, the two blocks might even be merged into a single block in the output, if I'm not mistaken.

I don't think that they would be merged into a single block. The time trace granularity applies a minimum length to the blocks of time, not to gaps. It's not a sample rate.

Note for example that if each "Finalize synthetic sections" was "< granularity" but the sum was "> granularity" then I don't think it would be traced.

> My concern was about having 2 different blocks with the same name

We ended up with something like this for the clang "FrontEnd" block as it was difficult to identify a single scope which covered all of that. It's not ideal but was the best option we could think of then (https://reviews.llvm.org/D63325).

One option may be to use the second parameter to say something about "which" sections.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90686