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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: lld/ELF/Writer.cpp:2182
+  {
+    llvm::TimeTraceScope timeScope("Finalize synthetic sections");
+    // finalizeAddressDependentContent may have added local symbols to the
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90686