Page MenuHomePhabricator

Add an assertion that parallelForEach doesn't nest.
AcceptedPublic

Authored by ruiu on Apr 16 2019, 1:04 AM.

Details

Summary

Add an assertion that parallelForEach doesn't nest.

Event Timeline

ruiu created this revision.Apr 16 2019, 1:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2019, 1:04 AM
andrewrk accepted this revision.Apr 16 2019, 1:12 AM

This will be nice to prevent bugs in the future. Should it be behind #ifndef NDEBUG though? Presumably if assertions are disabled then this bug would be expected to be undefined behavior and avoid the overhead of the thread local variable, is that right?

This revision is now accepted and ready to land.Apr 16 2019, 1:12 AM
ruiu added a comment.Apr 16 2019, 1:18 AM

If you worry about the cost of assigning a boolean value to the thread-local variable, I'd think that's really cheap and negligible. This function is not called too casually, because distributing jobs to threads waiting in the thread pool is not cheap. Compared to that, these assignments are virtually nothing.

That makes sense 👍

MaskRay added inline comments.Apr 16 2019, 6:00 AM
lld/Common/Threads.cpp
13

This doesn't have to be thread_local. This is a fundamental problem of TaskGroup.

template <class IterTy, class FuncTy>
void parallel_for_each(IterTy Begin, IterTy End, FuncTy Fn) {
  TaskGroup TG;
.... // In ~Latch called by ~TaskGroup, all threads of the default executor can block on different `std::mutex`es if there are multiple concurrent `parallel_for_each` invocations.
}

It doesn't need nested parallel_for_each to get stuck. It just needs more concurrent parallel_for_each invocations than the number of executors.

MaskRay added inline comments.Apr 16 2019, 6:07 AM
lld/Common/Threads.cpp
13

The code (with thread_local bool -> std::atomic<bool>) can probably be moved to include/llvm/Support/Parallel.h before it gets improved to allow concurrent invocations.

ruiu marked an inline comment as done.Apr 17 2019, 6:46 PM
ruiu added inline comments.
lld/Common/Threads.cpp
13

Do you think you can fix the function so that parallel_for_each is reentrant? If so, do you mind if I ask you do that?