In D71059#1838862, @Quuxplusone wrote:

I'm not qualified to comment on the functionality. This seems like it'll be OK with respect to thread_local on Mac OSX. No further comments from me.

Thanks, added const.

Add consts to references in lambda functions.

Ping. Okay to land new version?

In D73162#1834379, @asbirlea wrote:

Oh, wow! Might I ask you add the same for fmul? We may get a revision like that next time :).
Thank you!

Update to check for start of metadata.

Ping. Is this okay to land, or have I missed something? Thanks.

In D72399#1820866, @rnk wrote:

lgtm

I checked that __declspec(thread) and thread_local seem to be functionally equivalent with MSVC.

Fix condition

In D72399#1810145, @dblaikie wrote:

Sounds good to me - but probably good to get someone who works on the MSVC platform (like @rnk ) to confirm.

In D71786#1812388, @riccibruno wrote:

Am I the only one who is a little bit uncomfortable with including a replacement to the system allocator in LLVM?

At first, I configured rpmalloc in the "unlimited" mode because I wanted maximum performance. I now reverted to default rpmalloc flags, and the memory usage has decreased (albeit still higher than the Windows Heap), with no impact on link time (meaning that timings are still the same as in the Summary). Below are "Max" values when linking clang.exe with ThinLTO:

In D71786#1807883, @maniccoder wrote:

I would say it's mostly down to application usage patterns. The worst case is probably a gc-like usage where one thread does all allocation and another all deallocation, as this will cause all blocks to cross the thread cache via an atomic pointer CAS and eventually cause larger spans of blocks to traverse the thread caches via the global cache. However, even this scenario will probably be significantly faster than the standard runtime allocator.

I have not had the opportunity to test this on other hardware than single-socket consumer hardware, game consoles and handheld devices, mostly due to me working in that spectrum and being the sole developer of it. It does seem that @aganea has tried it on more service facing hardware, perhaps he can give additional input.

In D71786#1807550, @maniccoder wrote:

In D71786#1802970, @russell.gallop wrote:

1). This also seems to improve compile time by around 5% for me. Presumably this is due to some other benefit of rpmalloc as (AFAIK) clang.exe does not use multiple threads.

Yes, generally rpmalloc improves single-threaded allocation performance over the default runtime, especially for small block allocations (below 1KiB), enough to warrant using it only for this use case.

Add comment to clarify usage of timeTraceProfilerCleanup.

Fix memory leak.

I reverted this at the end of last year MacOS build issues.

Abandoning this review. Will roll fix for these issues into new version on D71059.

I have done some performance evaluations here and LLVM_ENABLE_RPMALLOC does seem to improve ThinLTO link times and CPU utilisation during ThinLTO.

In D71786#1802386, @aganea wrote:

In D71786#1801275, @russell.gallop wrote:

It fails creating a library

Could you please run ninja -v, and ensure LLVMSupport.lib appears first on the linker cmd-line? (before other libs).

Thanks for the patch. I applied it (to 8188c998ffa4d20253444b257402907d2aa74dc2), specified LLVM_ENABLE_RPMALLOC=ON and -DLLVM_USE_CRT_RELEASE=MT and tried to build with MS cl.exe/link.exe. It fails creating a library:

Ping! Okay to land this? (I moved it back to needs review as this was very different from before).

@ruiu or @Kai I would like this to go in with D71060. Are you okay with this revised fix?

Rename scopes as requested.

Removed "Merge input sections" scope.
Also moved "ExecuteLinker" to cover longer time period, including initLLVM() etc..

In D71060#1787251, @MaskRay wrote:

I have a general question about the llvm::TimeTraceScope timeScope("LTO"); trace sites. Shall we just use the container function name if applicable?

More specific implementation. Just avoid thread_local when LLVM_ENABLE_THREADS=OFF.

In D71548#1787415, @teemperor wrote:

I don't think LLVM_THREAD_LOCAL is correct here. unique_ptr isn't a POD so this doesn't really fit to the use case of` LLVM_THREAD_LOCAL`. Maybe we should have a LLVM_NONTRIVIAL_THREAD_LOCAL. that doesn't fall back to __thread. Or we just use raw pointer here instead of a global unique_ptr (it seems that we anyway use it here just as an annotation to describe memory ownership?).

Fix to work with LLVM_ENABLE_THREADS=OFF. In this mode ThinLTO still uses tasks but need to avoid re-initialising the time profiler.

In D71059#1785750, @Quuxplusone wrote:

This patch breaks the build on Apple Clang (Mac OSX 10.13.6).

$ ninja clang
[27/2870] Building CXX object lib/Support/CMakeFiles/LLVMSupport.dir/TimeProfiler.cpp.o
FAILED: lib/Support/CMakeFiles/LLVMSupport.dir/TimeProfiler.cpp.o 
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++  -DGTEST_HAS_RTTI=0 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib/Support -I/llvm-project/llvm/lib/Support -I/usr/include/libxml2 -Iinclude -I/llvm-project/llvm/include -fPIC -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -O2 -g -DNDEBUG   -std=c++14  -fno-exceptions -fno-rtti -MD -MT lib/Support/CMakeFiles/LLVMSupport.dir/TimeProfiler.cpp.o -MF lib/Support/CMakeFiles/LLVMSupport.dir/TimeProfiler.cpp.o.d -o lib/Support/CMakeFiles/LLVMSupport.dir/TimeProfiler.cpp.o -c /llvm-project/llvm/lib/Support/TimeProfiler.cpp
In file included from /llvm-project/llvm/lib/Support/TimeProfiler.cpp:13:
/llvm-project/llvm/include/llvm/Support/TimeProfiler.h:17:8: error: thread-local storage is not supported for the current target
extern thread_local std::unique_ptr<TimeTraceProfiler>
       ^
/llvm-project/llvm/lib/Support/TimeProfiler.cpp:36:1: error: thread-local storage is not supported for the current target
thread_local std::unique_ptr<TimeTraceProfiler> TimeTraceProfilerInstance =
^
2 errors generated.

I guess this is related to the LLVM_ENABLE_THREADS issue mentioned above.

In D71059#1785471, @Kai wrote:

This PR requires TLS support even if LLVM_ENABLE_THREADS is set to OFF. Is this intended?

Fix recent review comments.

Thanks for the comments. I believe I've fixed them all now.

Add test for writing trace to stdout

Change to appending .time-trace as default name.
Make -time-trace-file a separate option
Use temp names in tests

Removed StringRef("") in several places after D71347.
Added some more time scopes to account for gaps in time trace.
Updated tests for args/detail not always being present.

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.

In D71059#1778898, @ruiu wrote:

Can you share an example output of lld with this patch?

Add tests to patch.

In D71059#1776973, @anton-afanasyev wrote:

I mean the test like clang/test/Driver/check-time-trace.cpp, but for the multithreaded invocation. Yes, for instance, for ThinLTO (but I see no tests in the D71060).

In D71059#1776620, @anton-afanasyev wrote:

Btw, is it planned to add test for multithreading?

Committed in 061123f05ff0813c36287d2105cb5344d494592b. Closing.

Agreed that this usage shouldn't be encouraged so abandoning review.

Thanks for the updated timings. I have no objection to this going in.

I've re-implemented this using thread local time tracing.

In D69825#1760400, @aganea wrote:

In D69825#1760373, @russell.gallop wrote:

It looks like the git apply didn't work, but the script continued so this was a duff experiment, please ignore. I'll try to fix this and run it again.

In D69825#1760279, @russell.gallop wrote:

So in this case it saved 0.5% of time. Using the previous maths, with 2378 clang-cl jobs, this implies process creation time of 29ms. This was fairly soon after a reboot. Maybe I'm just lucky and none of the process creation problems are affecting me on this system (at this moment).

In D69825#1758949, @aganea wrote:

Thanks for the feedback Russell!

Can you possibly try again with /MT? (ie. -DLLVM_USE_CRT_RELEASE=MT)

Thanks for this change. I applied this patch (prior to splitting out https://reviews.llvm.org/D70568) and it built and worked okay (I only see one clang-cl in process explorer).