[PATCH] D142317: [Support][LLD] Avoid using uninitialized threadIndex.

[Andrew Ng via Phabricator via llvm-commits]

andrewng added a comment.

> I also have a connected question not related directly. What do you think about allocator from D142318 <https://reviews.llvm.org/D142318> and following design for such allocator:
>
>   class ThreadPoolAllocator {
>     void *Allocate(size_t size, size_t alignment);
>   };
>   
>   class ThreadPool {
>     std::unique_ptr<ThreadPoolAllocator> getAllocator ();
>   };
>   
>   class ThreadPoolExecutor {
>     std::unique_ptr<ThreadPoolAllocator> getAllocator ();
>   };
>
> The idea is to have thread-safe allocator connected with its execution context.

I think you need to be a bit careful with terminology because IIUC it's not actually "thread-safe" but a "per thread" allocator. I think this could be useful in some situations but perhaps a better overall approach would be to use a low-level thread aware memory allocator. I think some toolchains/runtimes already have such an allocator and there are other options such as `rpmalloc` and `mimalloc`. This way, the benefit is more widespread and doesn't require any extra effort (although some allocator libraries have some limitations on certain platforms).



================
Comment at: llvm/lib/Support/Parallel.cpp:231-232
     }
     for (; Begin != End; ++Begin)
       Fn(Begin);
     return;
----------------
avl wrote:
> andrewng wrote:
> > avl wrote:
> > > andrewng wrote:
> > > > andrewng wrote:
> > > > > Perhaps a "simpler" fix would be not to use the main thread here, i.e. should use `TG.spawn`? However, that assumes that `getThreadIndex()` is only for use in the "`parallelFor`" style functions otherwise the other parallel functions would also need to avoid using the main thread too.
> > > > > 
> > > > > I'm surprised that this hasn't been causing more issues.
> > > > My gut feeling is that this would be the "better" solution, i.e. not including the invoking thread (might not be the main thread) as part of the pool of worker threads. It just feels cleaner to me.
> > > > 
> > > > However, can we also assume that the use of `getThreadIndex()` is not supported with regards to `parallel_sort` which I believe can also make use of the invoking thread? I think this would be reasonable.
> > > yes, we can avoid using main thread in this case.
> > > 
> > > Though, it seems, having the same index for main thread and for one from ThreadPool threads is a potential bug. We would always need to remember to avoid using main thread and ThreadPool threads. Instead, it looks better, to always have different indexes. As this patch does : 0 for main thread, 1... for others.
> > > 
> > > How about using TG.spawn in this place, but still having different indexes for main thread and others?
> > My feeling is that the current `getThreadIndex()` should really only apply in the context of the parallel thread pool threads and the parallel algorithms. Remember there are other thread pools in LLVM too and there may be more threads than just the main one (although perhaps these would just share the same index of `0`?). Other threads may also not be long-lived.
> > 
> > Mixing the concept of the parallel `getThreadIndex()` with other threads just doesn't feel right.
> I see. To summaries the changes :
> 
> 1. remove zero initialization.
> 2. remove advancing index for zero thread.
> 3. use  TG.spawn  instead of main thread.
> 
> correct?
To keep things simple, I think the above change, i.e. avoiding the use of the main thread, is all that's required to fix the important concurrency issue. Any other changes related to D142318 can be moved to that patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142317