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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a comment.

In D150284#4347301 <https://reviews.llvm.org/D150284#4347301>, @EricWF wrote:

> The `__threading` header, which defines a stable API that libc++ uses to implement threading primitives. This has been used by a bunch of vendors to shim in the different implementations for their platforms.
> I believe we'll want to do the same thing here.
>
> Do we know what the full API will look like yet?

We don't know exactly which parts we will require, but I don't think that's relevant for this patch. We only implement a single (primitive) backend here, which will most likely not exist for a long time in this form.



================
Comment at: libcxx/include/__algorithm/pstl_backends/cpu_backends/thread.h:45
+
+  const auto __brick_size = ((__last - __first) / static_cast<__diff_t>(__thread_count)) + 1;
+
----------------
EricWF wrote:
> This can overflow in crazy large inputs, right? 
> 
> I don't know that it's a problem, but I wanted to point it out.
It could hypothetically overflow, but I don't think that is a problem for real code. This will also change later anyways.


================
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);
----------------
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.


================
Comment at: libcxx/include/__algorithm/pstl_backends/cpu_backends/thread.h:63
+
+  __thread_cpu_backend::__join_threads_and_destroy_pool(__thread_pool);
+}
----------------
EricWF wrote:
> What happens here if the user supplied function throws?
We terminate when the user throws inside a thread, so it's not relevant.


================
Comment at: libcxx/src/pstl/thread_backend.cpp:19
+
+void* __make_thread_pool(unsigned expected_thread_count) {
+  auto pool = new std::vector<thread>;
----------------
EricWF wrote:
> Why are we doing `void*` here?
> 
> I really really hate losing the type safety.
To avoid the transitive includes. As said in the description, this is not production-ready.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150284