[PATCH] D122922: [lld][common][lld-macho][lld-coff] Support per-thread allocators and StringSavers

Tue Apr 12 10:28:13 PDT 2022

oontvoo added a comment.

In D122922#3444540 <https://reviews.llvm.org/D122922#3444540>, @MaskRay wrote:

> In the `LLD_THREAD_SAFE_MEMORY` code path, `make<Foo>(...)` now has significantly larger overhead due to the `pthread_getspecific` call (via `llvm/Support/ThreadLocal.h`).
> A large number of `make<...>` instantiations in lld do not benefit from per-thread allocation.

Making individual pieces of code make this distinction (ie., thread safe vs no thread safe) is a bit bug prone. ie.,. a piece of code initially thinks that it doesnt need to be thread safe, but later ends up on a multi-thread code path.. There are a fair number of cases like this in MachO.
That's why we thought it's safer to have one setting: threadsafe for all or not thread safe for all.

Having said that, I understand your concern wrt performance in ELF. So how about offering these:

- make<>() : can alias to either of the following depending on a flag
- makeUnsafe<>(): Always use the global/shared allocator
- makeThreadSafe<>() Always use thread-local allocators

This way, ELF can retain its current behaviour. MachO can choose to be all-thread safe if it wants.

> Re:  native TLS vs LLVM's ThreadLocal

As noted in the inline comment, I agree that it seems like it could be slower but benchmarks didn't show any difference. I don't have a strong preference either way (except that LLVM"s ThreadLocal simplifies the code a bit ....)
So if anyone absolutely has a strong objection to either approach, please raise your concern now. If not, I will go with the more popular recommendation on this patch.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122922