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

Fri Apr 22 07:48:35 PDT 2022

aganea added a subscriber: sbc100.
aganea added a comment.

@int3 I don't want to block this, I understand your needs, and I value the intention of this patch. I'd be happy to reintroduce `llvm::sys::ThreadLocal` later. However I wasn't able to reproduce the 1% regression (see D123879 <https://reviews.llvm.org/D123879> for numbers), although I don't deny it could exist in some configurations (depending on the machine, OS, LLVM build options, allocator). I'm just wondering if 1% regression isn't acceptable for now in the trunk, so that we can commit a more frictionless version of this patch.

In D122922#3446555 <https://reviews.llvm.org/D122922#3446555>, @mstorsjo wrote:

> In D122922#3446489 <https://reviews.llvm.org/D122922#3446489>, @oontvoo wrote:
>
>>>> ! In D122922#3446376 <https://reviews.llvm.org/D122922#3446376>, @mstorsjo wrote:
>>>
>>> Can someone summarize the high level roadmap of this feature? If threadsafe allocation is going to be optional/opt-in, lld can't generally rely on that, right? Or is it meant as a gradual way of merging the code and trying it out, and after it's ready to be enabled unconditionally, it can be relied upon in lld in general?
>>
>> Yes, as it stands, the threadsafety is controlled by the new `LLD_THREAD_SAFE_MEMORY`. The intention is for each LLD ports to decide whether they want threadsafe allocator/saver (and not for *users* of lld).
>> For MachO, I think we'd like to enable it (already been hit by a few race-condition bugs). Can't speak for ELF/others ...
>
> Right, but if the MachO part of lld does allocations from multiple threads, then it's essentially unreliable unless this option is enabled? So anyone building lld wanting to use the MachO part of it would need to enable it.

This question was left unanswered -- how do you plan on using `LLD_THREAD_SAFE_MEMORY`? Will it be enabled only when building for Darwin? What happens for cross-compilation or if `LLD_THREAD_SAFE_MEMORY` isn't enabled? The make/makePerThread/makeUnsafe APIs seem a bit error-prone to me. Ideally it is a choice that should be avoided, if possible, by the business logic/driver developer.

It'd be nice if other drivers owners would pitch in to accept/veto this patch. @sbc100 @mstorsjo @MaskRay

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122922