Hmm, I went for this as it was more consistent with the comparable compiler option.
I do find this behaviour useful if you are tracing a build such as llvm which has a lot of links. While this probably isn't typical, it does make it easy to send all of the link traces to separate files just by adding a single link option to all links.
What about an option like "-ftime-trace -" to send to stdout?
Thanks. Will fix.
This is used for "Detail". E.g. optimisation passes specify which pass they are running on "OptModule" specifies which module it is running on. This helps to distinguish multiples of the same kind of scope in traces. I don't think there is anything useful to detail at this level.
I could add an overloaded constructor to TimeTraceScope with a default to not add "detail". That would cut down on the StringRef("") argument which come up quite a bit. It could also cut down on file size by not writing that block out.
Here are some examples of the output from LLD. One for a llvm-tblgen ThinLTO link (-time-trace-granularity=50000) and a clang Release build (-time-trace-granularity=500). The clang link has some significant gaps in it. I'll see if I can identify where they're from and trace them.
Yeah that makes sense, but I think that ".json" extension is the problem. We may want to add a different feature that outputs some data in the JSON format, and if we choose the same design, the option will conflict with the feature that you are adding. How about adding ".time-trace" to an output file then? Also, I don't think we should replace an extension -- usually Unix commands don't have extensions. I'd just append ".time-trace"
I think that giving two options the same name isn't very conventional as a Unix command, as --foo bar and --foo=bar are usually considered the same option. Could you rename the latter --time-trace-file?
These JSON outputs look nice! One feature request -- is there any way to add a key-value to an output JSON file? I mean if we can add something like "argv": ["ld.lld", "-o", ...] to the JSON output it would be great. (You don't need to do that in this patch though.)
s/time trace/time trace profiler/
I'd move this above initLLVM() so that we can measure time consumed by createFiles and other functions.
Please remove llvm::.
Let's use the same condition if (config->timeTraceEnabled) as before for consistency.
Add a single line comment -- // Write the result of the time trace profiler.
You could simplify this a little bit as shown below:
std::string path = args.getLastArgValue(OPT_time_trace_file_eq); if (path.empty()) path = (config->outputFile + ".time-trace").str();
Let's use shorter conventional names: ec and os
What does this cleanup function do? If some cleanup is needed, can we run it on timeTraceProfilerWrite?
This is the entry point function of ICF, so please move the TimeTraceScope here.
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.
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?
I have a general question about the llvm::TimeTraceScope timeScope("LTO"); trace sites. Shall we just use the container function name if applicable?
This comment may be misleading.
It creates MergeSyntheticSection's and does other tasks that cannot be summaries by "Merge input sections".
Probably delete the trace here. It shouldn't take a lot of time anyway.
Probably just reuse the function name: markLive
The clang --time-trace feature (https://aras-p.info/blog/2019/01/16/time-trace-timeline-flame-chart-profiler-for-Clang/) is intended to be helpful for users understanding what the tools are doing with their code, not (just) LLVM developers, and I think that this should have the same aim in LLD.
As such, I tried to add scopes that correspond to user visible features (e.g. LTO, GC, ICF), rather than the functions which implement them. In some places this can be tricky as scopes don't always correspond to notional blocks (e.g. clang Frontend). I would prefer to stick with the current names if possible, though there may be better places for them.
Okay, I'll remove this.
For code blocks, using a descriptive name seems fine. For a trace added for a whole function, I still prefer using the function name as the trace point name. I think for users who are so familiar with the linker that they know the existence of --time-trace and will like to investigate the bottleneck, the function names should not impair their understanding of the pass names.
I'd prefer "LinkerDriver::link".
This is the best place, though I still feel "GC" as the name is less ideal than the function name "markLive".
Unified scope name like "Link" is good for grouping blocks by chrome://tracing app. Isn't it better to put "LinkerDriver::link" to Details field of timeScope? (though this field is usually used for _user_ source code names, but here it is unused).
I'll put "LinkerDriver::link" in the Details field.
Longer term it might be better to add another "args" field for function name to distinguish compiler source code names and user source code names.