This is an archive of the discontinued LLVM Phabricator instance.

[libc++][PSTL] Add a simple std::thread backend
ClosedPublic

Authored by philnik on May 10 2023, 11:17 AM.

Details

Reviewers
ldionne
EricWF
Group Reviewers
Restricted Project
Commits
rGe837f4b7dbc3: [libc++][PSTL] Add a simple std::thread backend
Summary

This is just to test that the PSTL works with parallelization. This is not supposed to be a production-ready backend.

Diff Detail

Event Timeline

philnik created this revision.May 10 2023, 11:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 10 2023, 11:17 AM
philnik requested review of this revision.May 10 2023, 11:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 10 2023, 11:17 AM
Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 10 2023, 11:17 AM

I think this LGTM w/ CI passing. I'd like to see again once rebased and passing though.

libcxx/include/__algorithm/pstl_backends/cpu_backends/thread.h
21

__assert instead, and put this above with the other includes.

52

_LIBCPP_ASSERT

libcxx/include/__config
1281–1283

Do we still need this? If not let's remove it.

philnik updated this revision to Diff 521141.May 10 2023, 4:12 PM
philnik marked 2 inline comments as done.

Address comments

libcxx/include/__config
1281–1283

It's still used by the __pstl/ headers, so I'd rather keep it for now.

philnik updated this revision to Diff 521475.May 11 2023, 3:28 PM

Generate files

philnik updated this revision to Diff 521776.May 12 2023, 1:20 PM

Try to fix CI

philnik updated this revision to Diff 522183.May 15 2023, 7:32 AM

Try to fix CI

philnik updated this revision to Diff 522288.May 15 2023, 12:02 PM

Different approach

philnik updated this revision to Diff 522347.May 15 2023, 2:54 PM

Try to fix CI

philnik updated this revision to Diff 522638.May 16 2023, 8:28 AM

Try to fix CI

EricWF requested changes to this revision.May 16 2023, 12:07 PM
EricWF added a subscriber: EricWF.

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?

libcxx/include/__algorithm/pstl_backends/cpu_backends/thread.h
27

Small nit. These namespaces add a bunch of size to the mangled names. Do we need to be this descriptive?

This revision now requires changes to proceed.May 16 2023, 12:07 PM
EricWF added inline comments.May 16 2023, 12:14 PM
libcxx/include/__algorithm/pstl_backends/cpu_backends/thread.h
39

This doesn't need a capture default.

46

This can overflow in crazy large inputs, right?

I don't know that it's a problem, but I wanted to point it out.

56

Is there somewhere in the spec it says we don't want to copy the functor? That seems potentially racy?

64

What happens here if the user supplied function throws?

libcxx/src/pstl/thread_backend.cpp
19 ↗(On Diff #522638)

Why are we doing void* here?

I really really hate losing the type safety.

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.

libcxx/include/__algorithm/pstl_backends/cpu_backends/thread.h
46

It could hypothetically overflow, but I don't think that is a problem for real code. This will also change later anyways.

56

No, but it also doesn't prevent us from calling this from multiple threads.

64

We terminate when the user throws inside a thread, so it's not relevant.

libcxx/src/pstl/thread_backend.cpp
19 ↗(On Diff #522638)

To avoid the transitive includes. As said in the description, this is not production-ready.

philnik updated this revision to Diff 522752.May 16 2023, 1:06 PM
philnik marked an inline comment as done.

Try to fix CI

EricWF added inline comments.May 17 2023, 9:22 AM
libcxx/include/__algorithm/pstl_backends/cpu_backends/thread.h
33

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.

51

This seems like a correctness issue we could easily find in testing, and we probably don't need a dynamic check for it here.

56

Right, but given that we just copied it. It seems wiser to also copy it as well for each thread.

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.

I'm a bit worried about shipping these patches then. I normally prefer to have a bunch more certainty when committing stuff to non-experimental bits of the library. @ldionne, you're cool with this plan?

When will we know what we need?
When will the API be stable?

Because providing an API for vendors here is going to be important.

libcxx/include/__algorithm/pstl_backend.h
89

Mid-header includes are really gross. Can we avoid that?

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.

I'm a bit worried about shipping these patches then. I normally prefer to have a bunch more certainty when committing stuff to non-experimental bits of the library. @ldionne, you're cool with this plan?

This part of the library is experimental?

When will we know what we need?
When will the API be stable?

Because providing an API for vendors here is going to be important.

I don't know exactly what you mean by "here". Do you mean as part of the std::thread backend? If yes, I very much disagree. This is already layer two of abstractions which vendors can customize. Adding a third one would just be absurd.

philnik updated this revision to Diff 523091.May 17 2023, 10:06 AM

Try to fix CI

libcxx/include/__algorithm/pstl_backends/cpu_backends/thread.h
33

Frankly, I don't really care. As I already said, this is a shitty backend and shouldn't be used in production. This is more of a placeholder than anything else. We should implement a proper thread pool later and not instantiate threads every time an algorithm is called.

EricWF requested changes to this revision.May 17 2023, 10:13 AM
EricWF added inline comments.
libcxx/include/__algorithm/pstl_backends/cpu_backends/thread.h
33

OK, then I don't know that we should commit this.

What's the purpose of doing it poorly now, rather than properly later?

This revision now requires changes to proceed.May 17 2023, 10:13 AM
philnik added inline comments.May 17 2023, 10:18 AM
libcxx/include/__algorithm/pstl_backends/cpu_backends/thread.h
33

The purpose is that we can check that dispatching etc. works with multiple backends and we avoid assuming too much about what the backend does, since we would otherwise run everything on a single thread.

EricWF added inline comments.May 17 2023, 11:44 AM
libcxx/include/__algorithm/pstl_backends/cpu_backends/thread.h
33

OK, that seems like something that we don't need to check in in order for it to be initially tested.

Then, when we have a backend ready for production, we can check that in.

Or alternatively, we can check the backend in under test/ as a test header.

philnik updated this revision to Diff 523377.May 18 2023, 7:41 AM

Try to fix CI

I'm a bit worried about shipping these patches then. I normally prefer to have a bunch more certainty when committing stuff to non-experimental bits of the library. @ldionne, you're cool with this plan?

Yes, I believe that's the best way to make progress here. We're implementing different backends to make sure that the customization points work and get a feel for how we want things to look like. However, making the backend really good would be wasted time at this point because we don't know whether we'll have to change some of the customization points. This is all guarded behind -fexperimental-library, which means that we're not actually shipping any of it by default.

Don't get me wrong, we'll make the backend(s) better before declaring this non-experimental, but right now we're trying to figure out the backend API and how to implement algorithms in terms of each other more than we're trying to create a really well optimized backend implementation.

ldionne accepted this revision as: ldionne.May 19 2023, 8:24 AM

As I've said before, the goal of this backend is for us to figure out the customization points and create the plumbing for adding new CPU back-ends, and to weed out any issues we see while bringing up this "toy" backend. This is *not* the way the std::thread backend will look in the end, however it would be a waste of time to spend time optimizing it at this point since we're not certain what the CPU backend API will need to look like in the end. We suspect it'll look pretty much like the original PSTL customization points, but we might discover the need to do some things differently as we look into other backends (e.g. GCD). If that happens, any work put in making the std::thread backend good right now would potentially be wasted.

So given the spirit of this patch, I'm fine with it -- basically my bar here is:

  1. I'm happy with the pluming it adds
  2. The implementation looks correct and it passes tests

@EricWF Please let us know if you still see an issue with this plan.

libcxx/include/__algorithm/pstl_backends/cpu_backends/thread.h
30

Can you add a comment here explaining that this implementation is experimental and will be replaced by a real implementation when the library is ready for production? Just to make it clear it was never meant to be used prime time. This should be clear from the need to use -fexperimental-library, but a comment here clarifies even more, I guess.

philnik updated this revision to Diff 523835.May 19 2023, 10:10 AM
philnik marked an inline comment as done.
  • Address comments
  • Try to fix CI

After discussing with @EricWF, we agreed that the actually relevant part of this patch is not the implementation of the threading backend, but rather the plumbing around it (e.g the CMake configuration, the #ifdefs in pstl_backend.h, the __config_site bits, etc.). As such, I think we should rip out the .cpp file and only implement __parallel_for and __cancel_execution in a serial way here (with a comment saying it's temporary). Then, once we have the full CPU backend API checked in (e.g. __parallel_transform_reduce and friends), we can go back and create a reasonable std::thread backend implementation.

philnik updated this revision to Diff 524452.May 22 2023, 1:17 PM

Address comments

ldionne accepted this revision.May 24 2023, 8:05 AM

LGTM, this addresses the issues I had discussed with @EricWF .

EricWF resigned from this revision.May 24 2023, 3:26 PM

I'm going to unblock this. Because I don't think it's harmful.

However, It's my view that ff the goal of this whole project is to nail down the underlying API and customization points, this change doesn't help reach said goal.
In my opinion, the work that should be done first here, is work that allows other users to specify their own backends.

This revision is now accepted and ready to land.May 24 2023, 3:26 PM
This revision was automatically updated to reflect the committed changes.
Via Conduit