[libcxx-commits] [PATCH] D150284: [libc++][PSTL] Add a simple std::thread backend

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF added inline comments.


================
Comment at: libcxx/include/__algorithm/pstl_backends/cpu_backends/thread.h:32
+    auto __max_thread_count = thread::hardware_concurrency();
+    return __max_thread_count == 0 ? 1 : __max_thread_count;
+  }());
----------------
This calculation seems a bit weird to me.

We're basically saying that if no concurrency is reported, then we'll use 2 (not one) threads.
Why are we not just running the algorithm in the current thread in this case?


Either way, it seems like we should be using the calling thread to do some work rather than just waiting on the other threads to join.



================
Comment at: libcxx/include/__algorithm/pstl_backends/cpu_backends/thread.h:50
+  }
+  _LIBCPP_ASSERT(__first == __last, "__parallel_for didn't iterator over all elements!");
+
----------------
This seems like a correctness issue we could easily find in testing, and we probably don't need a dynamic check for it here.



================
Comment at: libcxx/include/__algorithm/pstl_backends/cpu_backends/thread.h:55
+    } else {
+      __thread_cpu_backend::__add_thread(__thread_pool, [&__f, __brick_first = __first, __brick_last = __last]() {
+        __f(__brick_first, __brick_last);
----------------
philnik wrote:
> EricWF wrote:
> > Is there somewhere in the spec it says we don't want to copy the functor? That seems potentially racy? 
> No, but it also doesn't prevent us from calling this from multiple threads.
Right, but given that we just copied it. It seems wiser to also copy it as well for each thread.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150284